Bug 33180 – [Qt] Support private browsing mode in plugins

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Robert Hogan]

Add support for NPNVprivateModeBool property in plugins.

See also: https://developer.mozilla.org/En/Supporting_private_browsing_in_plugins

The NPNVprivateModeBool property is supported as scriptable property privateBrowsingEnabled in the test WebKit plugin. The Mac platform also supports a cachedPrivateBrowsingEnabled
property implemented in the test plugin. This allows the Layout test plugins/private-browsing-mode.html to retrieve the previous value of NPNVprivateModeBool in the test plugin. Due to the platform-specific overhead required to support this bespoke property it is not implemented as part of this patch, instead a new test, plugins/private-browsing-mode-2.html, is added to ensure that setting and resetting privateBrowsingEnabled works as expected.

[Robert Hogan]

Created attachment 45828 [details]
Patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45828 [details] without any errors.

[Kenneth Rohde Christiansen]

Comment on attachment 45828 [details]
Patch

LGTM, but it seems that something went wrong with GTK, please have a look at that.

Also, where is that successfullyParsed = true; used?

[Simon Hausmann]

Comment on attachment 45828 [details]
Patch

> --- a/LayoutTests/platform/gtk/Skipped
> +++ b/LayoutTests/platform/gtk/Skipped
> @@ -3651,6 +3651,11 @@ plugins/plugin-javascript-access.html
>  # https://bugs.webkit.org/show_bug.cgi?id=30561
>  plugins/private-browsing-mode.html
>  
> +# https://bugs.webkit.org/show_bug.cgi?id=30561
> +plugins/private-browsing-mode.html

It seems strange to see these two lines being added that exist alread three lines further up :)

> +# https://bugs.webkit.org/show_bug.cgi?id=33180
> +plugins/private-browsing-mode-2.html
> +
>  #   Tests generating new results
>  plugins/embed-attributes-style.html
>  plugins/netscape-dom-access.html


> +Tests that NPNVprivateModeBool is supported by the WebKit plugin view. This test is for WebKit platforms that wish to support NPNVprivateModeBool but do not wish to implement the preference change listener required to support a cachedPrivateBrowsingEnabled property similar to the one provided by Safari and tested for in private-browsing-mode.html

I don't understand this part. It seems trivial to implement in DumpRenderTree/unix/TestNetscapePlugin/TestNetscapePlugin.cpp. Why not do it there instead of this extra test?

In fact, it seems like a bug right now that the cachedPrivateBrowsingEnabled variable isn't initialized in that file, where as it is initialized in
TestNetscapePlugIn.subproj/main.cpp. Could that be why the test was failing for you?

> +
> +    case NPNVprivateModeBool: {
> +        Page* page = m_parentFrame->page();
> +        if (!page)
> +            return NPERR_GENERIC_ERROR;
> +         *((NPBool*)value) = (!page->settings() || page->settings()->privateBrowsingEnabled())? true : false;
> +         return NPERR_NO_ERROR;
> +    }

I would leave out the ? true : false; part and the extra parentheses, it's redundant.

Otherwise the patch looks great, and certainly long-hanging fruit for the other PluginView implementations, too.

[Robert Hogan]

> 
> > +Tests that NPNVprivateModeBool is supported by the WebKit plugin view. This test is for WebKit platforms that wish to support NPNVprivateModeBool but do not wish to implement the preference change listener required to support a cachedPrivateBrowsingEnabled property similar to the one provided by Safari and tested for in private-browsing-mode.html
> 
> I don't understand this part. It seems trivial to implement in
> DumpRenderTree/unix/TestNetscapePlugin/TestNetscapePlugin.cpp. Why not do it
> there instead of this extra test?
> 
> In fact, it seems like a bug right now that the cachedPrivateBrowsingEnabled
> variable isn't initialized in that file, where as it is initialized in
> TestNetscapePlugIn.subproj/main.cpp. Could that be why the test was failing for
> you?

The mac port listens for changes to the users webkit preferences and halts any netscape plugins currently running if pluginsEnabled has changed to false. Setting the 'cachedPrivateBrowsingMode' value rides on the coat-tails of this feature - which Qt doesn't have, and possibly should. I don't think it's trivial to implement though. So will leave it out of this patch if that's ok?

[Robert Hogan]

Created attachment 47448 [details]
Updated Patch

Fixed style issues. Simon - if you agree with my response to your comments this should be ok to r+. I could look at halting plugins when pluginsEnabled gets set to false separately - or maybe address that first and make it a blocker for this bug.

