SubresourceFilter metadata loss can cause minor issues |
||
Issue descriptionThe subresource_filter metadata (i.e. our website setting CONTENT_SETTINGS_TYPE_ADS_DATA) tracks whether a given origin has activation. We do this by checking for the existence of the metadata. The existence triggers a bunch of settings UI (for instance, page info and site details). However, the data is lossy. So if the following sequence happens: 1. Site is activated, UI is shown, and metadata populated 2. User explicitly allows ads on the site 3. Metadata is lost via history deletion, etc. 4. User visits site, should show page info and site details but does not. Only way to enable ads on the site is through the site settings in settings/content/ads. One solution would be to re-add the metadata (without a timestamp) when the site is deemed whitelisted, but it is hard to know whether the whitelist status is from the global list or via an individual rule. To solve it really, we may need to sniff the safe browsing result.
,
Jul 26 2017
I have a fix for this in flight, but we need to be careful that it doesn't interfere with the devtools changes. I.e. we should NOT be adding empty metadata if devtools has forced activation.
,
Jul 26 2017
Just to confirm - the proposed solution doesn't change the invariant that the website setting will never be written for a website that wasn't visited, and will always be deleted when history is deleted? And regarding "it is hard to know whether the whitelist status is from the global list or via an individual rule" -> you could look for it in HostContentSettingsMap::GetSettingsForOneType() (which is basically what HostContentSettingsMap is, in fact, going to do).
,
Jul 26 2017
1. The solution adds metadata during a top-level navigation, right before the commit point (NavigationThrottle::WillProcessResponse), so I think it can be considered "visited" at that point, though I'm not sure if you have a more precise definition. 2. Everything will still be deleted when history entries are deleted. 3. Aha I knew there was something like that! Thanks! Honestly I still prefer the solution I ended up using since we already needed to plumb the activation for metadata deletion. So this is a more clean mapping of maintaining the invariant.
,
Jul 26 2017
Sounds good, thanks!
,
Jul 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bd66cd356ff33fff2d3396edecec46eae52034d4 commit bd66cd356ff33fff2d3396edecec46eae52034d4 Author: Charles Harrison <csharrison@chromium.org> Date: Wed Jul 26 21:46:09 2017 [subresource_filter] Properly refresh metadata upon activation Namely, if the metadata has been dropped for any reason, and the site is activated, then the metadata should be refreshed to an empty dict. Normally, this would happen anyway since the UI would show, but this may not necessarily happen if the site is whitelisted. Bug: 748691 Change-Id: Ib0e6fd96bb6a52641cde1f7130e23878ab5dc736 Reviewed-on: https://chromium-review.googlesource.com/585708 Reviewed-by: Shivani Sharma <shivanisha@chromium.org> Reviewed-by: Martin Šrámek <msramek@chromium.org> Commit-Queue: Charlie Harrison <csharrison@chromium.org> Cr-Commit-Position: refs/heads/master@{#489763} [modify] https://crrev.com/bd66cd356ff33fff2d3396edecec46eae52034d4/chrome/browser/subresource_filter/chrome_subresource_filter_client.cc [modify] https://crrev.com/bd66cd356ff33fff2d3396edecec46eae52034d4/chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc [modify] https://crrev.com/bd66cd356ff33fff2d3396edecec46eae52034d4/chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h [modify] https://crrev.com/bd66cd356ff33fff2d3396edecec46eae52034d4/chrome/browser/subresource_filter/subresource_filter_unittest.cc
,
Jul 31 2017
|
||
►
Sign in to add a comment |
||
Comment 1 by csharrison@chromium.org
, Jul 25 2017Status: Started (was: Untriaged)