Support for process-wide incidents in the safe browsing incident reporting service.

Support for process-wide incidents in the safe browsing incident reporting service.

BUG=399428
R=robertshield@chromium.org

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

[robertshield, 2014-08-04 01:35:12]

Awesome, some drive-by musings follow

https://codereview.chromium.org/441453002/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/delayed_callback_runner.h (right):

https://codereview.chromium.org/441453002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/delayed_callback_runner.h:22: // runner is doing it
job has no effect.
its

https://codereview.chromium.org/441453002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/delayed_callback_runner.h:23: class
DelayedCallbackRunner {
May be worth mentioning in the class comment that this class is not thread-safe/

https://codereview.chromium.org/441453002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/delayed_callback_runner.h:32: void
RegisterCallback(const base::Closure& callback);
Is it worth mentioning that a copy of the Closure is held for the lifetime of
the DelayedCallbackRunner instance?

https://codereview.chromium.org/441453002/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/incident_reporting_service.h (right):

https://codereview.chromium.org/441453002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.h:139: // Returns an
initialized profile that participates in safe browsing.
Mention that it returns the most opted-in profile if one exists?

https://codereview.chromium.org/441453002/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
(right):

https://codereview.chromium.org/441453002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:684: // with
a profile.
Is this true only if no other opted-in profile exists? If so, consider adding
the same test except that an opted-in profile is created first. In that case (if
I understand correctly) the incident should be associated to that opted-in
profile and an upload should occur?

[grt (UTC plus 2), 2014-08-04 14:15:34]

Many thanks for the early review. This CL is a work in progress and isn't yet
ready for final review. I've incorporated changes for all of your comments so
far except for the tests (more tests to come). Feel free to take another look or
wait until I send out a request for review.

https://codereview.chromium.org/441453002/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/delayed_callback_runner.h (right):

https://codereview.chromium.org/441453002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/delayed_callback_runner.h:22: // runner is doing it
job has no effect.
On 2014/08/04 01:35:12, robertshield wrote:
> its

Done.

https://codereview.chromium.org/441453002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/delayed_callback_runner.h:23: class
DelayedCallbackRunner {
On 2014/08/04 01:35:12, robertshield wrote:
> May be worth mentioning in the class comment that this class is not
thread-safe/

Added enforcement via ThreadChecker.

https://codereview.chromium.org/441453002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/delayed_callback_runner.h:32: void
RegisterCallback(const base::Closure& callback);
On 2014/08/04 01:35:12, robertshield wrote:
> Is it worth mentioning that a copy of the Closure is held for the lifetime of
> the DelayedCallbackRunner instance?

it should be held only until it is run. comment and code updated.

https://codereview.chromium.org/441453002/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/incident_reporting_service.h (right):

https://codereview.chromium.org/441453002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.h:139: // Returns an
initialized profile that participates in safe browsing.
On 2014/08/04 01:35:12, robertshield wrote:
> Mention that it returns the most opted-in profile if one exists?

Done.

https://codereview.chromium.org/441453002/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
(right):

https://codereview.chromium.org/441453002/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:684: // with
a profile.
On 2014/08/04 01:35:12, robertshield wrote:
> Is this true only if no other opted-in profile exists? If so, consider adding
> the same test except that an opted-in profile is created first. In that case
(if
> I understand correctly) the incident should be associated to that opted-in
> profile and an upload should occur?

Yes.

[grt (UTC plus 2), 2014-08-04 14:16:29]

https://codereview.chromium.org/441453002/diff/50001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/delayed_callback_runner.cc (right):

https://codereview.chromium.org/441453002/diff/50001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/delayed_callback_runner.cc:41:
callbacks_.push_back(base::Bind(&RunCallbackOnOriginThread,
oops, this is totally wrong.

[grt (UTC plus 2), 2014-08-04 14:32:49]

totally wrong thread return is now totally right (i think). will be proved in
unit tests to come.

[grt (UTC plus 2), 2014-08-05 15:33:55]

Sending out for early review. I will be adding tests for the new feature to this
CL in the coming day or so. There is one TODO which I plan to address in a
separate CL since I think it covers a fringe case and we'll get plenty of
benefit from this without it.

Robert: please review the overall approach to confirm that it meets your needs
and appears to do as we discussed.

Matt: please review the change overall.

Thanks.

[robertshield, 2014-08-05 21:26:28]

Looks awesome to me, some comments

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

https://codereview.chromium.org/441453002/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:76: const int64
kDefaultCallbackMs = 1000 * 20;
how about kDefaultCallbackIntervalMs ?

https://codereview.chromium.org/441453002/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:300: // the main
thread via AddIncidentOnOriginThread.
s/main/current ?

https://codereview.chromium.org/441453002/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:342: // participates
in safe browsing.
Re-mention here how Start() is idempotent for repeated calls?

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

https://codereview.chromium.org/441453002/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.h:60: // profile that
participates in safe browsing when one becomes available.
s/one/such a profile/ to resolve mild pronoun ambiguity with 'incidents'.

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

https://codereview.chromium.org/441453002/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:685:
TEST_F(IncidentReportingServiceTest, ProcessWideNoProfileNoUpload) {
I vaguely recall suggesting an extra test here in a previous comment. Would that
test be helpful?

[mattm, 2014-08-06 02:10:17]

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

https://codereview.chromium.org/441453002/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:432: if
(prefs->GetBoolean(prefs::kSafeBrowsingExtendedReportingEnabled)) {
Need to check prefs::kSafeBrowsingEnabled too (extended reporting flag could be
set while safe browsing is actually disabled)

https://codereview.chromium.org/441453002/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:491: // Createa a new
report if needed.
Createa

https://codereview.chromium.org/441453002/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:696: Profile*
analyses_profile = FindEligibleProfile();
is "analyses" intentional here? (vs analysis)

https://codereview.chromium.org/441453002/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:714:
profiles_.erase(NULL);
Does this mean no more process-wide incidents can be added after this?

[grt (UTC plus 2), 2014-08-06 02:37:31]

Responding to comments. More unit tests still to come.

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

https://codereview.chromium.org/441453002/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:76: const int64
kDefaultCallbackMs = 1000 * 20;
On 2014/08/05 21:26:28, robertshield wrote:
> how about kDefaultCallbackIntervalMs ?

Done.

https://codereview.chromium.org/441453002/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:300: // the main
thread via AddIncidentOnOriginThread.
On 2014/08/05 21:26:28, robertshield wrote:
> s/main/current ?

Done.

https://codereview.chromium.org/441453002/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:342: // participates
in safe browsing.
On 2014/08/05 21:26:28, robertshield wrote:
> Re-mention here how Start() is idempotent for repeated calls?

Done.

https://codereview.chromium.org/441453002/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:432: if
(prefs->GetBoolean(prefs::kSafeBrowsingExtendedReportingEnabled)) {
On 2014/08/06 02:10:17, mattm wrote:
> Need to check prefs::kSafeBrowsingEnabled too (extended reporting flag could
be
> set while safe browsing is actually disabled)

Done.

https://codereview.chromium.org/441453002/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:491: // Createa a new
report if needed.
On 2014/08/06 02:10:17, mattm wrote:
> Createa

Done.

https://codereview.chromium.org/441453002/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:696: Profile*
analyses_profile = FindEligibleProfile();
On 2014/08/06 02:10:17, mattm wrote:
> is "analyses" intentional here? (vs analysis)

Yeah, it's the profile used for the plural analyses. Kinda confusing, huh. I've
tweaked the names and comments. What do you think?

https://codereview.chromium.org/441453002/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:714:
profiles_.erase(NULL);
On 2014/08/06 02:10:17, mattm wrote:
> Does this mean no more process-wide incidents can be added after this?

Nice catch. Any added after this should go into a new report. The incidents in
the context should be cleared, but the context must live.

[grt (UTC plus 2), 2014-08-06 17:51:15]

I've rounded out the unit tests. Please perform final reviews. Thanks.

[robertshield, 2014-08-06 18:27:08]

lgtm

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

https://codereview.chromium.org/441453002/diff/250001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:775: //
participating in safe browsing is added.
perhaps add "but not before" to this comment to differentiate between the
comment on line 797

[mattm, 2014-08-06 22:36:13]

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

https://codereview.chromium.org/441453002/diff/250001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:799:
TEST_F(IncidentReportingServiceTest, AnalysisWhenReigsteredWithProfile) {
Reigstered

https://codereview.chromium.org/441453002/diff/250001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:862: 
The tests which add two incidents only test that the 2nd same incident gets
pruned, how bout a test in which the 2nd incident is different and that it gets
uploaded too? (eg, a test for the bug I pointed out earlier.)

[grt (UTC plus 2), 2014-08-07 01:46:14]

Thanks, PTAL. Feel at liberty to hit the CQ box if it looks good to you.

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

https://codereview.chromium.org/441453002/diff/250001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:775: //
participating in safe browsing is added.
On 2014/08/06 18:27:07, robertshield wrote:
> perhaps add "but not before" to this comment to differentiate between the
> comment on line 797

Done.

https://codereview.chromium.org/441453002/diff/250001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:799:
TEST_F(IncidentReportingServiceTest, AnalysisWhenReigsteredWithProfile) {
On 2014/08/06 22:36:12, mattm wrote:
> Reigstered

Done.

https://codereview.chromium.org/441453002/diff/250001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:862: 
On 2014/08/06 22:36:12, mattm wrote:
> The tests which add two incidents only test that the 2nd same incident gets
> pruned, how bout a test in which the 2nd incident is different and that it
gets
> uploaded too? (eg, a test for the bug I pointed out earlier.)

Sure. I'd thought that the prune test would cover the case you found since the
second call to AddIncident with a NULL profile would have hit the
DCHECK(context) in that function. As it happens, the same callback needs to be
used for both incidents to hit that exact path. I've added ProcessWideTwoUploads
to cover the case you mention.

[mattm, 2014-08-07 01:50:49]

lgtm

[commit-bot: I haz the power, 2014-08-07 01:53:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/grt@chromium.org/441453002/310001

[commit-bot: I haz the power, 2014-08-07 11:05:37]

Change committed as 288028