Bug 95790 – Allow third-party storage blocking setting to change while a page is loaded

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Vicki Pfau]

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.

[Vicki Pfau]

Created attachment 163265 [details]
Patch

[Build Bot]

Comment on attachment 163265 [details]
Patch

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

[kov's GTK+ EWS bot]

Comment on attachment 163265 [details]
Patch

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

[Vicki Pfau]

Created attachment 163451 [details]
Patch

[Vicki Pfau]

Created attachment 163473 [details]
Patch

[Vicki Pfau]

Created attachment 163710 [details]
More tests

[Brady Eidson]

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]

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

Created attachment 163751 [details]
Patch

[Early Warning System Bot]

Comment on attachment 163751 [details]
Patch

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

[WebKit Review Bot]

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 on attachment 163751 [details]
Patch

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

[Vicki Pfau]

Created attachment 163937 [details]
Patch

[Brady Eidson]

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]

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

> > > 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]

Created attachment 164219 [details]
Patch

[Vicki Pfau]

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

[Vicki Pfau]

<rdar://problem/12303831>