[Simon Hausmann]

(In reply to comment #5)
> > 
> > > +Tests that NPNVprivateModeBool is supported by the WebKit plugin view. This test is for WebKit platforms that wish to support NPNVprivateModeBool but do not wish to implement the preference change listener required to support a cachedPrivateBrowsingEnabled property similar to the one provided by Safari and tested for in private-browsing-mode.html
> > 
> > I don't understand this part. It seems trivial to implement in
> > DumpRenderTree/unix/TestNetscapePlugin/TestNetscapePlugin.cpp. Why not do it
> > there instead of this extra test?
> > 
> > In fact, it seems like a bug right now that the cachedPrivateBrowsingEnabled
> > variable isn't initialized in that file, where as it is initialized in
> > TestNetscapePlugIn.subproj/main.cpp. Could that be why the test was failing for
> > you?
> 
> The mac port listens for changes to the users webkit preferences and halts any
> netscape plugins currently running if pluginsEnabled has changed to false.
> Setting the 'cachedPrivateBrowsingMode' value rides on the coat-tails of this
> feature - which Qt doesn't have, and possibly should. I don't think it's
> trivial to implement though. So will leave it out of this patch if that's ok?

I think I understand now, but I don't see that the mac port stops the plugin. Instead looking at - (void)privateBrowsingModeDidChange in WebNetscapePluginView.mm I can see that the private browsing mode value is _propagated_ to the plugin (setvalue), and _that_ is what the other part of the private-browsing-mode.html tests.

Your patch implements the part that allows the plugin to query the browser if private browsing is enabled or not, and your added test verifies that.

The Mac port also implements _notifying_ the plugin if the private browsing mode changes, which appears to be an important feature to have.

I agree with you that this is okay to implement in a second patch. I guess once the setting in WebCore changes, the page should iterate through its plugins and notify them via setvalue calls.

[Simon Hausmann]

Comment on attachment 47448 [details]
Updated Patch


> +    case NPNVprivateModeBool: {
> +        Page* page = m_parentFrame->page();
> +        if (!page)
> +            return NPERR_GENERIC_ERROR;
> +        *((NPBool*)value) = !page->settings() || page->settings()->privateBrowsingEnabled();
> +        return NPERR_NO_ERROR;
> +    }

r=me, but when landing you may want to change the above to return NPERR_GENERIC_ERROR if !page->settings() and only return the real value to the plugin if we have a settings object.

[Robert Hogan]

(In reply to comment #7)
> (In reply to comment #5)
> > > 
> > > > +Tests that NPNVprivateModeBool is supported by the WebKit plugin view. This test is for WebKit platforms that wish to support NPNVprivateModeBool but do not wish to implement the preference change listener required to support a cachedPrivateBrowsingEnabled property similar to the one provided by Safari and tested for in private-browsing-mode.html
> > > 
> > > I don't understand this part. It seems trivial to implement in
> > > DumpRenderTree/unix/TestNetscapePlugin/TestNetscapePlugin.cpp. Why not do it
> > > there instead of this extra test?
> > > 
> > > In fact, it seems like a bug right now that the cachedPrivateBrowsingEnabled
> > > variable isn't initialized in that file, where as it is initialized in
> > > TestNetscapePlugIn.subproj/main.cpp. Could that be why the test was failing for
> > > you?
> > 
> > The mac port listens for changes to the users webkit preferences and halts any
> > netscape plugins currently running if pluginsEnabled has changed to false.
> > Setting the 'cachedPrivateBrowsingMode' value rides on the coat-tails of this
> > feature - which Qt doesn't have, and possibly should. I don't think it's
> > trivial to implement though. So will leave it out of this patch if that's ok?
> 
> I think I understand now, but I don't see that the mac port stops the plugin.

I came to the conclusion from reading in WebBaseNetscapePluginView.mm:

- (void)preferencesHaveChanged:(NSNotification *)notification
{
    WebPreferences *preferences = [[self webView] preferences];

    if ([notification object] != preferences)
        return;
    
    BOOL arePlugInsEnabled = [preferences arePlugInsEnabled];
    if (_isStarted != arePlugInsEnabled) {
        if (arePlugInsEnabled) {
            if ([self currentWindow]) {
                [self start];
            }
        } else {
            [self stop];
            [self invalidatePluginContentRect:[self bounds]];
        }
    }
    
    BOOL isPrivateBrowsingEnabled = [preferences privateBrowsingEnabled];
    if (isPrivateBrowsingEnabled != _isPrivateBrowsingEnabled) {
        _isPrivateBrowsingEnabled = isPrivateBrowsingEnabled;
        [self privateBrowsingModeDidChange];
    }
}


> Instead looking at - (void)privateBrowsingModeDidChange in
> WebNetscapePluginView.mm I can see that the private browsing mode value is
> _propagated_ to the plugin (setvalue), and _that_ is what the other part of the
> private-browsing-mode.html tests.
> 
> Your patch implements the part that allows the plugin to query the browser if
> private browsing is enabled or not, and your added test verifies that.
> 
> The Mac port also implements _notifying_ the plugin if the private browsing
> mode changes, which appears to be an important feature to have.
> 

Ah OK, it actually sets the value of NPNVprivateModeBool there and then:

- (void)privateBrowsingModeDidChange
{
    if (!_isStarted)
        return;
    
    NPBool value = _isPrivateBrowsingEnabled;

    [self willCallPlugInFunction];
    {
        JSC::JSLock::DropAllLocks dropAllLocks(JSC::SilenceAssertionsOnly);
        if ([_pluginPackage.get() pluginFuncs]->setvalue)
            [_pluginPackage.get() pluginFuncs]->setvalue(plugin, NPNVprivateModeBool, &value);
    }
    [self didCallPlugInFunction];
}

I was under the misapprehension that any time a plugin wants to check the value of NPNVprivateModeBool it would have to perform a getvalue() but it will actually receive the setvalue event from the browser and act appropriately. Got it now.

> I agree with you that this is okay to implement in a second patch. I guess once
> the setting in WebCore changes, the page should iterate through its plugins and
> notify them via setvalue calls.

[Robert Hogan]

(In reply to comment #8)
> (From update of attachment 47448 [details])
> 
> > +    case NPNVprivateModeBool: {
> > +        Page* page = m_parentFrame->page();
> > +        if (!page)
> > +            return NPERR_GENERIC_ERROR;
> > +        *((NPBool*)value) = !page->settings() || page->settings()->privateBrowsingEnabled();
> > +        return NPERR_NO_ERROR;
> > +    }
> 
> r=me, but when landing you may want to change the above to return
> NPERR_GENERIC_ERROR if !page->settings() and only return the real value to the
> plugin if we have a settings object.

Elsewhere in the code the default behaviour for security-sensitive settings is to assume that if !page->settings() then the setting is true.

e.g. in WebCore/loader/DocLoader.cpp:

    Settings* settings = frame()->settings();
    if (!settings || settings->privateBrowsingEnabled())
        return;

[Robert Hogan]

Manually committed r55358: http://trac.webkit.org/changeset/55358

[Dimitri Glazkov (Google)]

If this is a Qt-specific font, maybe it should live under platform/qt?

[Robert Hogan]

(In reply to comment #12)
> If this is a Qt-specific font, maybe it should live under platform/qt?

Hi Dimitri,

Is this comment on the correct bug? If it's regarding the additions to the skiplists, I've added plugins/private-browsing-mode-2.html to any platform that already skips plugins/private-browsing-mode.html since it likely as not will fail it.

[Dimitri Glazkov (Google)]

(In reply to comment #13)
> (In reply to comment #12)
> > If this is a Qt-specific font, maybe it should live under platform/qt?
> 
> Hi Dimitri,
> 
> Is this comment on the correct bug? If it's regarding the additions to the
> skiplists, I've added plugins/private-browsing-mode-2.html to any platform that
> already skips plugins/private-browsing-mode.html since it likely as not will
> fail it.

Sorry, comment fail on my part :)

What I meant to say is that if this is a Qt-specific test, you could've put it in platform/qt and this way there would've been no need to update Skipped lists for other platforms.

Just a suggestion.

[Mark Rowe (bdash)]

I believe the functionality that you described implementing in a second patch was what I just landed as part of bug 35576.  Qt should be able to unskip the original private browsing plug-in patch after that change.

[Tor Arne Vestbø]

Please follow the QtWebKit bug reporting guidelines when reporting bugs.

See http://trac.webkit.org/wiki/QtWebKitBugs

Specifically:

  - The 'QtWebKit' component should only be used for bugs/features in the
    public QtWebKit API layer, not to signify that the bug is specific to
    the Qt port of WebKit

      http://trac.webkit.org/wiki/QtWebKitBugs#Component

  - Add the keyword 'Qt' to signal that it's a Qt-related bug

      http://trac.webkit.org/wiki/QtWebKitBugs#Keywords