Add sampling of unknown filetypes in download protection.

Add sampling of unknown filetypes in Download Protection.

BUG=619743
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/373f7c4c9722ebc0a718be3387d6f6c9b95d8c1f
Cr-Commit-Position: refs/heads/master@{#401047}

[Nathan Parker, 2016-06-16 22:45:40]

Description was changed from

==========
Add sampling of unknown filetypes in Download Protection.

BUG=619743
==========

to

==========
Add sampling of unknown filetypes in Download Protection.

BUG=619743
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Nathan Parker, 2016-06-16 23:17:45]

nparker@chromium.org changed reviewers:
+ asanka@chromium.org

[Nathan Parker, 2016-06-16 23:17:45]

For reference, a normal ping looks like this:
{
  url  : "http://testsafebrowsing.appspot.com/s/content.exe"
  digests: {
    sha256:
"\x0B\xA8p\x9C\xFDJ\xA9\xCA\xF4\x12\x87)]\x9E\x1B\x8Ej\xDAN\x01\x11\xBF\xAE\x9F\xD4\x1F\x08\xEE\xBBWQ~"
  }
  length       : 276
  resources: {
    url      : "http://testsafebrowsing.appspot.com/s/content.exe"
    type     : DOWNLOAD_URL
    remote_ip: "2607:f8b0:4005:805::2011"
    referrer : "http://testsafebrowsing.appspot.com/"
  }
  resources: {
    url      : "http://testsafebrowsing.appspot.com/"
    type     : TAB_URL
  }
  signature: {
  }
  user_initiated       : true
  file_basename: "content (32).exe"
  download_type: WIN_EXECUTABLE
  population: {
    user_population: SAFE_BROWSING
  }
  skipped_url_whitelist: false
  skipped_certificate_whitelist: false
}


And a "light" ping looks like this:
{
  url  : "http://testsafebrowsing.appspot.com/"
  digests: {
    sha256:
"\x0B\xA8p\x9C\xFDJ\xA9\xCA\xF4\x12\x87)]\x9E\x1B\x8Ej\xDAN\x01\x11\xBF\xAE\x9F\xD4\x1F\x08\xEE\xBBWQ~"
  }
  length       : 276
  resources: {
    url      : "http://testsafebrowsing.appspot.com/"
    type     : DOWNLOAD_URL
    remote_ip: "2607:f8b0:4005:805::2011"
    referrer : "http://testsafebrowsing.appspot.com/"
  }
  resources: {
    url      : "http://testsafebrowsing.appspot.com/"
    type     : TAB_URL
  }
  signature: {
  }
  user_initiated       : true
  file_basename: ".exe"
  download_type: SAMPLED_UNSUPPORTED_FILE
  population: {
    user_population: EXTENDED_REPORTING
  }
  skipped_url_whitelist: false
  skipped_certificate_whitelist: false
}

[Nathan Parker, 2016-06-17 21:45:05]

For your review (not sure if this sent email).  Thanks.

[asanka, 2016-06-20 18:15:46]

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

