Bug 95790

Created attachment 163265 [details] Patch

Comment on attachment 163265 [details] Patch Attachment 163265 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13818188

Comment on attachment 163265 [details] Patch Attachment 163265 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13825062

Created attachment 163451 [details] Patch

Created attachment 163473 [details] Patch

Created attachment 163710 [details] More tests

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.

(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.

Created attachment 163751 [details] Patch

Comment on attachment 163751 [details] Patch Attachment 163751 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13824928

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

Comment on attachment 163751 [details] Patch Attachment 163751 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13822999

Created attachment 163937 [details] Patch

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.

(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.

> > > 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"?

Created attachment 164219 [details] Patch

Committed r128653: <http://trac.webkit.org/changeset/128653>

<rdar://problem/12303831>