Include the latest binary download info in safe browsing incident reports.

Include the latest binary download info in safe browsing incident reports.

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

[grt (UTC plus 2), 2014-06-26 17:28:53]

Hi Matt,

Here's the final big CL for the incident reporting service. Please take a look.
Thanks.

[mattm, 2014-06-27 21:50:32]

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

https://codereview.chromium.org/330653006/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:508:
EXPECT_TRUE(HasCreatedDownloadFinder());
should these expect that the downloadfinder was destroyed also?

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

https://codereview.chromium.org/330653006/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/last_download_finder.cc:48: // Returns a pointer to
the most interesting copleted download in |downloads|.
completed

https://codereview.chromium.org/330653006/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/last_download_finder.cc:124:
chrome::NOTIFICATION_PROFILE_CREATED,
Is there a reason this is CREATED instead of ADDED?

https://codereview.chromium.org/330653006/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/last_download_finder.cc:200: if (it ==
profiles_.end())
should this be a DCHECK?

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

https://codereview.chromium.org/330653006/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/last_download_finder_unittest.cc:60: }
hm. Why is this necessary instead of using the one TestingProfile creates?

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

https://codereview.chromium.org/330653006/diff/280001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:69: struct
IncidentReportingService::ProfileContext {
Since the IncidentReportingService already tracks profile addition/deletion, I
wonder if it would be cleaner to make LastDownloadFinder only operate on a
single Profile, and create one instance of it per-profile.

[grt (UTC plus 2), 2014-06-30 16:53:38]

Thanks, Matt. Please take another look.

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

https://codereview.chromium.org/330653006/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:508:
EXPECT_TRUE(HasCreatedDownloadFinder());
On 2014/06/27 21:50:32, mattm wrote:
> should these expect that the downloadfinder was destroyed also?

Sure, done.

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

https://codereview.chromium.org/330653006/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/last_download_finder.cc:48: // Returns a pointer to
the most interesting copleted download in |downloads|.
On 2014/06/27 21:50:32, mattm wrote:
> completed

Done.

https://codereview.chromium.org/330653006/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/last_download_finder.cc:124:
chrome::NOTIFICATION_PROFILE_CREATED,
On 2014/06/27 21:50:32, mattm wrote:
> Is there a reason this is CREATED instead of ADDED?

Hmm. I think it should be ADDED, actually, since the intent is to pick up
profiles added to the ProfileManager after the instance is created. Thanks for
catching this.

https://codereview.chromium.org/330653006/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/last_download_finder.cc:200: if (it ==
profiles_.end())
On 2014/06/27 21:50:32, mattm wrote:
> should this be a DCHECK?

No. Comment added.

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

https://codereview.chromium.org/330653006/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/last_download_finder_unittest.cc:60: }
On 2014/06/27 21:50:32, mattm wrote:
> hm. Why is this necessary instead of using the one TestingProfile creates?

The HistoryService created by TestingProfile cannot be obtained until some test
code calls TestingProfile::CreateHistoryService. This means that neither test
nor product code can obtain a profile's HistoryService during profile
initialization (when PROFILE_CREATED and/or PROFILE_ADDED notifications are
sent). LastDownloadFinder::SearchInProfile, which is called when the finder is
given a profile to look at (either at construction or when a new profile is
created), needs to be able to reliably get the HistoryService. This test sets up
a BCKSFactory when the profile is created so that any code from anywhere can
safely obtain the HistoryService. My guess is that TestingProfile has
CreateHistoryService because what this test does was not even possible (by
accident, I believe) until I landed r279691. I'm happy to try to remove
CreateHistoryService and move this test's technique for creating a
HistoryService into TestingProfile in a separate change. This isn't a trivial,
however, since this will result in tests creating a HistoryService when
previously they didn't (because
HistoryServiceFactory::ServiceIsNULLWhileTesting returns true).

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