https://codereview.chromium.org/2072933002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:297: bool
ShouldSampleUnsupportedFile(const base::FilePath filename) {
Nit: Use 'const base::FilePath&'. We aren't benefiting from copy elision here
since the return value from DownloadItem::GetTargetFilePath() is a const
reference.

https://codereview.chromium.org/2072933002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:329:
PostFinishTask(UNKNOWN, reason);
Can we do some sort of incomplete ping for these cases? Or is that not
necessary.

https://codereview.chromium.org/2072933002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:337: } else {
Nit: else { unnecessary.

https://codereview.chromium.org/2072933002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:902: 
Nit: Extra blank line.

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

https://codereview.chromium.org/2072933002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:521:
FileTypePoliciesTestOverlay policies;
policies_

https://codereview.chromium.org/2072933002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:879: //
TODO(nparker): Check that URLs don't have paths, and has no filename.
Were you planning on resolving this in this CL?

https://codereview.chromium.org/2072933002/diff/1/chrome/common/safe_browsing...
File chrome/common/safe_browsing/file_type_policies.cc (right):

https://codereview.chromium.org/2072933002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/file_type_policies.cc:34: FileTypePolicies*
FileTypePolicies::instance_for_tests_ = 0;
Nit: nullptr

https://codereview.chromium.org/2072933002/diff/1/chrome/common/safe_browsing...
File chrome/common/safe_browsing/file_type_policies_test_util.h (right):

https://codereview.chromium.org/2072933002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/file_type_policies_test_util.h:34:
policies_->SetConfigForTesting(new_config);
Can we have something like a SwapConfigForTesting that we can call directly on
the singleton? That way we won't need to rework how
FileTypePolicies::GetInstance() works. THat would allow
FileTypePoliciesTestOveraly to restore the state of the FileTypePolicies without
having to swap the instance out.

Or just a SwapConfig() method that can be used by PopulateFromBinaryPb as well
as FileTypePoliciesTestOverlay.

I'm suggesting this because FileTypePolicies should be updateable at runtime
anyway. It seems we aren't saving any logic by swapping out FileTypePolicies.

[Nathan Parker, 2016-06-20 22:47:32]

Thanks! PTAL.

FYI I had to --bypass-hooks since presubmit is failing with the following, which
for now I assume is unrelated to my CL:

Traceback (most recent call last):
  File "/usr/local/google/home/nparker/git/depot_tools/git_cl.py", line 4894, in
<module>
    sys.exit(main(sys.argv[1:]))
  File "/usr/local/google/home/nparker/git/depot_tools/git_cl.py", line 4876, in
main
    return dispatcher.execute(OptionParser(), argv)
  File "/usr/local/google/home/nparker/git/depot_tools/subcommand.py", line 252,
in execute
    return command(parser, args[1:])
  File "/usr/local/google/home/nparker/git/depot_tools/git_cl.py", line 3607, in
CMDupload
    return cl.CMDUpload(options, args, orig_args)
  File "/usr/local/google/home/nparker/git/depot_tools/git_cl.py", line 1377, in
CMDUpload
    change=change)
  File "/usr/local/google/home/nparker/git/depot_tools/git_cl.py", line 1321, in
RunHook
    gerrit_obj=self._codereview_impl.GetGerritObjForPresubmit())
  File "/usr/local/google/home/nparker/git/depot_tools/presubmit_support.py",
line 1548, in DoPresubmitChecks
    results += executer.ExecPresubmitScript(presubmit_script, filename)
  File "/usr/local/google/home/nparker/git/depot_tools/presubmit_support.py",
line 1460, in ExecPresubmitScript
    result = eval(function_name + '(*__args)', context)
  File "<string>", line 1, in <module>
  File "<string>", line 2173, in CheckChangeOnUpload
  File "<string>", line 1925, in _CommonChecks
  File "<string>", line 1992, in _CheckAuthorizedAuthor
  File "<string>", line 1992, in <genexpr>
AttributeError: 'InputApi' object has no attribute 'fnmatch'

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

