RESOLVED FIXED 95790
Allow third-party storage blocking setting to change while a page is loaded
https://bugs.webkit.org/show_bug.cgi?id=95790
Summary Allow third-party storage blocking setting to change while a page is loaded
Vicki Pfau
Reported 2012-09-04 15:43:44 PDT
The third-party storage blocking setting should affect pages that are already loaded, blocking (or unblocking, depending on the change) new interactions with third-party storage mechanisms and not just taking effect when a page is reloaded.
Attachments
Patch (43.25 KB, patch)
2012-09-10 19:09 PDT, Vicki Pfau
no flags
Patch (45.56 KB, patch)
2012-09-11 14:58 PDT, Vicki Pfau
no flags
Patch (45.57 KB, patch)
2012-09-11 16:18 PDT, Vicki Pfau
no flags
More tests (50.51 KB, patch)
2012-09-12 15:04 PDT, Vicki Pfau
no flags
Patch (51.64 KB, patch)
2012-09-12 17:59 PDT, Vicki Pfau
no flags
Patch (52.40 KB, patch)
2012-09-13 12:40 PDT, Vicki Pfau
no flags
Patch (54.00 KB, patch)
2012-09-14 13:42 PDT, Vicki Pfau
beidson: review+
Vicki Pfau
Comment 1 2012-09-10 19:09:56 PDT
Build Bot
Comment 2 2012-09-10 19:40:46 PDT
kov's GTK+ EWS bot
Comment 3 2012-09-10 20:58:18 PDT
Vicki Pfau
Comment 4 2012-09-11 14:58:34 PDT
Vicki Pfau
Comment 5 2012-09-11 16:18:37 PDT
Vicki Pfau
Comment 6 2012-09-12 15:04:17 PDT
Created attachment 163710 [details] More tests
Brady Eidson
Comment 7 2012-09-12 15:30:24 PDT
Comment on attachment 163710 [details] More tests View in context: https://bugs.webkit.org/attachment.cgi?id=163710&action=review The comment about asynchronous init doesn't necessarily need to be resolved now, but it does need a bug filed about it. My other comments are more pressing to me. > Source/WebKit2/PluginProcess/PluginControllerProxy.cpp:573 > + m_storageBlockingPolicy = static_cast<SecurityOrigin::StorageBlockingPolicy>(storageBlockingPolicy); > + > + if (!m_isPrivateBrowsingEnabled) > + m_plugin->storageBlockingStateChanged(m_storageBlockingPolicy); > +} > + We only update the storage blocking setting on each plug-in while private browsing is disabled? So what if the storage blocking setting changes while private browsing is enabled, then the user later disables private browsing? Do the plug-in instances get told about the storage blocking setting then? It feels to me like we should always pipe this through. > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:959 > +void NetscapePlugin::storageBlockingStateChanged(SecurityOrigin::StorageBlockingPolicy storageBlockingPolicy) > +{ > + privateBrowsingStateChanged(storageBlockingPolicy != SecurityOrigin::AllowAllStorage); > +} I think we should always pass through a call to storageBlockingStateChanged and the NetscapePlugin remembers both the current private browsing and storage blocking settings. Whenever *either* of them changes we calculate whether or not we need to update the plugin code or not. > Source/WebKit2/WebProcess/Plugins/PluginView.cpp:591 > +void PluginView::storageBlockingStateChanged(WebCore::SecurityOrigin::StorageBlockingPolicy storageBlockingPolicy) > +{ > + // The plug-in can be null here if it failed to initialize. > + if (!m_isInitialized || !m_plugin) > + return; > + > + m_plugin->storageBlockingStateChanged(storageBlockingPolicy); > +} In the brave new world of asynchronous plug-in initialization, we probably need to store off the new StorageBlockingPolicy value (as well as any new private browsing settings) so we can update the plug-in when it does actually finish initializing.
Vicki Pfau
Comment 8 2012-09-12 15:36:55 PDT
(In reply to comment #7) > (From update of attachment 163710 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163710&action=review > > The comment about asynchronous init doesn't necessarily need to be resolved now, but it does need a bug filed about it. > > My other comments are more pressing to me. > > > Source/WebKit2/PluginProcess/PluginControllerProxy.cpp:573 > > + m_storageBlockingPolicy = static_cast<SecurityOrigin::StorageBlockingPolicy>(storageBlockingPolicy); > > + > > + if (!m_isPrivateBrowsingEnabled) > > + m_plugin->storageBlockingStateChanged(m_storageBlockingPolicy); > > +} > > + > > We only update the storage blocking setting on each plug-in while private browsing is disabled? > > So what if the storage blocking setting changes while private browsing is enabled, then the user later disables private browsing? Do the plug-in instances get told about the storage blocking setting then? > > It feels to me like we should always pipe this through. I would agree, but that would require the state of private browsing and storage blocking to be stored in each plugin. As you say later, that probably should be done anyway, but I was trying to approach this without having to do that. As for the case you mentioned, I believe the test storage-blocking-strengthened-private-browsing-plugin.html covers that, and it passes as would be expected: - Turn private browsing on. - Turn storage blocking all the way up. - Turn private browsing off. - The plugin still stays in its faux-private browsing mode, as desired. Perhaps this wasn't the case you were talking about? But I can modify the patch to store the values of the states as needed. > > > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:959 > > +void NetscapePlugin::storageBlockingStateChanged(SecurityOrigin::StorageBlockingPolicy storageBlockingPolicy) > > +{ > > + privateBrowsingStateChanged(storageBlockingPolicy != SecurityOrigin::AllowAllStorage); > > +} > > I think we should always pass through a call to storageBlockingStateChanged and the NetscapePlugin remembers both the current private browsing and storage blocking settings. Whenever *either* of them changes we calculate whether or not we need to update the plugin code or not. > > > Source/WebKit2/WebProcess/Plugins/PluginView.cpp:591 > > +void PluginView::storageBlockingStateChanged(WebCore::SecurityOrigin::StorageBlockingPolicy storageBlockingPolicy) > > +{ > > + // The plug-in can be null here if it failed to initialize. > > + if (!m_isInitialized || !m_plugin) > > + return; > > + > > + m_plugin->storageBlockingStateChanged(storageBlockingPolicy); > > +} > > In the brave new world of asynchronous plug-in initialization, we probably need to store off the new StorageBlockingPolicy value (as well as any new private browsing settings) so we can update the plug-in when it does actually finish initializing.
Vicki Pfau
Comment 9 2012-09-12 17:59:24 PDT
Early Warning System Bot
Comment 10 2012-09-12 18:46:46 PDT
WebKit Review Bot
Comment 11 2012-09-12 20:15:20 PDT
Comment on attachment 163751 [details] Patch Attachment 163751 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13827829 New failing tests: http/tests/security/storage-blocking-loosened-private-browsing-plugin.html
Build Bot
Comment 12 2012-09-12 20:42:23 PDT
Vicki Pfau
Comment 13 2012-09-13 12:40:12 PDT
Brady Eidson
Comment 14 2012-09-13 17:29:58 PDT
Comment on attachment 163937 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163937&action=review I like this one *MUCH* better. I have a few comments. r+, but please address them before landing. > Source/WebCore/page/Page.cpp:1009 > +void Page::storageBlockingStateChanged() I think this method should get the same comment privateBrowsingStateChanged has. > Source/WebCore/page/Page.cpp:1031 > +void Page::privateBrowsingStateChanged() > +{ > + bool privateBrowsingEnabled = m_settings->privateBrowsingEnabled(); > + > + for (Frame* frame = mainFrame(); frame; frame = frame->tree()->traverseNext()) > + frame->document()->privateBrowsingStateDidChange(); > + > + // Collect the PluginViews in to a vector to ensure that action the plug-in takes > + // from below privateBrowsingStateChanged does not affect their lifetime. > + Vector<RefPtr<PluginViewBase>, 32> pluginViewBases; > + collectPluginViews(pluginViewBases); I think this comment is useful, and its usefulness also applies to the new storageBlockingStateChanged() method > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:972 > +void NetscapePlugin::enactPrivateBrowsingState() "enact" sounds like it is always enabling private browsing. I'd rather see "updatePrivateBrowsingState" since it both enables and disables it.
Vicki Pfau
Comment 15 2012-09-13 17:35:01 PDT
(In reply to comment #14) > (From update of attachment 163937 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163937&action=review > > I like this one *MUCH* better. > > I have a few comments. r+, but please address them before landing. > > > Source/WebCore/page/Page.cpp:1009 > > +void Page::storageBlockingStateChanged() > > I think this method should get the same comment privateBrowsingStateChanged has. > > > Source/WebCore/page/Page.cpp:1031 > > +void Page::privateBrowsingStateChanged() > > +{ > > + bool privateBrowsingEnabled = m_settings->privateBrowsingEnabled(); > > + > > + for (Frame* frame = mainFrame(); frame; frame = frame->tree()->traverseNext()) > > + frame->document()->privateBrowsingStateDidChange(); > > + > > + // Collect the PluginViews in to a vector to ensure that action the plug-in takes > > + // from below privateBrowsingStateChanged does not affect their lifetime. > > + Vector<RefPtr<PluginViewBase>, 32> pluginViewBases; > > + collectPluginViews(pluginViewBases); > > I think this comment is useful, and its usefulness also applies to the new storageBlockingStateChanged() method Alright, I can add that comment back and update its wording. > > > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:972 > > +void NetscapePlugin::enactPrivateBrowsingState() > > "enact" sounds like it is always enabling private browsing. > > I'd rather see "updatePrivateBrowsingState" since it both enables and disables it. My problem here is that there's already a setPrivateBrowsingState function that is slightly different than this. I'm wondering if there's a better name, like updatePluginPrivateBrowsingState or updateInternalPrivateBrowsingState that makes it less ambiguous which one does what.
Brady Eidson
Comment 16 2012-09-13 17:58:31 PDT
> > > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:972 > > > +void NetscapePlugin::enactPrivateBrowsingState() > > > > "enact" sounds like it is always enabling private browsing. > > > > I'd rather see "updatePrivateBrowsingState" since it both enables and disables it. > > My problem here is that there's already a setPrivateBrowsingState function that is slightly different than this. I'm wondering if there's a better name, like updatePluginPrivateBrowsingState or updateInternalPrivateBrowsingState that makes it less ambiguous which one does what. This is about a concept called "PrivateMode" for Netscape plug-ins - How about we call it "updateNetscapePrivateMode"?
Vicki Pfau
Comment 17 2012-09-14 13:42:43 PDT
Vicki Pfau
Comment 18 2012-09-14 14:31:14 PDT
Vicki Pfau
Comment 19 2012-09-14 14:36:10 PDT
Note You need to log in before you can comment on or make changes to this bug.