https://codereview.chromium.org/330653006/diff/280001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:69: struct
IncidentReportingService::ProfileContext {
On 2014/06/27 21:50:32, mattm wrote:
> Since the IncidentReportingService already tracks profile addition/deletion, I
> wonder if it would be cleaner to make LastDownloadFinder only operate on a
> single Profile, and create one instance of it per-profile.

I'll explore this to see if it simplifies things. In the meantime, please
consider this change as-is for now since I'd like to get this feature into M37.
Thanks.

[grt (UTC plus 2), 2014-06-30 19:38:37]

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

https://codereview.chromium.org/330653006/diff/280001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:69: struct
IncidentReportingService::ProfileContext {
On 2014/06/30 16:53:38, grt wrote:
> On 2014/06/27 21:50:32, mattm wrote:
> > Since the IncidentReportingService already tracks profile addition/deletion,
I
> > wonder if it would be cleaner to make LastDownloadFinder only operate on a
> > single Profile, and create one instance of it per-profile.
> 
> I'll explore this to see if it simplifies things. In the meantime, please
> consider this change as-is for now since I'd like to get this feature into
M37.
> Thanks.

I see pros and cons. I'm inclined to keep things as they are, but I'm open to
being convinced otherwise.

- Making the finder profile-specific would remove the dependence on
ProfileManager and the finder's need to observe profile lifecycle notifications.
This is a nice simplification.
- Profile-specific finder would mean more bookkeeping in the incident reporting
service. While the ProfileContext could hold a profile's finder, the service's
WaitingForMostRecentDownload would get more complicated since it would need to
know if any context's finder is outstanding.
- The finder's function that decides if one download is more interesting than
another (IsMoreInterestingThan) would have to be used by each finder instance to
pick the most interesting for that profile and also by the incident reporting
service to pick the most interesting among all downloads. I can think of ways of
sharing this, but I think it's nice that the finder is self-contained at the
moment.
- I'm trying to work out a way for each finder to not need a weak pointer
factory, but so far I'm coming up blank. Without one, I think there's a race if
a profile is destroyed before its finder's call to QueryDownloads returns and a
new one is created at the same memory location. Maybe this can never happen, but
it seems fragile.

I've played around with the code a bit, and so far it seems more complicated to
make the finder per-profile. Let me know what you think.

[mattm, 2014-06-30 21:52:15]

lgtm with one suggestion.

I'm okay with not doing the LastDownloadFinder change if it ends up being a
wash.
 
> I see pros and cons. I'm inclined to keep things as they are, but I'm open to
> being convinced otherwise.
> 
> - Making the finder profile-specific would remove the dependence on
> ProfileManager and the finder's need to observe profile lifecycle
notifications.
> This is a nice simplification.
> - Profile-specific finder would mean more bookkeeping in the incident
reporting
> service. While the ProfileContext could hold a profile's finder, the service's
> WaitingForMostRecentDownload would get more complicated since it would need to
> know if any context's finder is outstanding.

It already has the loop checking ProfileContext->added, could probably just
change it to also check if the finder still exists:
if (!scan->second->added || scan->second->last_download_finder_)
  return true;

> - The finder's function that decides if one download is more interesting than
> another (IsMoreInterestingThan) would have to be used by each finder instance
to
> pick the most interesting for that profile and also by the incident reporting
> service to pick the most interesting among all downloads. I can think of ways
of
> sharing this, but I think it's nice that the finder is self-contained at the
> moment.

This seems like the main thing that gets more complicated.


> - I'm trying to work out a way for each finder to not need a weak pointer
> factory, but so far I'm coming up blank. Without one, I think there's a race
if
> a profile is destroyed before its finder's call to QueryDownloads returns and
a
> new one is created at the same memory location. Maybe this can never happen,
but
> it seems fragile.

I don't think there's a problem with each finder needing it's own weak pointer
factory.

> 
> I've played around with the code a bit, and so far it seems more complicated
to
> make the finder per-profile. Let me know what you think.

https://codereview.chromium.org/330653006/diff/300001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/last_download_finder.cc (right):

https://codereview.chromium.org/330653006/diff/300001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/last_download_finder.cc:203: if (it ==
profiles_.end())
Maybe it would be cleaner to do this check in AbandonSearchInProfile, and have
RemoveProfileAndReportIfDone always expect a valid iterator?
Just seems a little weird to take an iterator as an argument and allow it to not
be a valid one.

[grt (UTC plus 2), 2014-06-30 22:58:05]

Thanks.

https://codereview.chromium.org/330653006/diff/300001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/last_download_finder.cc (right):

https://codereview.chromium.org/330653006/diff/300001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/last_download_finder.cc:203: if (it ==
profiles_.end())
On 2014/06/30 21:52:15, mattm wrote:
> Maybe it would be cleaner to do this check in AbandonSearchInProfile, and have
> RemoveProfileAndReportIfDone always expect a valid iterator?
> Just seems a little weird to take an iterator as an argument and allow it to
not
> be a valid one.

Done.

[grt (UTC plus 2), 2014-06-30 22:59:12]

+mpearson for histograms OWNERS review.

[Mark P, 2014-06-30 23:11:06]

https://codereview.chromium.org/330653006/diff/340001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/330653006/diff/340001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:26024: +    The elapsed time to collect
the most recent binary download for a safe
This sounds awkward.  What does "collect" mean in this context?

Also, what does most recent mean in this context?  Do you not report this on
some downloads?

FWIW, the histogram name implies the description should simply be

The elapsed time to download a safe browsing incident report.

