Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2396133003:
Change the logic how Subesource Filter propagates activation. (Closed)
DescriptionChange the logic how Subesource Filter propagates activation.
Activate the Safe Browsing subresource filter only if the committed URL
was a Safe Browsing hit.
BUG=653686, 653805
Committed: https://crrev.com/ca2463865bb7900a0404dd8fed6523ad7ae605f1
Cr-Commit-Position: refs/heads/master@{#424986}
Patch Set 1 #Patch Set 2 : fix tests #Patch Set 3 : fix tests #Patch Set 4 : fix test, for real now #
Total comments: 3
Patch Set 5 : change clear() #
Total comments: 1
Patch Set 6 : activate only when hit #Patch Set 7 : fix browser tests #Patch Set 8 : remove empty line #Patch Set 9 : fix browser test #
Total comments: 16
Patch Set 10 : comments #
Total comments: 2
Patch Set 11 : grepsubst "hits_set()" "safe_browsing_blacklisted_patterns_set()" #Patch Set 12 : clear on start #Patch Set 13 : fix tests #Messages
Total messages: 80 (66 generated)
The CQ bit was checked by melandory@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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by melandory@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...
Description was changed from ========== Change the logic how Subesource Filter propagates activation. Activate the Safe Browsing subresource filter only if the committed URL was a Safe Browsing hit. BUG=653686 ========== to ========== Change the logic how Subesource Filter propagates activation. Activate the Safe Browsing subresource filter only if the committed URL was a Safe Browsing hit. BUG=653686, 653805 ==========
melandory@chromium.org changed reviewers: + engedy@chromium.org
The CQ bit was checked by melandory@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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by melandory@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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
melandory@chromium.org changed reviewers: + nparker@chromium.org
nparker@chromium.org: Please review changes in chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (bug description has more context)
nparker@chromium.org changed reviewers: + jialiul@chromium.org
-->jialiul since she is more expert on navigation
lgtm for safe_browsing_service_browsertests.cc with a couple of comments in content_subresource_filter_driver_factory https://codereview.chromium.org/2396133003/diff/60001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2396133003/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:149: HostSet activate_on_hosts_; (Please correct me if I missed something) It seems this set never gets cleared as long as the targeted WebContents is alive. I'm a little bit concerned that this set might grow very big in some corner cases. https://codereview.chromium.org/2396133003/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:154: HostSet hits_hosts_; nit: maybe add more comments to explain the differences between hits_hosts_ and activate_on_hosts_? It takes me a while to figure out hits_hosts_ is a super set of activate_on_hosts_.
The CQ bit was checked by melandory@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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Some initial comments. https://codereview.chromium.org/2396133003/diff/80001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2396133003/diff/80001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:154: HostSet hits_hosts_; Can we get away without adding any new state for now? Looks like we could just add the host of the SB blacklisted URL to the |activate_on_hosts_| in OnMainResourceMatchedSafeBrowsingBlacklist() directly.
As discussed off-line, let's remove the concept of activation sets altogether, and rely merely on the signal from the SafeBrowsingResourceThrottle. We will still need the whitelist, though.
The CQ bit was checked by melandory@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 checked by melandory@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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by melandory@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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by melandory@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 checked by melandory@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...
Balazs, please take another look. https://codereview.chromium.org/2396133003/diff/60001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2396133003/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:149: HostSet activate_on_hosts_; On 2016/10/07 21:54:27, Jialiu Lin wrote: > (Please correct me if I missed something) It seems this set never gets cleared > as long as the targeted WebContents is alive. I'm a little bit concerned that > this set might grow very big in some corner cases. Yes, it's true. We had a discussion within the team and decided that we actually can get rid of |activate_on_hosts_| and as result be more conservative with the activation.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by melandory@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...
Patchset #7 (id:120001) has been deleted
Patchset #7 (id:140001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2396133003/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right): https://codereview.chromium.org/2396133003/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:937: // Tests that URLS which doesn't belong to the SOCIAL_ENGINEERING_ADS threat Drive-by comment: Don't we need the ScopedSubresourceFilterFeatureToggle set up here for this test too? (Address in follow-up CL if needed.) https://codereview.chromium.org/2396133003/diff/200001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2396133003/diff/200001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:143: // List of host+path of the URLs, where the Safe Browsing detected hit nit: Extra words in this sentence. https://codereview.chromium.org/2396133003/diff/200001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:145: // to determine whenever the activation signal should be send, then all nit: s/send/sent/ https://codereview.chromium.org/2396133003/diff/200001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:146: // entities is deleted from the list. nit: s/is/are/ https://codereview.chromium.org/2396133003/diff/200001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:147: HostSet hits_hosts_; This set no longer stores hosts, but host+path pairs. Let's reflect that in the type name. Also, how about naming this something like |safe_browsing_blacklisted_patterns|, so it's more expressive? I would also suggest renaming helper functions similarly. https://codereview.chromium.org/2396133003/diff/200001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2396133003/diff/200001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:403: // Check when the experiment is enabled for all site, the activation signal is nit: Check that when ... https://codereview.chromium.org/2396133003/diff/200001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc (right): https://codereview.chromium.org/2396133003/diff/200001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc:37: navigation_handle()->GetRenderFrameHost(), initial_url_); We will need to send the final URL here.
The CQ bit was checked by melandory@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 checked by melandory@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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #10 (id:220001) has been deleted
Patchset #10 (id:240001) has been deleted
The CQ bit was checked by melandory@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...
https://codereview.chromium.org/2396133003/diff/200001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right): https://codereview.chromium.org/2396133003/diff/200001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:937: // Tests that URLS which doesn't belong to the SOCIAL_ENGINEERING_ADS threat On 2016/10/12 09:32:29, engedy wrote: > Drive-by comment: Don't we need the ScopedSubresourceFilterFeatureToggle set up > here for this test too? (Address in follow-up CL if needed.) Done. https://codereview.chromium.org/2396133003/diff/200001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2396133003/diff/200001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:143: // List of host+path of the URLs, where the Safe Browsing detected hit On 2016/10/12 09:32:29, engedy wrote: > nit: Extra words in this sentence. Haven't found them, but changed the formulation, so might accidentally fixed the issue. https://codereview.chromium.org/2396133003/diff/200001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:145: // to determine whenever the activation signal should be send, then all On 2016/10/12 09:32:29, engedy wrote: > nit: s/send/sent/ Done. https://codereview.chromium.org/2396133003/diff/200001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:146: // entities is deleted from the list. On 2016/10/12 09:32:29, engedy wrote: > nit: s/is/are/ Done. https://codereview.chromium.org/2396133003/diff/200001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:147: HostSet hits_hosts_; On 2016/10/12 09:32:29, engedy wrote: > This set no longer stores hosts, but host+path pairs. Let's reflect that in the > type name. > > Also, how about naming this something like |safe_browsing_blacklisted_patterns|, > so it's more expressive? I would also suggest renaming helper functions > similarly. Done. https://codereview.chromium.org/2396133003/diff/200001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2396133003/diff/200001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:403: // Check when the experiment is enabled for all site, the activation signal is On 2016/10/12 09:32:29, engedy wrote: > nit: Check that when ... Done. https://codereview.chromium.org/2396133003/diff/200001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc (right): https://codereview.chromium.org/2396133003/diff/200001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc:37: navigation_handle()->GetRenderFrameHost(), initial_url_); On 2016/10/12 09:32:29, engedy wrote: > We will need to send the final URL here. Done.
LGTM % comments. Once we get rid of the NavigationThrottle, let's do a huge overhaul of the unittests for the Driver, it's getting really hard to follow what is tested and what isn't. https://codereview.chromium.org/2396133003/diff/200001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2396133003/diff/200001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:143: // List of host+path of the URLs, where the Safe Browsing detected hit On 2016/10/12 12:29:00, melandory wrote: > On 2016/10/12 09:32:29, engedy wrote: > > nit: Extra words in this sentence. > > Haven't found them, but changed the formulation, so might accidentally fixed the > issue. Phrasing suggestion: During the course of one navigation, collects those URLs in the redirect chain that match Safe Browsing blacklists. The URLs stripped to (host + path). When the navigation is committed, the activation signal is sent depending on whether the (host + path) of the final URL is in this list. Contents are wiped between navigations. https://codereview.chromium.org/2396133003/diff/200001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:147: HostSet hits_hosts_; Please see the two other requests in this comment too. https://codereview.chromium.org/2396133003/diff/260001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2396133003/diff/260001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:122: safe_browsing_blacklisted_patterns_.clear(); We need to clear this in case the navigation is cancelled and never gets committed. Maybe just clear it in DidStartProvisionalLoadForFrame inside the conditional for the main frame?
The CQ bit was checked by melandory@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...
https://codereview.chromium.org/2396133003/diff/260001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2396133003/diff/260001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:122: safe_browsing_blacklisted_patterns_.clear(); On 2016/10/12 12:47:05, engedy wrote: > We need to clear this in case the navigation is cancelled and never gets > committed. Maybe just clear it in DidStartProvisionalLoadForFrame inside the > conditional for the main frame? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by melandory@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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jialiul@chromium.org, engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2396133003/#ps320001 (title: "fix tests")
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.
Description was changed from ========== Change the logic how Subesource Filter propagates activation. Activate the Safe Browsing subresource filter only if the committed URL was a Safe Browsing hit. BUG=653686, 653805 ========== to ========== Change the logic how Subesource Filter propagates activation. Activate the Safe Browsing subresource filter only if the committed URL was a Safe Browsing hit. BUG=653686, 653805 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Change the logic how Subesource Filter propagates activation. Activate the Safe Browsing subresource filter only if the committed URL was a Safe Browsing hit. BUG=653686, 653805 ========== to ========== Change the logic how Subesource Filter propagates activation. Activate the Safe Browsing subresource filter only if the committed URL was a Safe Browsing hit. BUG=653686, 653805 Committed: https://crrev.com/ca2463865bb7900a0404dd8fed6523ad7ae605f1 Cr-Commit-Position: refs/heads/master@{#424986} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/ca2463865bb7900a0404dd8fed6523ad7ae605f1 Cr-Commit-Position: refs/heads/master@{#424986} |
