Bug 130099

Summary: Unify private browsing with sessions
Product: WebKit Reporter: Martin Hock <mhock>
Component: WebKit2Assignee: Martin Hock <mhock>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, berto, bunhere, calvaris, cdumez, cgarcia, commit-queue, eric.carlson, esprehn+autocc, glenn, gns, gyuyoung.kim, japhet, jer.noble, kling, kondapallykalyan, mkwst, mrobinson, philipj, sergio, webkit-ews
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
style + gtk build fix
none
efl + gtk build fixes
none
efl fix + restore test
none
new window inherits session
none
gtk build fix
ap: review-
remove bug fix
none
remove changes to reference counting, change isUsingEphemeralSession to usesEphemeralSession
none
fix gtk build + address Page comments
none
address comments
ap: review+, ap: commit-queue-
fix gtk build
none
fix typo none

Description Martin Hock 2014-03-11 16:22:12 PDT
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.
Comment 1 Martin Hock 2014-03-11 17:45:48 PDT
Created attachment 226453 [details]
patch
Comment 2 Martin Hock 2014-03-11 18:20:52 PDT
Created attachment 226459 [details]
style + gtk build fix
Comment 3 Martin Hock 2014-03-12 15:24:34 PDT
Created attachment 226554 [details]
efl + gtk build fixes
Comment 4 Early Warning System Bot 2014-03-13 02:41:54 PDT
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
Comment 5 Martin Hock 2014-03-13 11:16:50 PDT
Created attachment 226601 [details]
efl fix + restore test
Comment 6 Martin Hock 2014-03-17 13:10:53 PDT
Created attachment 226948 [details]
new window inherits session
Comment 7 Martin Hock 2014-03-17 14:05:45 PDT
Created attachment 226960 [details]
gtk build fix
Comment 8 Alexey Proskuryakov 2014-03-20 10:25:47 PDT
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.
Comment 9 Martin Hock 2014-03-20 14:19:31 PDT
Created attachment 227334 [details]
remove bug fix
Comment 10 Alexey Proskuryakov 2014-03-20 15:15:04 PDT
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?
Comment 11 Martin Hock 2014-03-25 14:42:50 PDT
Created attachment 227800 [details]
remove changes to reference counting, change isUsingEphemeralSession to usesEphemeralSession
Comment 12 Martin Hock 2014-03-25 15:39:03 PDT
Created attachment 227810 [details]
fix gtk build + address Page comments
Comment 13 Martin Hock 2014-03-25 15:44:55 PDT
(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.
Comment 14 Alexey Proskuryakov 2014-03-26 10:16:45 PDT
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.
Comment 15 Martin Hock 2014-03-27 12:35:45 PDT
Created attachment 227965 [details]
address comments
Comment 16 Alexey Proskuryakov 2014-03-27 13:56:08 PDT
Comment on attachment 227965 [details]
address comments

Looks good to me. Please fix Gtk build.
Comment 17 Martin Hock 2014-03-27 14:02:37 PDT
Created attachment 227976 [details]
fix gtk build
Comment 18 Martin Hock 2014-03-27 14:10:04 PDT
Created attachment 227978 [details]
fix typo
Comment 19 WebKit Commit Bot 2014-04-02 13:08:00 PDT
Comment on attachment 227978 [details]
fix typo

Clearing flags on attachment: 227978

Committed r166661: <http://trac.webkit.org/changeset/166661>
Comment 20 WebKit Commit Bot 2014-04-02 13:08:09 PDT
All reviewed patches have been landed.  Closing bug.