Bug 127926 – Add session support to WebPlatformStrategies

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

[Martin Hock]

Add session support to WebPlatformStrategies.  Also, ensure storage session exists when we call WebPage::setSessionID().

[Martin Hock]

Created attachment 222698 [details]
patch

[Alexey Proskuryakov]

Comment on attachment 222698 [details]
patch

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

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm:48
>  void WebFrameNetworkingContext::ensurePrivateBrowsingSession(uint64_t sessionID)
>  {
> -    if (SessionTracker::session(sessionID))
> +    if (sessionID == SessionTracker::defaultSessionID || SessionTracker::session(sessionID))
>          return;

I don't think that this is quite right. The function name means that it can be only invoked with private browsing sessions, so having a check for whether the session is private inside it is too late.

[Martin Hock]

Created attachment 222717 [details]
disallow calling ensurePrivateBrowsingSession with non-ephemeral session

[Martin Hock]

Created attachment 222724 [details]
right patch this time...

[Alexey Proskuryakov]

Comment on attachment 222724 [details]
right patch this time...

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

> Source/WebKit2/Shared/SessionTracker.cpp:49
> +    static NeverDestroyed<HashMap<const NetworkStorageSession*, uint64_t>> map;

I would put an ASSERT(isMainThread()) here too.

[Martin Hock]

Created attachment 222736 [details]
add assert per review comment

[WebKit Commit Bot]

Comment on attachment 222736 [details]
add assert per review comment

Clearing flags on attachment: 222736

Committed r163125: <http://trac.webkit.org/changeset/163125>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andy Estes]

This caused 9 layout tests to start asserting in Debug builds on the Mountain Lion bot:

Regressions: Unexpected crashes (9)
  http/tests/security/cross-frame-access-selection.html [ Crash ]
  http/tests/security/cross-origin-plugin-private-browsing-toggled.html [ Crash ]
  http/tests/security/dataURL/xss-DENIED-from-data-url-in-foreign-domain-subframe.html [ Crash ]
  http/tests/security/storage-blocking-loosened-shared-worker.html [ Crash ]
  http/tests/security/storage-blocking-strengthened-shared-worker.html [ Crash ]
  plugins/private-browsing-mode.html [ Crash ]
  storage/domstorage/localstorage/string-conversion.html [ Crash ]
  storage/domstorage/sessionstorage/string-conversion.html [ Crash ]
  storage/websql/read-transactions-running-concurrently.html [ Crash ]

See <http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK2%20(Tests)/builds/15691>.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 127980

[Martin Hock]

Created attachment 222789 [details]
fix test failure

The test failed because we attempt to remove a session that doesn't exist, because the test manually sets private browsing to false before a private browsing session exists.  Check to see the session exists before removing it.

[Alexey Proskuryakov]

Comment on attachment 222789 [details]
fix test failure

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

You can test these on Mavericks if you comment out private browsing tests in LayoutTests/platform/mac-wk2/TestExpectations, and also comment out these lines in WebKitTestRunner:

    WKContextSetUsesNetworkProcess(m_context.get(), true);
    WKContextSetProcessModel(m_context.get(), kWKProcessModelMultipleSecondaryProcesses);

> Source/WebKit2/Shared/SessionTracker.cpp:100
> +    if (staticSessionMap().contains(sessionID)) {
> +        storageSessionToID().remove(session(sessionID));
> +        staticSessionMap().remove(sessionID);
> +    }

I don't think that this is right. remove() doesn't crash when removing a non-existent key. The crashes that happen mean that sessionID is 0, and contains() will crash in the same manner.

The right fix will be to make sure that InjectedBundle::setPrivateBrowsingEnabled() passes a correct (legacy) session ID.

[Martin Hock]

(In reply to comment #12)
> (From update of attachment 222789 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222789&action=review
> 
> You can test these on Mavericks if you comment out private browsing tests in LayoutTests/platform/mac-wk2/TestExpectations, and also comment out these lines in WebKitTestRunner:
> 
>     WKContextSetUsesNetworkProcess(m_context.get(), true);
>     WKContextSetProcessModel(m_context.get(), kWKProcessModelMultipleSecondaryProcesses);
> 

Thanks for the tip!

> > Source/WebKit2/Shared/SessionTracker.cpp:100
> > +    if (staticSessionMap().contains(sessionID)) {
> > +        storageSessionToID().remove(session(sessionID));
> > +        staticSessionMap().remove(sessionID);
> > +    }
> 
> I don't think that this is right. remove() doesn't crash when removing a non-existent key. The crashes that happen mean that sessionID is 0, and contains() will crash in the same manner.
> 
> The right fix will be to make sure that InjectedBundle::setPrivateBrowsingEnabled() passes a correct (legacy) session ID.

I'll take a look tomorrow, but InjectedBundle::setPrivateBrowsingEnabled() does appear to pass the legacy private session ID.  Here's what I think is happening: the test calls setPrivateBrowsingEnabled(false) which in turn calls SessionTracker::destroySession(legacyPrivateSessionID).  Then, session(sessionID) is returning nullptr because there is no session with the legacy private ID (because it hasn't been created yet), and that nullptr is causing storageSessionToID().remove(nullptr) to fail the assert.

[Alexey Proskuryakov]

> Here's what I think is happening:

Agreed, this sounds plausible.

[Martin Hock]

All of the http, plugin, and storage tests run as expected or pass with the above patch.

[Alexey Proskuryakov]

Comment on attachment 222789 [details]
fix test failure

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

r=me

> Source/WebKit2/Shared/SessionTracker.cpp:99
> +    if (staticSessionMap().contains(sessionID)) {
> +        storageSessionToID().remove(session(sessionID));
> +        staticSessionMap().remove(sessionID);

It's not great that we look up sessionID in staticSessionMap three times here. It can be easily avoided by using find().

But this is not hot code at all.

[WebKit Commit Bot]

Comment on attachment 222789 [details]
fix test failure

Clearing flags on attachment: 222789

Committed r163217: <http://trac.webkit.org/changeset/163217>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.