WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
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
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Martin Hock
Comment 1
2014-01-30 11:29:38 PST
Created
attachment 222698
[details]
patch
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.
Top of Page
Format For Printing
XML
Clone This Bug