Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2146703002:
Sample 1% url whitelisted PPAPI downloads to ping safe browsing server (Closed)
|
Created:
4 years, 5 months ago by Jialiu Lin Modified:
4 years, 5 months ago CC:
chromium-reviews, grt+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSample 1% url whitelisted PPAPI downloads to ping SB server
Similar to regular downloads, we want SB pings for requestors whose
URLs match the SB whitelist in flash downloads. To achieve that, we
sample 1% of whitelisted PPAPI downloads based on the
SafeBrowsingService::whitelist_sample_rate() if the user is in
extended reporting group and not in incognito mode.
Also refactored download_protection_service_unittest to elliminate
the use of MessageLoop (deprecated).
BUG=610924
Committed: https://crrev.com/96f81d5c22a83e5ea061484c7390f37e41816f31
Cr-Commit-Position: refs/heads/master@{#406101}
Patch Set 1 : Sample 1% whitelisted PPAPI downloads #Patch Set 2 : fix browser_tests #
Total comments: 2
Patch Set 3 : pass in profile instead #Patch Set 4 : nit #
Total comments: 4
Patch Set 5 : address asanka@'s comment #
Total comments: 8
Patch Set 6 : address thestig@'s comments #
Total comments: 4
Patch Set 7 : refactor unit tests #
Messages
Total messages: 39 (21 generated)
Description was changed from ========== temp BUG= ========== to ========== Similar to regular downloads, we want SB pings for requestors whose URLs match the SB whitelist in flash downloads. To achieve that, we sampling 1% of them based on the SafeBrowsingService::whitelist_sample_rate() if the user is in extended reporting group and not in incognito mode. BUG=610924 ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Similar to regular downloads, we want SB pings for requestors whose URLs match the SB whitelist in flash downloads. To achieve that, we sampling 1% of them based on the SafeBrowsingService::whitelist_sample_rate() if the user is in extended reporting group and not in incognito mode. BUG=610924 ========== to ========== Similar to regular downloads, we want SB pings for requestors whose URLs match the SB whitelist in flash downloads. To achieve that, we sample 1% of whitelisted PPAPI downloads based on the SafeBrowsingService::whitelist_sample_rate() if the user is in extended reporting group and not in incognito mode. BUG=610924 ==========
jialiul@chromium.org changed reviewers: + asanka@chromium.org
Hi asanka@, Let me know if you have time to take a look at this short CL before your vacation. Thanks,
https://codereview.chromium.org/2146703002/diff/40001/chrome/browser/file_sel... File chrome/browser/file_select_helper.cc (right): https://codereview.chromium.org/2146703002/diff/40001/chrome/browser/file_sel... chrome/browser/file_select_helper.cc:543: is_extended_reporting, is_incognito, Since CheckPPAPIDownloadRequest's behavior changes depending on the type of profile and profile preferences, shall we pass the Profile into CheckPPAPIDownloadRequest() instead? That way FileSelectHelper doesn't need to determine whether extended reporting is enabled. The latter should be a part of the SafeBrowsing logic and not something that callers should be concerned with.
Thanks, asanka@! Now this code passes in Profile instead of two booleans. https://codereview.chromium.org/2146703002/diff/40001/chrome/browser/file_sel... File chrome/browser/file_select_helper.cc (right): https://codereview.chromium.org/2146703002/diff/40001/chrome/browser/file_sel... chrome/browser/file_select_helper.cc:543: is_extended_reporting, is_incognito, On 2016/07/14 at 16:48:15, asanka wrote: > Since CheckPPAPIDownloadRequest's behavior changes depending on the type of profile and profile preferences, shall we pass the Profile into CheckPPAPIDownloadRequest() instead? That way FileSelectHelper doesn't need to determine whether extended reporting is enabled. The latter should be a part of the SafeBrowsing logic and not something that callers should be concerned with. Yep, passing the profile is much better. Done.
Overall LG. One thing to fix about how we handle Profile*. I didn't look closely, but you'll also want to make sure that the RecordCountOfWhitelistedDownload() is either invoked for all download requests or just for requests which end up matching the whitelist. In the PPAPI case, UMA is only recorded if there's a whitelist hit. https://codereview.chromium.org/2146703002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2146703002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.cc:1202: profile_(profile), Note that DownloadProtectionService has not lifetime tie-in with Profile. As such, storing a pointer to a Profile can be dangerous. Since we are now in /safe_browsing/ you can now call ShouldSampleWhitelistedDownload() and store the resulting boolean rather than keep Profile* around. https://codereview.chromium.org/2146703002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/download_protection_service_unittest.cc (right): https://codereview.chromium.org/2146703002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service_unittest.cc:2079: ASSERT_TRUE(IsResult(DownloadProtectionService::SAFE)); Did you verify that these two tests reliably fail if DownloadProtectionService attempts to send a ping?
On 2016/07/14 at 20:44:32, asanka wrote: > Overall LG. One thing to fix about how we handle Profile*. > > I didn't look closely, but you'll also want to make sure that the RecordCountOfWhitelistedDownload() is either invoked for all download requests or just for requests which end up matching the whitelist. In the PPAPI case, UMA is only recorded if there's a whitelist hit. > > https://codereview.chromium.org/2146703002/diff/80001/chrome/browser/safe_bro... > File chrome/browser/safe_browsing/download_protection_service.cc (right): > > https://codereview.chromium.org/2146703002/diff/80001/chrome/browser/safe_bro... > chrome/browser/safe_browsing/download_protection_service.cc:1202: profile_(profile), > Note that DownloadProtectionService has not lifetime tie-in with Profile. As such, storing a pointer to a Profile can be dangerous. Since we are now in /safe_browsing/ you can now call ShouldSampleWhitelistedDownload() and store the resulting boolean rather than keep Profile* around. > > https://codereview.chromium.org/2146703002/diff/80001/chrome/browser/safe_bro... > File chrome/browser/safe_browsing/download_protection_service_unittest.cc (right): > > https://codereview.chromium.org/2146703002/diff/80001/chrome/browser/safe_bro... > chrome/browser/safe_browsing/download_protection_service_unittest.cc:2079: ASSERT_TRUE(IsResult(DownloadProtectionService::SAFE)); > Did you verify that these two tests reliably fail if DownloadProtectionService attempts to send a ping? Thanks for catching the RecordCountOfWhitelistedDownload problem. Your other comments are addressed too. Thanks!
jialiul@chromium.org changed reviewers: + thestig@chromium.org
thestig@chromium.org: PTAL, need your owner review of chrome/browser/file_select_helper.cc and chrome/test/ppapi/ppapi_filechooser_browsertest.cc Thanks! https://codereview.chromium.org/2146703002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2146703002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.cc:1202: profile_(profile), On 2016/07/14 at 20:44:32, asanka wrote: > Note that DownloadProtectionService has not lifetime tie-in with Profile. As such, storing a pointer to a Profile can be dangerous. Since we are now in /safe_browsing/ you can now call ShouldSampleWhitelistedDownload() and store the resulting boolean rather than keep Profile* around. Make sense. Thanks! https://codereview.chromium.org/2146703002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/download_protection_service_unittest.cc (right): https://codereview.chromium.org/2146703002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service_unittest.cc:2079: ASSERT_TRUE(IsResult(DownloadProtectionService::SAFE)); On 2016/07/14 at 20:44:32, asanka wrote: > Did you verify that these two tests reliably fail if DownloadProtectionService attempts to send a ping? Added some code (and comments) to make these test cases stronger. To verify there is no ping sent out, server side response is set to DANGEROUS. If DownloadProtectionService does send a ping, result should be DANGEROUS instead of SAFE.
Will take a look. BTW, the CL description is what shows up in the git commit. Not sure why, but it's missing the content of the subject line. As is, if this lands, the commit message will be a bit confusing.
Description was changed from ========== Similar to regular downloads, we want SB pings for requestors whose URLs match the SB whitelist in flash downloads. To achieve that, we sample 1% of whitelisted PPAPI downloads based on the SafeBrowsingService::whitelist_sample_rate() if the user is in extended reporting group and not in incognito mode. BUG=610924 ========== to ========== Sample 1% url whitelisted PPAPI downloads to ping SB server Similar to regular downloads, we want SB pings for requestors whose URLs match the SB whitelist in flash downloads. To achieve that, we sample 1% of whitelisted PPAPI downloads based on the SafeBrowsingService::whitelist_sample_rate() if the user is in extended reporting group and not in incognito mode. BUG=610924 ==========
Description was changed from ========== Sample 1% url whitelisted PPAPI downloads to ping SB server Similar to regular downloads, we want SB pings for requestors whose URLs match the SB whitelist in flash downloads. To achieve that, we sample 1% of whitelisted PPAPI downloads based on the SafeBrowsingService::whitelist_sample_rate() if the user is in extended reporting group and not in incognito mode. BUG=610924 ========== to ========== Sample 1% url whitelisted PPAPI downloads to ping SB server Similar to regular downloads, we want SB pings for requestors whose URLs match the SB whitelist in flash downloads. To achieve that, we sample 1% of whitelisted PPAPI downloads based on the SafeBrowsingService::whitelist_sample_rate() if the user is in extended reporting group and not in incognito mode. BUG=610924 ==========
lgtm with asanka's approval. https://codereview.chromium.org/2146703002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2146703002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:75: namespace { Add some blank lines on the insides of the namespace. https://codereview.chromium.org/2146703002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:87: static void RecordCountOfWhitelistedDownload(WhitelistType type) { static inside an anonymous namespace is redundant. Ditto nitto above. https://codereview.chromium.org/2146703002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:1292: if (ShouldSampleWhitelistedDownload()) { Logic might flow more easily if written as: if (!Foo()) { DoBar(); return; } DoFoo(); https://codereview.chromium.org/2146703002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service_unittest.cc (right): https://codereview.chromium.org/2146703002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service_unittest.cc:2114: MessageLoop::current()->Run(); This is deprecated. Use a base::RunLoop instead. See the MessageLoop::Run() header / impl.
Thanks for your thorough review, thestig@! https://codereview.chromium.org/2146703002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2146703002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:75: namespace { On 2016/07/14 23:56:11, Lei Zhang wrote: > Add some blank lines on the insides of the namespace. Done. https://codereview.chromium.org/2146703002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:87: static void RecordCountOfWhitelistedDownload(WhitelistType type) { On 2016/07/14 23:56:11, Lei Zhang wrote: > static inside an anonymous namespace is redundant. Ditto nitto above. Done. https://codereview.chromium.org/2146703002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:1292: if (ShouldSampleWhitelistedDownload()) { On 2016/07/14 23:56:11, Lei Zhang wrote: > Logic might flow more easily if written as: > > if (!Foo()) { > DoBar(); > return; > } > DoFoo(); Done. https://codereview.chromium.org/2146703002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service_unittest.cc (right): https://codereview.chromium.org/2146703002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service_unittest.cc:2114: MessageLoop::current()->Run(); On 2016/07/14 23:56:11, Lei Zhang wrote: > This is deprecated. Use a base::RunLoop instead. See the MessageLoop::Run() > header / impl. Done.
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by jialiul@chromium.org
LGTM! https://codereview.chromium.org/2146703002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2146703002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:1219: is_extended_reporting_ = profile && Minor nit: Just noting that Profile had better be non-null. PPAPIDownloadRequest is always called on behalf of a specific profile even though DownloadProtectionService itself isn't tied to one. It might be clearer to our readers if you DCHECK(profile) and then use it without additional non-null checks. https://codereview.chromium.org/2146703002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service_unittest.cc (right): https://codereview.chromium.org/2146703002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service_unittest.cc:2074: base::RunLoop().Run(); Perhaps not for this CL since it's going to involve changing all the tests that use DownloadProtectionService::CheckDoneCallback(). The recommended pattern for running message loops during tests is to: 1. Pick a point in the test where whatever the test was waiting for has happened, and make the test invoke base::RunLoop().QuitClosure() at that point. 2. Invoke base::RunLoop().Run(). This way, the test doesn't need to worry about some other test method invoking a nested run loop. In addition the test won't be sensitive to any stray tasks that may also be scheduled on the same message loop. The call site would look like: base::RunLoop run_loop; download_service_->CheckPPAPI...(base::Bind(&CheckDoneCallback, base:Unretained(this), run_loop.QuitClosure()); run_loop.Run(); CheckDoneCallback would take a base::Closure parameter that can be invoked to break out of the correct run loop instead of invoking MessageLoop::current()->QuitWhenIdle().
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
The CQ bit was unchecked by jialiul@chromium.org
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by jialiul@chromium.org
Description was changed from ========== Sample 1% url whitelisted PPAPI downloads to ping SB server Similar to regular downloads, we want SB pings for requestors whose URLs match the SB whitelist in flash downloads. To achieve that, we sample 1% of whitelisted PPAPI downloads based on the SafeBrowsingService::whitelist_sample_rate() if the user is in extended reporting group and not in incognito mode. BUG=610924 ========== to ========== Sample 1% url whitelisted PPAPI downloads to ping SB server Similar to regular downloads, we want SB pings for requestors whose URLs match the SB whitelist in flash downloads. To achieve that, we sample 1% of whitelisted PPAPI downloads based on the SafeBrowsingService::whitelist_sample_rate() if the user is in extended reporting group and not in incognito mode. Also refactored download_protection_service_unittest to elliminate the use of MessageLoop (deprecated). BUG=610924 ==========
Thanks, asanka@! https://codereview.chromium.org/2146703002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2146703002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:1219: is_extended_reporting_ = profile && On 2016/07/15 at 14:59:52, asanka (On leave until Aug 29) wrote: > Minor nit: Just noting that Profile had better be non-null. PPAPIDownloadRequest is always called on behalf of a specific profile even though DownloadProtectionService itself isn't tied to one. It might be clearer to our readers if you DCHECK(profile) and then use it without additional non-null checks. Done. https://codereview.chromium.org/2146703002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service_unittest.cc (right): https://codereview.chromium.org/2146703002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service_unittest.cc:2074: base::RunLoop().Run(); On 2016/07/15 at 14:59:52, asanka (On leave until Aug 29) wrote: > Perhaps not for this CL since it's going to involve changing all the tests that use DownloadProtectionService::CheckDoneCallback(). > > The recommended pattern for running message loops during tests is to: > > 1. Pick a point in the test where whatever the test was waiting for has happened, and make the test invoke base::RunLoop().QuitClosure() at that point. > 2. Invoke base::RunLoop().Run(). > > This way, the test doesn't need to worry about some other test method invoking a nested run loop. In addition the test won't be sensitive to any stray tasks that may also be scheduled on the same message loop. > > The call site would look like: > > base::RunLoop run_loop; > download_service_->CheckPPAPI...(base::Bind(&CheckDoneCallback, base:Unretained(this), run_loop.QuitClosure()); > run_loop.Run(); > > CheckDoneCallback would take a base::Closure parameter that can be invoked to break out of the correct run loop instead of invoking MessageLoop::current()->QuitWhenIdle(). Thanks for your detailed explanation on RunLoop. I have refactored all the tests accordingly. No more MessageLoop needed.
The CQ bit was checked by jialiul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, asanka@chromium.org Link to the patchset: https://codereview.chromium.org/2146703002/#ps140001 (title: "refactor unit tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by jialiul@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Sample 1% url whitelisted PPAPI downloads to ping SB server Similar to regular downloads, we want SB pings for requestors whose URLs match the SB whitelist in flash downloads. To achieve that, we sample 1% of whitelisted PPAPI downloads based on the SafeBrowsingService::whitelist_sample_rate() if the user is in extended reporting group and not in incognito mode. Also refactored download_protection_service_unittest to elliminate the use of MessageLoop (deprecated). BUG=610924 ========== to ========== Sample 1% url whitelisted PPAPI downloads to ping SB server Similar to regular downloads, we want SB pings for requestors whose URLs match the SB whitelist in flash downloads. To achieve that, we sample 1% of whitelisted PPAPI downloads based on the SafeBrowsingService::whitelist_sample_rate() if the user is in extended reporting group and not in incognito mode. Also refactored download_protection_service_unittest to elliminate the use of MessageLoop (deprecated). BUG=610924 Committed: https://crrev.com/96f81d5c22a83e5ea061484c7390f37e41816f31 Cr-Commit-Position: refs/heads/master@{#406101} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/96f81d5c22a83e5ea061484c7390f37e41816f31 Cr-Commit-Position: refs/heads/master@{#406101}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:140001) has been created in https://codereview.chromium.org/2182333003/ by jialiul@chromium.org. The reason for reverting is: high crashing rate on canary https://crbug.com/630778. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
