RESOLVED FIXED 127926
Add session support to WebPlatformStrategies
https://bugs.webkit.org/show_bug.cgi?id=127926
Summary Add session support to WebPlatformStrategies
Martin Hock
Reported 2014-01-30 11:28:11 PST
Add session support to WebPlatformStrategies. Also, ensure storage session exists when we call WebPage::setSessionID().
Attachments
patch (11.70 KB, patch)
2014-01-30 11:29 PST, Martin Hock
ap: review-
disallow calling ensurePrivateBrowsingSession with non-ephemeral session (14.67 KB, patch)
2014-01-30 12:58 PST, Martin Hock
no flags
right patch this time... (12.38 KB, patch)
2014-01-30 13:26 PST, Martin Hock
ap: review+
add assert per review comment (12.41 KB, patch)
2014-01-30 14:25 PST, Martin Hock
no flags
fix test failure (12.56 KB, patch)
2014-01-30 20:23 PST, Martin Hock
no flags
Martin Hock
Comment 1 2014-01-30 11:29:38 PST
Alexey Proskuryakov
Comment 2 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.
Martin Hock
Comment 3 2014-01-30 12:58:01 PST
Created attachment 222717 [details] disallow calling ensurePrivateBrowsingSession with non-ephemeral session
Martin Hock
Comment 4 2014-01-30 13:26:43 PST
Created attachment 222724 [details] right patch this time...
Alexey Proskuryakov
Comment 5 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.
Martin Hock
Comment 6 2014-01-30 14:25:19 PST
Created attachment 222736 [details] add assert per review comment
WebKit Commit Bot
Comment 7 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>
WebKit Commit Bot
Comment 8 2014-01-30 15:06:35 PST
All reviewed patches have been landed. Closing bug.
Andy Estes
Comment 9 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>.
WebKit Commit Bot
Comment 10 2014-01-30 19:08:34 PST
Re-opened since this is blocked by bug 127980
Martin Hock
Comment 11 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.
Alexey Proskuryakov
Comment 12 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.
Martin Hock
Comment 13 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.
Alexey Proskuryakov
Comment 14 2014-01-30 22:57:27 PST
> Here's what I think is happening: Agreed, this sounds plausible.
Martin Hock
Comment 15 2014-01-31 15:31:40 PST
All of the http, plugin, and storage tests run as expected or pass with the above patch.
Alexey Proskuryakov
Comment 16 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.
WebKit Commit Bot
Comment 17 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>
WebKit Commit Bot
Comment 18 2014-01-31 16:19:50 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.