https://codereview.chromium.org/330653006/diff/340001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:47217: +  <int value="6"
label="NoDownload">No download found to report</int>
This doesn't seem consistent with the description, which implies that an upload
took place.

Please update the description if you think this is still the right way to report
this information.

[grt (UTC plus 2), 2014-07-01 14:41:34]

I've updated the descriptions for the new histogram and for all values of the
UploadResult enum. Please take another look. Thanks.

[Mark P, 2014-07-01 17:22:08]

Some minor nits left, I'll lgtm and trust you to fix them well.

-mark

https://codereview.chromium.org/330653006/diff/360001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/330653006/diff/360001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:26021: +<histogram
name="SBIRS.DownloadCollectionTime" units="milliseconds">
The revised summary is definitely clearer.  Perhaps we can revise the histogram
name too?  I'll suggest SBIRS.FindDownloadedBinariesTime,
SBIRS.CollectDownloadsTime, or SBIRS.ScanDownloadsTime as some examples.  In
short, it'd be nice to have a verb in there, and I have a slight preference for
plurality (downloads versus download, because that's more accurate) and accuracy
(specify binaries, because that's more accurate too).

https://codereview.chromium.org/330653006/diff/360001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:47226: +    A report was not uploaded
because no binary download was found to report
On 2014/06/30 23:11:06, Mark P wrote:
> This doesn't seem consistent with the description, which implies that an
upload
> took place.
> 
> Please update the description if you think this is still the right way to
report
> this information.

The new detailed labels look good.  However, I still think you need to update
the the description.
You have
The result of a report upload ..
It could simply be
The result of an attempted report upload ...
or
The result of a planned report upload ...

Maybe while you're at it, you can change the name of the enum to
ReportProcessingResult as you suggest.
I can understand not wanting to change the name of the histogram; I won't
require you to do that.

[grt (UTC plus 2), 2014-07-01 17:49:06]

Thanks, Mark. I've deviated slightly from your suggestion on the name of the new
histogram, so I'll wait for your ack before committing. Cheers.

https://codereview.chromium.org/330653006/diff/360001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/330653006/diff/360001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:26021: +<histogram
name="SBIRS.DownloadCollectionTime" units="milliseconds">
On 2014/07/01 17:22:08, Mark P wrote:
> The revised summary is definitely clearer.  Perhaps we can revise the
histogram
> name too?  I'll suggest SBIRS.FindDownloadedBinariesTime,
> SBIRS.CollectDownloadsTime, or SBIRS.ScanDownloadsTime as some examples.  In
> short, it'd be nice to have a verb in there, and I have a slight preference
for
> plurality (downloads versus download, because that's more accurate) and
accuracy
> (specify binaries, because that's more accurate too).

Done. I've gone with FindDownloadedBinaryTime since the operation (finding a
binary in profiles' history) never results in more than one binary being found
(zero found is a possibility).

https://codereview.chromium.org/330653006/diff/360001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:47226: +    A report was not uploaded
because no binary download was found to report
On 2014/07/01 17:22:08, Mark P wrote:
> On 2014/06/30 23:11:06, Mark P wrote:
> > This doesn't seem consistent with the description, which implies that an
> upload
> > took place.
> > 
> > Please update the description if you think this is still the right way to
> report
> > this information.
> 
> The new detailed labels look good.  However, I still think you need to update
> the the description.
> You have
> The result of a report upload ..
> It could simply be
> The result of an attempted report upload ...
> or
> The result of a planned report upload ...
> 
> Maybe while you're at it, you can change the name of the enum to
> ReportProcessingResult as you suggest.
> I can understand not wanting to change the name of the histogram; I won't
> require you to do that.

Done.

[Mark P, 2014-07-01 17:50:50]

still lgtm

[grt (UTC plus 2), 2014-07-01 17:54:49]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2014-07-01 17:55:49]

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

[commit-bot: I haz the power, 2014-07-01 19:56:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[commit-bot: I haz the power, 2014-07-01 20:01:31]

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

[commit-bot: I haz the power, 2014-07-01 20:01:32]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[grt (UTC plus 2), 2014-07-01 20:11:43]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2014-07-01 20:12:56]

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

[commit-bot: I haz the power, 2014-07-01 20:36:08]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[commit-bot: I haz the power, 2014-07-01 20:50:17]

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

[commit-bot: I haz the power, 2014-07-01 20:50:19]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[grt (UTC plus 2), 2014-07-02 17:48:24]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2014-07-02 17:49:30]

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

[grt (UTC plus 2), 2014-07-02 18:04:50]

+brettw for BUILD.gn OWNERS review.

[brettw, 2014-07-02 19:12:25]

.gn LGTM

[Lei Zhang, 2014-07-02 20:00:11]

lgtm++

[commit-bot: I haz the power, 2014-07-02 20:45:07]

Change committed as 281089