Safe browsing incident reporting service improvements.

Safe browsing incident reporting service improvements.

Honour the safe browsing opt out and report on incidents that arrive
before profile initialization is complete.

BUG=383039, 383365
R=mattm@chromium.org, noms@chromium.org
TBR=asvitkine@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278358

[grt (UTC plus 2), 2014-06-18 17:30:22]

please take a look. review assignments:
mattm: safe_browsing
asvitkine: metrics
noms: profiles

[mattm, 2014-06-18 21:07:18]

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service.cc (right):

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:252: // The profile
is unknown if it has already been destroyed.
Probably not safe to allow this to be called after the profile has been
destroyed (what if one profile gets destroyed and another gets created at the
same address?)

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:418:
prefs->GetBoolean(prefs::kSafeBrowsingDownloadFeedbackEnabled)) {
kSafeBrowsingExtendedReportingEnabled now

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:419:
process->set_extended_consent(true);
is it okay that this isn't sent on a per-profile/incident basis?

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service.h (right):

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.h:72: // service on
behalf of |profile|.
comment should mention expectations regarding using the callback and the
lifetime of profile.

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.h:76: // for validation
failures in |profile|.
same

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
(right):

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:90: //
service notices that profile creation has completed.
Is it guaranteed that this observer will be called before the one that the
IncidentReportingService registers?

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:154:
task_runner_->RunUntilIdle();
I didn't notice before, but how does this work? Shouldn't the reporting service
be waiting one minute before it uploads the report?

[grt (UTC plus 2), 2014-06-18 22:47:59]

Thanks. Please see clarifying questions below. I'll send up another patch set in
a little bit.

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service.cc (right):

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:252: // The profile
is unknown if it has already been destroyed.
On 2014/06/18 21:07:18, mattm wrote:
> Probably not safe to allow this to be called after the profile has been
> destroyed (what if one profile gets destroyed and another gets created at the
> same address?)

The only case where GetProfileContext would return NULL is if it were called
with a profile after NOTIFICATION_PROFILE_DESTROYED had been sent for the
Profile. This should never happen. Is the Chromium-y thing to do to let the
process crash when the NULL pointer is dereferenced so we see the impossible
appear in crash reports (and therefore remove lines 252-254)?

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:418:
prefs->GetBoolean(prefs::kSafeBrowsingDownloadFeedbackEnabled)) {
On 2014/06/18 21:07:17, mattm wrote:
> kSafeBrowsingExtendedReportingEnabled now

thanks for the pointer.

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:419:
process->set_extended_consent(true);
On 2014/06/18 21:07:18, mattm wrote:
> is it okay that this isn't sent on a per-profile/incident basis?

it is, thanks for checking.

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
(right):

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:90: //
service notices that profile creation has completed.
On 2014/06/18 21:07:18, mattm wrote:
> Is it guaranteed that this observer will be called before the one that the
> IncidentReportingService registers?

In the current implementation, yes. It's not documented, but ObserverList
appends observers, so they are called in the order they're added. I looked for a
way to hook into profile initialization, but the obvious thing (passing a
Delegate to TestingProfile) has a comment making it look like there's a movement
to do away with it, so I chose this route. Are you okay with leaving this in
place until it becomes an issue?

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:154:
task_runner_->RunUntilIdle();
On 2014/06/18 21:07:18, mattm wrote:
> I didn't notice before, but how does this work? Shouldn't the reporting
service
> be waiting one minute before it uploads the report?

I had the same "huh?" moment. TestSimpleTaskRunner ignores the delay.

[mattm, 2014-06-18 23:05:38]

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service.cc (right):

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:252: // The profile
is unknown if it has already been destroyed.
On 2014/06/18 22:47:58, grt wrote:
> On 2014/06/18 21:07:18, mattm wrote:
> > Probably not safe to allow this to be called after the profile has been
> > destroyed (what if one profile gets destroyed and another gets created at
the
> > same address?)
> 
> The only case where GetProfileContext would return NULL is if it were called
> with a profile after NOTIFICATION_PROFILE_DESTROYED had been sent for the
> Profile. This should never happen. Is the Chromium-y thing to do to let the
> process crash when the NULL pointer is dereferenced so we see the impossible
> appear in crash reports (and therefore remove lines 252-254)?

yeah, that sounds good. You could replace it with a DCHECK to make it explicit.

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
(right):

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:90: //
service notices that profile creation has completed.
On 2014/06/18 22:47:58, grt wrote:
> On 2014/06/18 21:07:18, mattm wrote:
> > Is it guaranteed that this observer will be called before the one that the
> > IncidentReportingService registers?
> 
> In the current implementation, yes. It's not documented, but ObserverList
> appends observers, so they are called in the order they're added. I looked for
a
> way to hook into profile initialization, but the obvious thing (passing a
> Delegate to TestingProfile) has a comment making it look like there's a
movement
> to do away with it, so I chose this route. Are you okay with leaving this in
> place until it becomes an issue?

Realistically it's probably fine.. main concern would be if it changes, and the
test continues to pass but just not actually testing what it meant to test
anymore. Is it easy to make the test fail if things happen in the wrong order?

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:154:
task_runner_->RunUntilIdle();
On 2014/06/18 22:47:58, grt wrote:
> On 2014/06/18 21:07:18, mattm wrote:
> > I didn't notice before, but how does this work? Shouldn't the reporting
> service
> > be waiting one minute before it uploads the report?
> 
> I had the same "huh?" moment. TestSimpleTaskRunner ignores the delay.

Ah ha, thanks.

[grt (UTC plus 2), 2014-06-19 02:24:39]

Thanks for the comments. Patch set 7 is only a sync. Patch set 8 contains the
meat.

asvitkine, noms: ping

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service.cc (right):

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:252: // The profile
is unknown if it has already been destroyed.
On 2014/06/18 23:05:38, mattm wrote:
> On 2014/06/18 22:47:58, grt wrote:
> > On 2014/06/18 21:07:18, mattm wrote:
> > > Probably not safe to allow this to be called after the profile has been
> > > destroyed (what if one profile gets destroyed and another gets created at
> the
> > > same address?)
> > 
> > The only case where GetProfileContext would return NULL is if it were called
> > with a profile after NOTIFICATION_PROFILE_DESTROYED had been sent for the
> > Profile. This should never happen. Is the Chromium-y thing to do to let the
> > process crash when the NULL pointer is dereferenced so we see the impossible
> > appear in crash reports (and therefore remove lines 252-254)?
> 
> yeah, that sounds good. You could replace it with a DCHECK to make it
explicit.

Done.

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:418:
prefs->GetBoolean(prefs::kSafeBrowsingDownloadFeedbackEnabled)) {
On 2014/06/18 21:07:17, mattm wrote:
> kSafeBrowsingExtendedReportingEnabled now

Done.

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service.h (right):

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.h:72: // service on
behalf of |profile|.
On 2014/06/18 21:07:18, mattm wrote:
> comment should mention expectations regarding using the callback and the
> lifetime of profile.

Done.

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.h:76: // for validation
failures in |profile|.
On 2014/06/18 21:07:18, mattm wrote:
> same

Done.

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
(right):

https://codereview.chromium.org/341563003/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:90: //
service notices that profile creation has completed.
On 2014/06/18 23:05:38, mattm wrote:
> On 2014/06/18 22:47:58, grt wrote:
> > On 2014/06/18 21:07:18, mattm wrote:
> > > Is it guaranteed that this observer will be called before the one that the
> > > IncidentReportingService registers?
> > 
> > In the current implementation, yes. It's not documented, but ObserverList
> > appends observers, so they are called in the order they're added. I looked
for
> a
> > way to hook into profile initialization, but the obvious thing (passing a
> > Delegate to TestingProfile) has a comment making it look like there's a
> movement
> > to do away with it, so I chose this route. Are you okay with leaving this in
> > place until it becomes an issue?
> 
> Realistically it's probably fine.. main concern would be if it changes, and
the
> test continues to pass but just not actually testing what it meant to test
> anymore. Is it easy to make the test fail if things happen in the wrong order?

I've removed the notification service magic in favor of making OnProfileCreated
virtual in IncidentReportingService so that the test can override it and inject
an incident before calling the base implementation. This is less fragile at the
expense of coupling the test to the implementation a bit more (but this is
probably a better tradeoff anyway since it was previously coupled to an
implementation detail of ObserverList). What do you think?

[mattm, 2014-06-19 02:47:36]

lgtm

[noms (inactive), 2014-06-19 13:32:08]

profiles lgtm

[grt (UTC plus 2), 2014-06-19 13:56:32]

Committed patchset #9 manually as r278358 (presubmit successful).

[Alexei Svitkine (slow), 2014-06-19 14:25:03]

lgtm % nit, I guess now that it's committed the nit isn't super important

In the future, please don't commit changes to histograms without an owner
review. tools/metrics/OWNERS has "set noparent" for a reason.

https://codereview.chromium.org/341563003/diff/240001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service.cc (right):

https://codereview.chromium.org/341563003/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:50: static const char
kAcceptedMetric[] = "SBIRS.Incident";
Nit: It's fine to inline these into the macros, rather having static
function-level variables for them.