Add session support to WebPlatformStrategies. Also, ensure storage session exists when we call WebPage::setSessionID().
Created attachment 222698 [details] patch
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.
Created attachment 222717 [details] disallow calling ensurePrivateBrowsingSession with non-ephemeral session
Created attachment 222724 [details] right patch this time...
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.
Created attachment 222736 [details] add assert per review comment
Comment on attachment 222736 [details] add assert per review comment Clearing flags on attachment: 222736 Committed r163125: <http://trac.webkit.org/changeset/163125>
All reviewed patches have been landed. Closing bug.
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>.
Re-opened since this is blocked by bug 127980
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 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.
(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.
> Here's what I think is happening: Agreed, this sounds plausible.
All of the http, plugin, and storage tests run as expected or pass with the above patch.
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 on attachment 222789 [details] fix test failure Clearing flags on attachment: 222789 Committed r163217: <http://trac.webkit.org/changeset/163217>