Bug 127926 - Add session support to WebPlatformStrategies
Summary: Add session support to WebPlatformStrategies
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Martin Hock
URL:
Keywords:
Depends on: 127980
Blocks:
  Show dependency treegraph
 
Reported: 2014-01-30 11:28 PST by Martin Hock
Modified: 2014-01-31 16:19 PST (History)
4 users (show)

See Also:


Attachments
patch (11.70 KB, patch)
2014-01-30 11:29 PST, Martin Hock
ap: review-
Details | Formatted Diff | Diff
disallow calling ensurePrivateBrowsingSession with non-ephemeral session (14.67 KB, patch)
2014-01-30 12:58 PST, Martin Hock
no flags Details | Formatted Diff | Diff
right patch this time... (12.38 KB, patch)
2014-01-30 13:26 PST, Martin Hock
ap: review+
Details | Formatted Diff | Diff
add assert per review comment (12.41 KB, patch)
2014-01-30 14:25 PST, Martin Hock
no flags Details | Formatted Diff | Diff
fix test failure (12.56 KB, patch)
2014-01-30 20:23 PST, Martin Hock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Hock 2014-01-30 11:28:11 PST
Add session support to WebPlatformStrategies.  Also, ensure storage session exists when we call WebPage::setSessionID().
Comment 1 Martin Hock 2014-01-30 11:29:38 PST
Created attachment 222698 [details]
patch
Comment 2 Alexey Proskuryakov 2014-01-30 11:53:41 PST
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.
Comment 3 Martin Hock 2014-01-30 12:58:01 PST
Created attachment 222717 [details]
disallow calling ensurePrivateBrowsingSession with non-ephemeral session
Comment 4 Martin Hock 2014-01-30 13:26:43 PST
Created attachment 222724 [details]
right patch this time...
Comment 5 Alexey Proskuryakov 2014-01-30 14:14:56 PST
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.
Comment 6 Martin Hock 2014-01-30 14:25:19 PST
Created attachment 222736 [details]
add assert per review comment
Comment 7 WebKit Commit Bot 2014-01-30 15:06:32 PST
Comment on attachment 222736 [details]
add assert per review comment

Clearing flags on attachment: 222736

Committed r163125: <http://trac.webkit.org/changeset/163125>
Comment 8 WebKit Commit Bot 2014-01-30 15:06:35 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Andy Estes 2014-01-30 18:57:50 PST
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>.
Comment 10 WebKit Commit Bot 2014-01-30 19:08:34 PST
Re-opened since this is blocked by bug 127980
Comment 11 Martin Hock 2014-01-30 20:23:27 PST
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.
Comment 12 Alexey Proskuryakov 2014-01-30 20:45:10 PST
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.
Comment 13 Martin Hock 2014-01-30 22:41:20 PST
(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.
Comment 14 Alexey Proskuryakov 2014-01-30 22:57:27 PST
> Here's what I think is happening:

Agreed, this sounds plausible.
Comment 15 Martin Hock 2014-01-31 15:31:40 PST
All of the http, plugin, and storage tests run as expected or pass with the above patch.
Comment 16 Alexey Proskuryakov 2014-01-31 15:49:28 PST
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.
Comment 17 WebKit Commit Bot 2014-01-31 16:19:47 PST
Comment on attachment 222789 [details]
fix test failure

Clearing flags on attachment: 222789

Committed r163217: <http://trac.webkit.org/changeset/163217>
Comment 18 WebKit Commit Bot 2014-01-31 16:19:50 PST
All reviewed patches have been landed.  Closing bug.