Submit a sample of notification images to Safe Browsing

Submit a sample of notification images to Safe Browsing

These will be scanned for social engineering behavior.

Only uploads if all of the following are true:
- User has opted in to SBER_LEVEL_SCOUT.
- Origin is not on CSD phishing whitelist.
- Device has sent < 5 reports in last 24 hours (see persistence TODO).
- Do a dice roll, and keep only 20% of notifications (see Finch TODO).

The notification image bitmap is downscaled to <= 512x512, encoded as a
PNG, then sent to the CSD server as a NotificationImageReportRequest
protobuf from chrome/common/safe_browsing/csd.proto.

BUG=678443

[johnme, 2017-01-11 17:44:00]

johnme@chromium.org changed reviewers:
+ nparker@chromium.org, peter@chromium.org

[johnme, 2017-01-11 17:44:01]

nparker@chromium.org: Please review changes in chrome/*/safe_browsing/

peter@chromium.org: Please review changes in chrome/browser/notifications/

[johnme, 2017-01-11 17:50:53]

Left comments on two parts I was unsure about.

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/notification_image_reporter.cc (right):

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/notification_image_reporter.cc:76:
make_scoped_refptr(g_browser_process->safe_browsing_service()),
I feel a little dirty for accessing this global here in order to reach the
SafeBrowsingDatabaseManager which is this class's "uncle". But the ping manager
doesn't seem to have any references to it or the service. Any ideas?

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/ping_manager.cc (right):

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/ping_manager.cc:181: BrowserThread::UI, FROM_HERE,
It's a shame that the PlatformNotificationServiceImpl has to post to the IO
thread to get the ping manager, only for the ping manager to have to post to the
UI thread for the NotificationImageReporter to check the SBER status. Any ideas
for simplifying this?

[Peter Beverloo, 2017-01-11 18:23:57]

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:102:
ping_manager->ReportNotificationImage(profile, origin, image);
nit: since initializing and shutting down the ping manager happens on the IO
thread, where as are, you could avoid the null check by checking whether it's
still enabled. That takes away the "why could this be NULL" question from the
reader.

>>>>
DCHECK_CURRENTLY_ON(BrowserThread::IO);
if (!safe_browsing_service->enabled())
  return;

safe_browsing_service->ping_manager()->ReportNotificationImage(
    profile, origin, image);
>>>>

(or with a DCHECK if you prefer)

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:451:
make_scoped_refptr(g_browser_process->safe_browsing_service()),
I see a bunch of other features doing NULL checks on the
safe_browsing_service(). It appears that you need to call
g_browser_process->SetSafeBrowsingService() when running tests (i.e.
TestingBrowserProcess). Should probably do that too, or ensure that our tests
set one?

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/notification_image_reporter.cc (right):

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/notification_image_reporter.cc:34: const int
kMaxReportsPerDay = 5;
micro nit: int -> size_t to match std::vector::size()

also please have a blank line at the beginning and end of the anonymous
namespace

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/notification_image_reporter.cc:76:
make_scoped_refptr(g_browser_process->safe_browsing_service()),
On 2017/01/11 17:50:53, johnme wrote:
> I feel a little dirty for accessing this global here in order to reach the
> SafeBrowsingDatabaseManager which is this class's "uncle". But the ping
manager
> doesn't seem to have any references to it or the service. Any ideas?

This question is for Nathan, but I'd love to see a DCHECK() if this is OK.
(Otherwise we may try to invoke on a nullptr on the receiving thread, which
leads to weird stack traces.)

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/notification_image_reporter.cc:89: return;
micro nit: {} as this is considered a multi-line statement

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/notification_image_reporter.cc:114: const double
MAX_SIZE = 512;
nit: maybe this should be finch controllable too?

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/notification_image_reporter.cc:126: NOTREACHED();
q: this should only happen for OOM, right?

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/notification_image_reporter.cc:162:
base::Callback<void(const GURL&, int)>());
nit: it'd be cool to have UMA for logging (a) the amount of data transferred,
and (b) the time it took for the transfer to complete. Consider a TODO?

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/notification_image_reporter.h (right):

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/notification_image_reporter.h:8: #include <queue>
#include <memory>

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/notification_image_reporter.h:49: // Report
notification content image to SafeBrowsing CSD server if necessary.
"if necessary" -> would be cool to give a one sentence summary of how that's
decided.

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/notification_image_reporter.h:64: gfx::Size
original_dimensions);
micro nit: we usually pass gfx::Size as a const&

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/ping_manager.h (right):

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/ping_manager.h:114:
scoped_refptr<NotificationImageReporter> notification_image_reporter_;
Higher level question: we could avoid ref counting if we're OK with dropping
events on the floor when the PingManager is being shut down. (I.e. use a
WeakPtr<>.)

That may be safer too, e.g. is the net::URLRequestContext* passed to the
net::ReportSender assumed to remain valid after the PingManager goes away?

WDYT?

https://codereview.chromium.org/2624193004/diff/1/chrome/common/safe_browsing...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/2624193004/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/csd.proto:863: // Response: None (empty body).
Stray line?

[johnme, 2017-01-11 20:05:08]

Addressed peter's review comments.

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:102:
ping_manager->ReportNotificationImage(profile, origin, image);
On 2017/01/11 18:23:56, Peter Beverloo wrote:
> nit: since initializing and shutting down the ping manager happens on the IO
> thread, where as are, you could avoid the null check by checking whether it's
> still enabled. That takes away the "why could this be NULL" question from the
> reader.
> 
> >>>>
> DCHECK_CURRENTLY_ON(BrowserThread::IO);
> if (!safe_browsing_service->enabled())
>   return;
> 
> safe_browsing_service->ping_manager()->ReportNotificationImage(
>     profile, origin, image);
> >>>>
> 
> (or with a DCHECK if you prefer)

Done.

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:451:
make_scoped_refptr(g_browser_process->safe_browsing_service()),
On 2017/01/11 18:23:56, Peter Beverloo wrote:
> I see a bunch of other features doing NULL checks on the
> safe_browsing_service(). It appears that you need to call
> g_browser_process->SetSafeBrowsingService() when running tests (i.e.
> TestingBrowserProcess). Should probably do that too, or ensure that our tests
> set one?

Done.

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/notification_image_reporter.cc (right):

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/notification_image_reporter.cc:34: const int
kMaxReportsPerDay = 5;
On 2017/01/11 18:23:56, Peter Beverloo wrote:
> micro nit: int -> size_t to match std::vector::size()
> 
> also please have a blank line at the beginning and end of the anonymous
> namespace

Done.

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/notification_image_reporter.cc:76:
make_scoped_refptr(g_browser_process->safe_browsing_service()),
On 2017/01/11 18:23:56, Peter Beverloo wrote:
> On 2017/01/11 17:50:53, johnme wrote:
> > I feel a little dirty for accessing this global here in order to reach the
> > SafeBrowsingDatabaseManager which is this class's "uncle". But the ping
> manager
> > doesn't seem to have any references to it or the service. Any ideas?
> 
> This question is for Nathan, but I'd love to see a DCHECK() if this is OK.
> (Otherwise we may try to invoke on a nullptr on the receiving thread, which
> leads to weird stack traces.)

I've added a !safe_browsing_service to the condition in
ReportNotificationImageOnIO below. Note that this is an ordinary parameter not
the "this" being used for the PostTask.

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/notification_image_reporter.cc:89: return;
On 2017/01/11 18:23:56, Peter Beverloo wrote:
> micro nit: {} as this is considered a multi-line statement

Done.

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/notification_image_reporter.cc:114: const double
MAX_SIZE = 512;
On 2017/01/11 18:23:56, Peter Beverloo wrote:
> nit: maybe this should be finch controllable too?

Added TODO.

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/notification_image_reporter.cc:126: NOTREACHED();
On 2017/01/11 18:23:56, Peter Beverloo wrote:
> q: this should only happen for OOM, right?

I guess so; don't know enough about how libpng operated under the hood. Also for
empty input, which I don't think can happen here.

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/notification_image_reporter.cc:162:
base::Callback<void(const GURL&, int)>());
On 2017/01/11 18:23:56, Peter Beverloo wrote:
> nit: it'd be cool to have UMA for logging (a) the amount of data transferred,
> and (b) the time it took for the transfer to complete. Consider a TODO?

Added TODO.

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/notification_image_reporter.h (right):

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/notification_image_reporter.h:8: #include <queue>
On 2017/01/11 18:23:56, Peter Beverloo wrote:
> #include <memory>

Done.

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/notification_image_reporter.h:49: // Report
notification content image to SafeBrowsing CSD server if necessary.
On 2017/01/11 18:23:56, Peter Beverloo wrote:
> "if necessary" -> would be cool to give a one sentence summary of how that's
> decided.

Done ("if the user has opted in, the origin is not whitelisted, and it is
randomly sampled").

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/notification_image_reporter.h:64: gfx::Size
original_dimensions);
On 2017/01/11 18:23:56, Peter Beverloo wrote:
> micro nit: we usually pass gfx::Size as a const&

Done.

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/ping_manager.h (right):

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/ping_manager.h:114:
scoped_refptr<NotificationImageReporter> notification_image_reporter_;
On 2017/01/11 18:23:56, Peter Beverloo wrote:
> Higher level question: we could avoid ref counting if we're OK with dropping
> events on the floor when the PingManager is being shut down. (I.e. use a
> WeakPtr<>.)
> 
> That may be safer too, e.g. is the net::URLRequestContext* passed to the
> net::ReportSender assumed to remain valid after the PingManager goes away?
> 
> WDYT?

I considered this, but it needs to be possible to post to methods on the IO, UI
and blocking pool worker threads. Especially for the latter, there seems no
practical way of satisfying weak pointers' requirement that the weak pointers
are always dereferenced on the same thread.

The lifetime question is relevant though - Nathan, do you know if it's ok that
NotificationImageReporter and its net::ReportSender might outlive the ping
manager due to ref counting?

https://codereview.chromium.org/2624193004/diff/1/chrome/common/safe_browsing...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/2624193004/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/csd.proto:863: // Response: None (empty body).
On 2017/01/11 18:23:56, Peter Beverloo wrote:
> Stray line?

Done (moved to comment above NotificationImageReportRequest)

[Nathan Parker, 2017-01-11 22:34:38]

Overall I think it looks like a great approach.  How about some tests?

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/notification_image_reporter.cc (right):

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/notification_image_reporter.cc:76:
make_scoped_refptr(g_browser_process->safe_browsing_service()),
On 2017/01/11 20:05:07, johnme wrote:
> On 2017/01/11 18:23:56, Peter Beverloo wrote:
> > On 2017/01/11 17:50:53, johnme wrote:
> > > I feel a little dirty for accessing this global here in order to reach the
> > > SafeBrowsingDatabaseManager which is this class's "uncle". But the ping
> > manager
> > > doesn't seem to have any references to it or the service. Any ideas?
> > 
> > This question is for Nathan, but I'd love to see a DCHECK() if this is OK.
> > (Otherwise we may try to invoke on a nullptr on the receiving thread, which
> > leads to weird stack traces.)
> 
> I've added a !safe_browsing_service to the condition in
> ReportNotificationImageOnIO below. Note that this is an ordinary parameter not
> the "this" being used for the PostTask.

This is the common way to get the db_manager, so I think it's reasonable.

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/ping_manager.h (right):

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/ping_manager.h:114:
scoped_refptr<NotificationImageReporter> notification_image_reporter_;
On 2017/01/11 20:05:07, johnme wrote:
> On 2017/01/11 18:23:56, Peter Beverloo wrote:
> > Higher level question: we could avoid ref counting if we're OK with dropping
> > events on the floor when the PingManager is being shut down. (I.e. use a
> > WeakPtr<>.)
> > 
> > That may be safer too, e.g. is the net::URLRequestContext* passed to the
> > net::ReportSender assumed to remain valid after the PingManager goes away?
> > 
> > WDYT?
> 
> I considered this, but it needs to be possible to post to methods on the IO,
UI
> and blocking pool worker threads. Especially for the latter, there seems no
> practical way of satisfying weak pointers' requirement that the weak pointers
> are always dereferenced on the same thread.
> 
> The lifetime question is relevant though - Nathan, do you know if it's ok that
> NotificationImageReporter and its net::ReportSender might outlive the ping
> manager due to ref counting?

It feels like a weak_ptr would be safer, since then we'd just drop the report
rather than access a bad URLRequestContext. I think we don't want report_sender_
to outlive PingManager. The PingManager will destruct the
NotificationImageReporter before the URLRequestContextGetter, so that would be
safe.

https://codereview.chromium.org/2624193004/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/notification_image_reporter.cc (right):

https://codereview.chromium.org/2624193004/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/notification_image_reporter.cc:61:
DCHECK_EQ(origin, origin.GetOrigin());
How about DCHECK(origin.is_valid()) as well

https://codereview.chromium.org/2624193004/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/notification_image_reporter.cc:72: // Sample a
Finch-controlled fraction only.
I'd suggest adding this now, so we have a kill switch. (Anything that initiates
a request to Google can accidentally turn into a DDoS.)

https://codereview.chromium.org/2624193004/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/notification_image_reporter.cc:81: origin, image));
Does the image get copied here?

https://codereview.chromium.org/2624193004/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/notification_image_reporter.h (right):

https://codereview.chromium.org/2624193004/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/notification_image_reporter.h:61: void
DownscaleNotificationImageOnBlockingPool(const GURL& origin,
What's the output of this? (i.e. does it invoke SendReport?) Add that to the
comment.

[Nathan Parker, 2017-01-11 22:46:10]

Ah, I just noticed your comment about dereferencing weakPtr's on different
threads.  Hrm. We need a thread-safe WeakPtr.

Another (not great) solution is to pass the URLRequestContextGetter in so it can
be up-reff'd.

[johnme, 2017-01-12 13:44:32]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 13:44:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 15:07:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 15:07:25]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2017-01-13 00:48:35]

//c/b/notifications lgtm

https://codereview.chromium.org/2624193004/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/notification_image_reporter_unittest.cc
(right):

https://codereview.chromium.org/2624193004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/notification_image_reporter_unittest.cc:31: const
char kReportingUploadUrl[] =
nit: comment on the source of the duplication. (Or consider making this a static
property on the NotificationImageReporter.)

https://codereview.chromium.org/2624193004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/notification_image_reporter_unittest.cc:37:
TestingNotificationImageReporter(
nit: explicit

https://codereview.chromium.org/2624193004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/notification_image_reporter_unittest.cc:51: bool
match_all_ = false;
nit: unused?

[johnme, 2017-01-13 02:38:17]

Addressed Nathan's review comments, except for adding Finch param. Haven't yet
looked at Peter's nits.

Lifetimes changed significantly - NotificationImageReporter is no longer ref
counted, and instead WeakPtrs are used.

> Overall I think it looks like a great approach.  How about some tests?

Yup, I was just waiting for confirmation that the overall approach made sense.
I've added an initial test for the report being sent correctly; I'll add some
more tomorrow (Friday) for negative cases like Scout being disabled.

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/ping_manager.h (right):

https://codereview.chromium.org/2624193004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/ping_manager.h:114:
scoped_refptr<NotificationImageReporter> notification_image_reporter_;
On 2017/01/11 22:34:38, Nathan Parker wrote:
> On 2017/01/11 20:05:07, johnme wrote:
> > On 2017/01/11 18:23:56, Peter Beverloo wrote:
> > > Higher level question: we could avoid ref counting if we're OK with
dropping
> > > events on the floor when the PingManager is being shut down. (I.e. use a
> > > WeakPtr<>.)
> > > 
> > > That may be safer too, e.g. is the net::URLRequestContext* passed to the
> > > net::ReportSender assumed to remain valid after the PingManager goes away?
> > > 
> > > WDYT?
> > 
> > I considered this, but it needs to be possible to post to methods on the IO,
> UI
> > and blocking pool worker threads. Especially for the latter, there seems no
> > practical way of satisfying weak pointers' requirement that the weak
pointers
> > are always dereferenced on the same thread.
> > 
> > The lifetime question is relevant though - Nathan, do you know if it's ok
that
> > NotificationImageReporter and its net::ReportSender might outlive the ping
> > manager due to ref counting?
> 
> It feels like a weak_ptr would be safer, since then we'd just drop the report
> rather than access a bad URLRequestContext. I think we don't want
report_sender_
> to outlive PingManager. The PingManager will destruct the
> NotificationImageReporter before the URLRequestContextGetter, so that would be
> safe.

Switched to WeakPtrs. The trick was to only dereference them on the IO thread,
and make the method that run on other threads static (with a weak_this_on_io
parameter which will later get used to post to the non-static IO thread method).

https://codereview.chromium.org/2624193004/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/notification_image_reporter.cc (right):

https://codereview.chromium.org/2624193004/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/notification_image_reporter.cc:61:
DCHECK_EQ(origin, origin.GetOrigin());
On 2017/01/11 22:34:38, Nathan Parker wrote:
> How about DCHECK(origin.is_valid()) as well

Done.

https://codereview.chromium.org/2624193004/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/notification_image_reporter.cc:72: // Sample a
Finch-controlled fraction only.
On 2017/01/11 22:34:38, Nathan Parker wrote:
> I'd suggest adding this now, so we have a kill switch. (Anything that
initiates
> a request to Google can accidentally turn into a DDoS.)

Makes sense, especially since the server isn't yet ready. I haven't gotten
around to this yet in the current iteration of the patch.

https://codereview.chromium.org/2624193004/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/notification_image_reporter.cc:81: origin, image));
On 2017/01/11 22:34:38, Nathan Parker wrote:
> Does the image get copied here?

No, SkBitmaps have an internal thread safe refcounted storage for the image.

https://codereview.chromium.org/2624193004/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/notification_image_reporter.h (right):

https://codereview.chromium.org/2624193004/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/notification_image_reporter.h:61: void
DownscaleNotificationImageOnBlockingPool(const GURL& origin,
On 2017/01/11 22:34:38, Nathan Parker wrote:
> What's the output of this? (i.e. does it invoke SendReport?) Add that to the
> comment.

Done.

[johnme, 2017-01-13 17:12:26]

Addressed Peter's nits - thanks

https://codereview.chromium.org/2624193004/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/notification_image_reporter_unittest.cc
(right):

https://codereview.chromium.org/2624193004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/notification_image_reporter_unittest.cc:31: const
char kReportingUploadUrl[] =
On 2017/01/13 00:48:35, Peter Beverloo (OOO- Jan 19th) wrote:
> nit: comment on the source of the duplication. (Or consider making this a
static
> property on the NotificationImageReporter.)

Done (static).

https://codereview.chromium.org/2624193004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/notification_image_reporter_unittest.cc:37:
TestingNotificationImageReporter(
On 2017/01/13 00:48:35, Peter Beverloo (OOO- Jan 19th) wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2624193004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/notification_image_reporter_unittest.cc:51: bool
match_all_ = false;
On 2017/01/13 00:48:35, Peter Beverloo (OOO- Jan 19th) wrote:
> nit: unused?

Done.

[Nathan Parker, 2017-01-18 21:38:04]

This CL is obsoleted by harkness's copy below, while johnme is ooo

https://codereview.chromium.org/2637153002/

[Nathan Parker, 2017-01-18 21:38:36]

Description was changed from

==========
Submit a sample of notification images to Safe Browsing

These will be scanned for social engineering behavior.

Only uploads if all of the following are true:
- User has opted in to SBER_LEVEL_SCOUT.
- Origin is not on CSD phishing whitelist.
- Device has sent < 5 reports in last 24 hours (see persistence TODO).
- Do a dice roll, and keep only 20% of notifications (see Finch TODO).

The notification image bitmap is downscaled to <= 512x512, encoded as a
PNG, then sent to the CSD server as a NotificationImageReportRequest
protobuf from chrome/common/safe_browsing/csd.proto.

BUG=678443
==========

to

==========
Submit a sample of notification images to Safe Browsing

These will be scanned for social engineering behavior.

Only uploads if all of the following are true:
- User has opted in to SBER_LEVEL_SCOUT.
- Origin is not on CSD phishing whitelist.
- Device has sent < 5 reports in last 24 hours (see persistence TODO).
- Do a dice roll, and keep only 20% of notifications (see Finch TODO).

The notification image bitmap is downscaled to <= 512x512, encoded as a
PNG, then sent to the CSD server as a NotificationImageReportRequest
protobuf from chrome/common/safe_browsing/csd.proto.

BUG=678443
==========

[johnme, 2017-01-31 16:27:21]

Superceded by https://codereview.chromium.org/2637153002