Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2510363003:
Only activate radio buttons in ContentSettingsBubble for valid content settings. This prevents fail… (Closed)
DescriptionOnly activate radio buttons in ContentSettingsBubble for valid content settings. This prevents failed DCHECK e.g. for about:blank if you try to create popups from the console.
BUG=244176
Committed: https://crrev.com/b98654eba7ccaf4bc7e554c0e11c412f9d72d690
Cr-Commit-Position: refs/heads/master@{#436285}
Patch Set 1 #
Total comments: 4
Patch Set 2 : extract GetNarrowestSetting, add isValid check #Patch Set 3 : add static modifier #
Total comments: 6
Patch Set 4 : rename and move GetNarrowestPattern #Patch Set 5 : fix ContentSettingsBubbleUnittest #Patch Set 6 : use narrowest pattern for isValid check #Patch Set 7 : add test for radio_group_enabled with valid and invalid url #Patch Set 8 : rename IsValidSetting #
Total comments: 10
Patch Set 9 : fix comments #Patch Set 10 : rebase #
Messages
Total messages: 35 (20 generated)
dullweber@chromium.org changed reviewers: + msramek@chromium.org
Martin, please review this change
Description was changed from ========== Only activate radio buttons in ContentSettingsBubble for valid content settings. This prevents failed DCHECK e.g. for about:blank. BUG=244176 ========== to ========== Only activate radio buttons in ContentSettingsBubble for valid content settings. This prevents failed DCHECK e.g. for about:blank if you try to create popups from the console. BUG=244176 ==========
https://codereview.chromium.org/2510363003/diff/1/components/content_settings... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2510363003/diff/1/components/content_settings... components/content_settings/core/browser/host_content_settings_map.cc:330: content_settings::PatternPair patterns = GetPatternsFromScopingType( This tests whether the patterns generated from the URL following the default scoping are valid, i.e. whether SetContentSettingDefaultScope would succeed. However, that method already contains the same check internally (line 363 and 364). The problem is that ContentSettingBubbleModel calls SetNarrowestContentSetting which does not have the same check for pattern validity. We should probably add it there. That will prevent the crash. Now, if we also want to disable the radio group, we still need this method, but it should test the narrowest setting. Could we maybe extract the computation of the narrowest setting to a helper function and use it here and in SetNarrowestContentSetting? https://codereview.chromium.org/2510363003/diff/1/components/content_settings... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/2510363003/diff/1/components/content_settings... components/content_settings/core/browser/host_content_settings_map.h:130: bool IsValidSetting(const GURL& primary_url, const GURL& secondary_url, static?
https://codereview.chromium.org/2510363003/diff/1/components/content_settings... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2510363003/diff/1/components/content_settings... components/content_settings/core/browser/host_content_settings_map.cc:330: content_settings::PatternPair patterns = GetPatternsFromScopingType( On 2016/11/21 09:34:53, msramek wrote: > This tests whether the patterns generated from the URL following the default > scoping are valid, i.e. whether SetContentSettingDefaultScope would succeed. > However, that method already contains the same check internally (line 363 and > 364). > > The problem is that ContentSettingBubbleModel calls SetNarrowestContentSetting > which does not have the same check for pattern validity. We should probably add > it there. That will prevent the crash. > > Now, if we also want to disable the radio group, we still need this method, but > it should test the narrowest setting. Could we maybe extract the computation of > the narrowest setting to a helper function and use it here and in > SetNarrowestContentSetting? I added the additional check and extracted a helper function to get the narrowest pattern https://codereview.chromium.org/2510363003/diff/1/components/content_settings... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/2510363003/diff/1/components/content_settings... components/content_settings/core/browser/host_content_settings_map.h:130: bool IsValidSetting(const GURL& primary_url, const GURL& secondary_url, On 2016/11/21 09:34:53, msramek wrote: > static? Done.
https://codereview.chromium.org/2510363003/diff/40001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2510363003/diff/40001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:334: content_settings::PatternPair HostContentSettingsMap::GetNarrowestPattern( This is not the narrowest pattern, it's the default-scoped pattern GetPatternsFromScopingType(website_settings_info->scoping_type(), ... ^ | default pattern scoping for this type The narrowest pattern is computed inside SetNarrowestContentSetting() by comparing the default-scoped pattern with a setting that already exists in the map, and narrowing it if necessary. To explain the usecase: 1. You have an exception blocking cookies for "https://google.com:443" 2. You use the bubble to allow cookies for "google.com". 3. The bubble adds an allow exception for "[*.]google.com" 4. Nothing happened, because the new pattern is wider than the old one 5. Solution: The bubble should attempt to add a narrower (or equally wide) pattern. https://codereview.chromium.org/2510363003/diff/40001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/2510363003/diff/40001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.h:131: ContentSettingsType type); style: offset https://codereview.chromium.org/2510363003/diff/40001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.h:331: // Gets the most specific pattern that currently defines the setting for the Prefer a helper function in an anonymous namespace instead of a private static one.
The CQ bit was checked by dullweber@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/2510363003/diff/40001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2510363003/diff/40001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:334: content_settings::PatternPair HostContentSettingsMap::GetNarrowestPattern( On 2016/11/21 15:48:30, msramek wrote: > This is not the narrowest pattern, it's the default-scoped pattern > > GetPatternsFromScopingType(website_settings_info->scoping_type(), ... > ^ > | > default pattern scoping for this type > > > The narrowest pattern is computed inside SetNarrowestContentSetting() by > comparing the default-scoped pattern with a setting that already exists in the > map, and narrowing it if necessary. > > To explain the usecase: > 1. You have an exception blocking cookies for "https://google.com:443" > 2. You use the bubble to allow cookies for "google.com". > 3. The bubble adds an allow exception for "[*.]google.com" > 4. Nothing happened, because the new pattern is wider than the old one > 5. Solution: The bubble should attempt to add a narrower (or equally wide) > pattern. I renamed it to GetPatternsFromContentSettingsType() https://codereview.chromium.org/2510363003/diff/40001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/2510363003/diff/40001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.h:131: ContentSettingsType type); On 2016/11/21 15:48:30, msramek wrote: > style: offset Done. https://codereview.chromium.org/2510363003/diff/40001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.h:331: // Gets the most specific pattern that currently defines the setting for the On 2016/11/21 15:48:30, msramek wrote: > Prefer a helper function in an anonymous namespace instead of a private static > one. 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 dullweber@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by dullweber@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 dullweber@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...
dullweber@chromium.org changed reviewers: + markusheintz@chromium.org
markusheintz@chromium.org: Please review changes in content_settings_bubble_model
Thanks. I think this is a better design than before. LGTM % comments and nits. https://codereview.chromium.org/2510363003/diff/140001/chrome/browser/content... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/2510363003/diff/140001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_unittest.cc:1747: auto* map = HostContentSettingsMapFactory::GetForProfile(&profile); nit: const That way, we'll also test that CanSetNarrowestContentSetting is const. https://codereview.chromium.org/2510363003/diff/140001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2510363003/diff/140001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:404: auto* map = HostContentSettingsMapFactory::GetForProfile(profile()); nit: const https://codereview.chromium.org/2510363003/diff/140001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2510363003/diff/140001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:156: content_settings::PatternPair GetPatternsFromContentSettingsType( nit: I'd suggest "For" rather than "From". We're not directly transforming CS type to patterns; we're transforming URLs to patterns, according to rules for a CS type. https://codereview.chromium.org/2510363003/diff/140001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:406: ContentSettingsPattern narrow_primary = patterns.first; This renaming seems quite unnecessary. https://codereview.chromium.org/2510363003/diff/140001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/2510363003/diff/140001/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:128: // Check if a call to SetNarrowestContentSetting would succeed or if it would nit: Maybe move this right before SetNarrowestContentSetting? (Here, and in the .cc file as well)
https://codereview.chromium.org/2510363003/diff/140001/chrome/browser/content... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/2510363003/diff/140001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_unittest.cc:1747: auto* map = HostContentSettingsMapFactory::GetForProfile(&profile); On 2016/11/22 14:24:11, msramek wrote: > nit: const > > That way, we'll also test that CanSetNarrowestContentSetting is const. Done. https://codereview.chromium.org/2510363003/diff/140001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2510363003/diff/140001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:404: auto* map = HostContentSettingsMapFactory::GetForProfile(profile()); On 2016/11/22 14:24:11, msramek wrote: > nit: const Done. https://codereview.chromium.org/2510363003/diff/140001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2510363003/diff/140001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:156: content_settings::PatternPair GetPatternsFromContentSettingsType( On 2016/11/22 14:24:11, msramek wrote: > nit: I'd suggest "For" rather than "From". > > We're not directly transforming CS type to patterns; we're transforming URLs to > patterns, according to rules for a CS type. Done. https://codereview.chromium.org/2510363003/diff/140001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:406: ContentSettingsPattern narrow_primary = patterns.first; On 2016/11/22 14:24:11, msramek wrote: > This renaming seems quite unnecessary. Done. https://codereview.chromium.org/2510363003/diff/140001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/2510363003/diff/140001/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:128: // Check if a call to SetNarrowestContentSetting would succeed or if it would On 2016/11/22 14:24:12, msramek wrote: > nit: Maybe move this right before SetNarrowestContentSetting? (Here, and in the > .cc file as well) Done.
Still LGTM.
On 2016/11/22 14:56:22, msramek wrote: > Still LGTM. LGTM
The CQ bit was checked by dullweber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org Link to the patchset: https://codereview.chromium.org/2510363003/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1480945144391310,
"parent_rev": "03be08ceed442595a701fe664b7716d10da3aad5", "commit_rev":
"644490a184803bd7a96c7142a6751da37af66938"}
Message was sent while issue was closed.
Description was changed from ========== Only activate radio buttons in ContentSettingsBubble for valid content settings. This prevents failed DCHECK e.g. for about:blank if you try to create popups from the console. BUG=244176 ========== to ========== Only activate radio buttons in ContentSettingsBubble for valid content settings. This prevents failed DCHECK e.g. for about:blank if you try to create popups from the console. BUG=244176 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Only activate radio buttons in ContentSettingsBubble for valid content settings. This prevents failed DCHECK e.g. for about:blank if you try to create popups from the console. BUG=244176 ========== to ========== Only activate radio buttons in ContentSettingsBubble for valid content settings. This prevents failed DCHECK e.g. for about:blank if you try to create popups from the console. BUG=244176 Committed: https://crrev.com/b98654eba7ccaf4bc7e554c0e11c412f9d72d690 Cr-Commit-Position: refs/heads/master@{#436285} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/b98654eba7ccaf4bc7e554c0e11c412f9d72d690 Cr-Commit-Position: refs/heads/master@{#436285}
Message was sent while issue was closed.
On 2016/12/05 14:33:00, commit-bot: I haz the power wrote: > Patchset 10 (id:??) landed as > https://crrev.com/b98654eba7ccaf4bc7e554c0e11c412f9d72d690 > Cr-Commit-Position: refs/heads/master@{#436285} Hey, this CL causes a crash. Repro steps: 1. Put a policy for Flash in /etc/chromium/policies/managed/flash_policy.json It just has to contain: { "DefaultPluginsSetting": 2, } 2. Go to http://pps-test-a.appspot.com/small_only.html 3. Click "Download Flash" 4. Click the Omnibox Icon. Stacktrace: [18855:18855:1206/170336.332191:FATAL:host_content_settings_map.cc(434)] Check failed: content_settings::SETTING_SOURCE_USER == info.source (3 vs. 1) #0 0x7fed52a4c75e base::debug::StackTrace::StackTrace() #1 0x7fed52a712bb logging::LogMessage::~LogMessage() #2 0x7fed53f1835f HostContentSettingsMap::GetNarrowestPatterns() #3 0x7fed53f181ab HostContentSettingsMap::CanSetNarrowestContentSetting() #4 0x7fed54857109 ContentSettingSingleRadioGroup::SetRadioGroup() #5 0x7fed548576dc ContentSettingPluginBubbleModel::ContentSettingPluginBubbleModel() #6 0x7fed5485c4dd ContentSettingBubbleModel::CreateContentSettingBubbleModel() I think you just have to delete the DCHECK_EQ in that file, but please double check. Thanks, Tommy
Message was sent while issue was closed.
Hi Tommy, thanks for noticing this. I removed the DCHECK and it fixes the issue. I created a CL for it: http://crrev.com/2558003002 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