https://codereview.chromium.org/2072933002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:297: bool
ShouldSampleUnsupportedFile(const base::FilePath filename) {
On 2016/06/20 18:15:46, asanka wrote:
> Nit: Use 'const base::FilePath&'. We aren't benefiting from copy elision here
> since the return value from DownloadItem::GetTargetFilePath() is a const
> reference.

Done.

https://codereview.chromium.org/2072933002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:329:
PostFinishTask(UNKNOWN, reason);
On 2016/06/20 18:15:46, asanka wrote:
> Can we do some sort of incomplete ping for these cases? Or is that not
> necessary.

I don't really know what these are, other than file:// URLs.  They account for
0.04% of downloads.  Jialiu is splitting out file:///foo and file://host/foo, so
we'll be able to see how many of those, but I don't know that we need pings for
them.  We can't do much with file: URLs.

https://codereview.chromium.org/2072933002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:337: } else {
On 2016/06/20 18:15:46, asanka wrote:
> Nit: else { unnecessary.

Done.

https://codereview.chromium.org/2072933002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:902: 
On 2016/06/20 18:15:46, asanka wrote:
> Nit: Extra blank line.

Done.

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

https://codereview.chromium.org/2072933002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:521:
FileTypePoliciesTestOverlay policies;
On 2016/06/20 18:15:46, asanka wrote:
> policies_

Done.

https://codereview.chromium.org/2072933002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:879: //
TODO(nparker): Check that URLs don't have paths, and has no filename.
On 2016/06/20 18:15:46, asanka wrote:
> Were you planning on resolving this in this CL?

yes, and done.

https://codereview.chromium.org/2072933002/diff/1/chrome/common/safe_browsing...
File chrome/common/safe_browsing/file_type_policies.cc (right):

https://codereview.chromium.org/2072933002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/file_type_policies.cc:34: FileTypePolicies*
FileTypePolicies::instance_for_tests_ = 0;
On 2016/06/20 18:15:46, asanka wrote:
> Nit: nullptr

Done.

https://codereview.chromium.org/2072933002/diff/1/chrome/common/safe_browsing...
File chrome/common/safe_browsing/file_type_policies_test_util.h (right):

https://codereview.chromium.org/2072933002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/file_type_policies_test_util.h:34:
policies_->SetConfigForTesting(new_config);
On 2016/06/20 18:15:46, asanka wrote:
> Can we have something like a SwapConfigForTesting that we can call directly on
> the singleton? That way we won't need to rework how
> FileTypePolicies::GetInstance() works. THat would allow
> FileTypePoliciesTestOveraly to restore the state of the FileTypePolicies
without
> having to swap the instance out.
> 
> Or just a SwapConfig() method that can be used by PopulateFromBinaryPb as well
> as FileTypePoliciesTestOverlay.
> 
> I'm suggesting this because FileTypePolicies should be updateable at runtime
> anyway. It seems we aren't saving any logic by swapping out FileTypePolicies.

Sure, SGTM.  Done.

[asanka, 2016-06-21 00:51:08]

LGTM

Is your depot_tools checkout out-of-days? fnmatch was added to InputApi a couple
of days ago.

[Nathan Parker, 2016-06-21 01:07:41]

The CQ bit was checked by nparker@chromium.org

[commit-bot: I haz the power, 2016-06-21 01:08:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2072933002/40001

[Nathan Parker, 2016-06-21 01:08:20]

huh, that was it.  Ah: I had a modified file there that prevented the
auto-updating.

[Nathan Parker, 2016-06-21 16:10:06]

The CQ bit was checked by nparker@chromium.org

[Nathan Parker, 2016-06-21 16:10:06]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org
Link to the patchset: https://codereview.chromium.org/2072933002/#ps60001
(title: ".")

[commit-bot: I haz the power, 2016-06-21 16:10:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2072933002/60001

[commit-bot: I haz the power, 2016-06-21 16:12:58]

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

[commit-bot: I haz the power, 2016-06-21 16:12:59]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[Nathan Parker, 2016-06-21 16:38:36]

nparker@chromium.org changed reviewers:
+ holte@chromium.org

[Nathan Parker, 2016-06-21 16:38:36]

holte: Please review change to histograms.xml.  Thanks!

[Steven Holte, 2016-06-21 17:56:33]

lgtm

histograms lgtm

[Nathan Parker, 2016-06-21 18:00:25]

The CQ bit was checked by nparker@chromium.org

[Nathan Parker, 2016-06-21 18:00:25]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org
Link to the patchset: https://codereview.chromium.org/2072933002/#ps100001
(title: "Add int values for histograms to ensure they match")

[commit-bot: I haz the power, 2016-06-21 18:01:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2072933002/100001

[commit-bot: I haz the power, 2016-06-21 18:09:52]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-21 18:16:48]

Description was changed from

==========
Add sampling of unknown filetypes in Download Protection.

BUG=619743
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Add sampling of unknown filetypes in Download Protection.

BUG=619743
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/373f7c4c9722ebc0a718be3387d6f6c9b95d8c1f
Cr-Commit-Position: refs/heads/master@{#401047}
==========

[commit-bot: I haz the power, 2016-06-21 18:16:49]

Patchset 6 (id:??) landed as
https://crrev.com/373f7c4c9722ebc0a718be3387d6f6c9b95d8c1f
Cr-Commit-Position: refs/heads/master@{#401047}