Bug 130099 – Unify private browsing with sessions

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Martin Hock]

Unify private browsing with sessions.

Remove private browsing setting from WebCore::Settings and replace use with sessions. Reference-count sessions in SessionTracker to properly dispose of unused sessions and to allow implementation of WebContext::somePageIsPrivateBrowsing (needed for WebIconDatabase's private browsing setting). Update corresponding WebKit2 session code.

[Martin Hock]

Created attachment 226453 [details]
patch

[Martin Hock]

Created attachment 226459 [details]
style + gtk build fix

[Martin Hock]

Created attachment 226554 [details]
efl + gtk build fixes

[Early Warning System Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Martin Hock]

Created attachment 226601 [details]
efl fix + restore test

[Martin Hock]

Created attachment 226948 [details]
new window inherits session

[Martin Hock]

Created attachment 226960 [details]
gtk build fix

[Alexey Proskuryakov]

Comment on attachment 226960 [details]
gtk build fix

The refactoring looks like a good idea. This patch also includes a bug fix, which is not the right way to fix it. As discussed in person, please fix it in a separate bug.

When session ID is set in createNewPage, it should be captured in WebPageCreationParameters, and passed back to WebProcess as a response to createNewPage.

[Martin Hock]

Created attachment 227334 [details]
remove bug fix

[Alexey Proskuryakov]

Comment on attachment 227334 [details]
remove bug fix

View in context: https://bugs.webkit.org/attachment.cgi?id=227334&action=review

> Source/WebCore/ChangeLog:8
> +        Unless otherwise noted, the following consists solely of mechanical changes to replace querying WebCore::Settings::privateBrowsingEnabled() with Page::sessionID().isEphemeral().

In addition to this, the patch fixes session leaks by introducing refcounting.

I'm very concerned about this, as I think that it's not possible to implement session refcounting for as long as we support old private browsing SPI. Having a large patch with many mechanical changes makes it very difficult to verify whether this works.

> Source/WebCore/html/HTMLMediaElement.cpp:1237
> +    bool privateMode = document().page() && document().page()->sessionID().isEphemeral();

SessionID still annoys me to no end. An ID can't be ephemeral, it's a concrete immutable unsigned number.

I think that for this patch, a reasonable approach would be changing page()->sessionID().isEphemeral() to page()->usesEphemeralSession(), and then we should fix the SessionID class.

> Source/WebCore/page/Page.cpp:1497
> +    ASSERT(m_sessionID.isValid());

I thought this change made it do that every page had a session. How can one be invalid?

> Source/WebCore/page/Page.cpp:1517
> +    if (sessionID != m_sessionID) {
> +        m_sessionID = sessionID;
> +
> +        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.
> +        auto views = pluginViews();
> +
> +        for (unsigned i = 0; i < views.size(); ++i)
> +            views[i]->privateBrowsingStateChanged(sessionID.isEphemeral());
> +    }

It might not matter in current use, but this code is not correct in general case - we probably don't want to call privateBrowsingStateDidChange when session changes, but private browsing state doesn't change.

Also,
- we prefer early return to nesting code;
- for loops should be C++ style.

> Tools/TestWebKitAPI/Tests/WebKit2/EphemeralSessionPushStateNoHistoryCallback.cpp:47
> +TEST(WebKit2, EphemeralSessionPushStateNoHistoryCallback)

Does this test for yet another bug that is fixed in this patch? Which bug is that?

[Martin Hock]

Created attachment 227800 [details]
remove changes to reference counting, change isUsingEphemeralSession to usesEphemeralSession

[Martin Hock]

Created attachment 227810 [details]
fix gtk build + address Page comments

[Martin Hock]

(In reply to comment #10)
> (From update of attachment 227334 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227334&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Unless otherwise noted, the following consists solely of mechanical changes to replace querying WebCore::Settings::privateBrowsingEnabled() with Page::sessionID().isEphemeral().
> 
> In addition to this, the patch fixes session leaks by introducing refcounting.
> 
> I'm very concerned about this, as I think that it's not possible to implement session refcounting for as long as we support old private browsing SPI. Having a large patch with many mechanical changes makes it very difficult to verify whether this works.

OK, I have gotten rid of the refcounting changes for this patch.

> > Source/WebCore/html/HTMLMediaElement.cpp:1237
> > +    bool privateMode = document().page() && document().page()->sessionID().isEphemeral();
> 
> SessionID still annoys me to no end. An ID can't be ephemeral, it's a concrete immutable unsigned number.
> 
> I think that for this patch, a reasonable approach would be changing page()->sessionID().isEphemeral() to page()->usesEphemeralSession(), and then we should fix the SessionID class.

Renamed (as well as renaming the parallel WebPage::isUsingEphemeralSession()).
 
> > Source/WebCore/page/Page.cpp:1497
> > +    ASSERT(m_sessionID.isValid());
> 
> I thought this change made it do that every page had a session. How can one be invalid?

You're right, this is unnecessary, so I've removed it.

> > Source/WebCore/page/Page.cpp:1517
> > +    if (sessionID != m_sessionID) {
> > +        m_sessionID = sessionID;
> > +
> > +        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.
> > +        auto views = pluginViews();
> > +
> > +        for (unsigned i = 0; i < views.size(); ++i)
> > +            views[i]->privateBrowsingStateChanged(sessionID.isEphemeral());
> > +    }
> 
> It might not matter in current use, but this code is not correct in general case - we probably don't want to call privateBrowsingStateDidChange when session changes, but private browsing state doesn't change.
> 
> Also,
> - we prefer early return to nesting code;
> - for loops should be C++ style.

I've updated the logic to try to implement this. I don't know how to improve the first "for" loop since it's doing a nonstandard iteration.

> > Tools/TestWebKitAPI/Tests/WebKit2/EphemeralSessionPushStateNoHistoryCallback.cpp:47
> > +TEST(WebKit2, EphemeralSessionPushStateNoHistoryCallback)
> 
> Does this test for yet another bug that is fixed in this patch? Which bug is that?

This test simply reimplements PrivateBrowsingPushStateNoHistoryCallback for sessions. It didn't pass before these changes because we needed to unify private browsing with sessions, which is what this bug is fixing.

[Alexey Proskuryakov]

Comment on attachment 227810 [details]
fix gtk build + address Page comments

View in context: https://bugs.webkit.org/attachment.cgi?id=227810&action=review

The changes in WebPage::updatePreferences look like they break disabling private browsing. Was that intentional?

> Source/WebCore/page/Page.cpp:1513
> +    bool stateChanged = (sessionID.isEphemeral() != m_sessionID.isEphemeral());

I'd name this variable "privateBrowsingStateChanged". There is no implicit "state" that we are dealing with here.

> Source/WebCore/page/Page.cpp:1526
> +    for (auto &view : pluginViews())

Space position: should be "auto&", not "auto &".

> Source/WebCore/page/Settings.cpp:429
>  void Settings::setPrivateBrowsingEnabled(bool privateBrowsingEnabled)

Does this need to remain a function on Settings object? It's misleading at call sites, because this is no longer a setting.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2489
> -    if (m_page->isSessionIDSet())
> -        settings.setPrivateBrowsingEnabled(m_page->sessionID().isEphemeral());
> -    else
> -        settings.setPrivateBrowsingEnabled(store.getBoolValueForKey(WebPreferencesKey::privateBrowsingEnabledKey()));
> +    if (store.getBoolValueForKey(WebPreferencesKey::privateBrowsingEnabledKey()) && !m_page->usesEphemeralSession())
> +        m_page->setSessionID(SessionID::legacyPrivateSessionID());

This code used to disable private browsing when the preference was unset, but no longer does that. This looks like a bug.

[Martin Hock]

Created attachment 227965 [details]
address comments

[Alexey Proskuryakov]

Comment on attachment 227965 [details]
address comments

Looks good to me. Please fix Gtk build.

[Martin Hock]

Created attachment 227976 [details]
fix gtk build

[Martin Hock]

Created attachment 227978 [details]
fix typo

[WebKit Commit Bot]

Comment on attachment 227978 [details]
fix typo

Clearing flags on attachment: 227978

Committed r166661: <http://trac.webkit.org/changeset/166661>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.