Sessions should be identified by a uint64 (much like web pages). The uint64 can be passed among processes.
Created attachment 221064 [details] patch
Attachment 221064 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp', u'Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h', u'Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.messages.in', u'Source/WebKit2/UIProcess/WebContext.cpp', u'Source/WebKit2/UIProcess/WebContext.h', u'Source/WebKit2/UIProcess/WebPageProxy.cpp', u'Source/WebKit2/UIProcess/WebPageProxy.h', u'Source/WebKit2/UIProcess/WebProcessProxy.cpp', u'Source/WebKit2/UIProcess/WebProcessProxy.h', '--commit-queue']" exit_code: 1 ERROR: Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:166: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebKit2/UIProcess/WebProcessProxy.cpp:85: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 221065 [details] style
Comment on attachment 221065 [details] style View in context: https://bugs.webkit.org/attachment.cgi?id=221065&action=review > Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h:96 > + void startDownloadSession(uint64_t sessionID, uint64_t downloadID, const WebCore::ResourceRequest&); > + > + void cookiesForDOMSession(uint64_t sessionID, const WebCore::URL& firstParty, const WebCore::URL&, String& result); > + void setCookiesFromDOMSession(uint64_t sessionID, const WebCore::URL& firstParty, const WebCore::URL&, const String&); > + void cookiesEnabledSession(uint64_t sessionID, const WebCore::URL& firstParty, const WebCore::URL&, bool& result); > + void cookieRequestHeaderFieldValueSession(uint64_t sessionID, const WebCore::URL& firstParty, const WebCore::URL&, String& result); > + void getRawCookiesSession(uint64_t sessionID, const WebCore::URL& firstParty, const WebCore::URL&, Vector<WebCore::Cookie>&); > + void deleteCookieSession(uint64_t sessionID, const WebCore::URL&, const String& cookieName); I don't think that we need to add any new functions here. What I had in mind when adding privateBrowsingEnabled flag was that it would be replaced with session ID eventually. > Source/WebKit2/UIProcess/WebProcessProxy.cpp:184 > + uint64_t sessionID = generateSessionID(isEphemeral); This means that multiple pages with ephemeral sessions will never use the same session. I don't think that this is how the API should work - the client should provide a session. > Source/WebKit2/UIProcess/WebProcessProxy.h:80 > + PassRefPtr<WebPageProxy> createWebPage(PageClient&, WebPageGroup&, bool); This new boolean arguments looks mysterious - there is no way for me to know what it does from this line of code.
Created attachment 221223 [details] WIP sessions
Comment on attachment 221223 [details] WIP sessions View in context: https://bugs.webkit.org/attachment.cgi?id=221223&action=review > Source/WebKit2/UIProcess/APISession.cpp:35 > + ASSERT(isMainThread()); No need for this. > Source/WebKit2/UIProcess/APISession.cpp:44 > + ASSERT(isMainThread()); No need for this. > Source/WebKit2/UIProcess/APISession.cpp:52 > + ASSERT(isMainThread()); No need for this. > Source/WebKit2/UIProcess/APISession.h:39 > static PassRefPtr<Session> create(bool isEphemeral); Perhaps we should have this called createEphemeral() instead since it is illegal currently to call if with isEphemeral=false. Also, please add a FIXME about adding support for non-default, non-ephemeral sessions. > Source/WebKit2/UIProcess/APISession.h:48 > + static uint64_t generateID(bool isEphemeral); This does not need to be a member. > Source/WebKit2/UIProcess/WebPageProxy.h:1314 > + API::Session m_session; This needs to be a RefPtr<API::Session> > Source/WebKit2/UIProcess/WebProcessProxy.h:96 > + static bool isEphemeralSession(uint64_t); This doesn't seem to have an implemenation. > Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm:41 > +#import <wtf/text/CString.h> This should be an #include.
Thanks for taking a look. (In reply to comment #6) > (From update of attachment 221223 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=221223&action=review > > > Source/WebKit2/UIProcess/APISession.cpp:35 > > + ASSERT(isMainThread()); > > No need for this. > > > Source/WebKit2/UIProcess/APISession.cpp:44 > > + ASSERT(isMainThread()); > > No need for this. > > > Source/WebKit2/UIProcess/APISession.cpp:52 > > + ASSERT(isMainThread()); > > No need for this. It's true that these methods aren't thread safe, though - why isn't it needed? > > > Source/WebKit2/UIProcess/APISession.h:48 > > + static uint64_t generateID(bool isEphemeral); > > This does not need to be a member. I wanted it to be able to access private static members. But I guess those members can be translation unit static variables instead, so I'll do that.
(In reply to comment #7) > Thanks for taking a look. > > (In reply to comment #6) > > (From update of attachment 221223 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=221223&action=review > > > > > Source/WebKit2/UIProcess/APISession.cpp:35 > > > + ASSERT(isMainThread()); > > > > No need for this. > > > > > Source/WebKit2/UIProcess/APISession.cpp:44 > > > + ASSERT(isMainThread()); > > > > No need for this. > > > > > Source/WebKit2/UIProcess/APISession.cpp:52 > > > + ASSERT(isMainThread()); > > > > No need for this. > > It's true that these methods aren't thread safe, though - why isn't it needed? We don't annotate all non-thread safe things, just ones where it might be confusing.
I find these assertions very useful as a debugging tool. First, a function like Session::defaultSession() could be called very deep in specialized code that may not even know itself which thread it is running on. Second, quickly catching API misuse by clients is invaluable when debugging issues caused by such misuse.
Comment on attachment 221223 [details] WIP sessions View in context: https://bugs.webkit.org/attachment.cgi?id=221223&action=review > Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:153 > + if (API::Session::isEphemeralID(sessionID)) { I'm not sure about how we use API objects in general. Is it right to use API objects in NetworkProcess code? > Source/WebKit2/NetworkProcess/mac/RemoteNetworkingContext.mm:42 > +static HashMap<uint64_t, std::unique_ptr<NetworkStorageSession>>& sessionMap() We need this kind of session map in both network process and web process. Where can we put it in shared code? > Source/WebKit2/NetworkProcess/mac/RemoteNetworkingContext.mm:121 > + RetainPtr<CFStringRef> cfIdentifier = String::format("%s.%lld.PrivateBrowsing", privateBrowsingStorageSessionIdentifierBase().utf8().data() , sessionID).createCFString(); I don't think that this is quite right. String::format only works with Latin-1 strings, not utf-8. Please use either StringBuilder or "+" instead. The plus operator is overridden in a way that makes it efficient (and this is a super cold code path anyway). > Source/WebKit2/NetworkProcess/mac/RemoteNetworkingContext.mm:129 > + privateBrowsingStorageSession(sessionID) = nullptr; > + sessionMap().remove(sessionID); Why is the first assignment needed? > Source/WebKit2/UIProcess/WebContext.cpp:482 > +void WebContext::ensurePrivateBrowsingSession(uint64_t sessionID) Is this called from anywhere in this patch? I couldn't find it. Should probably add the appropriate calls to round up the patch. > Source/WebKit2/UIProcess/WebContext.h:296 > + static bool isEphemeralSession(uint64_t sessionID); Why is this a member of WebContext? > Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm:60 > + > + Extra newline here, please remove. > Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm:85 > + privateSession(sessionID) = NetworkStorageSession::createPrivateBrowsingSession(String::format("%s.%lld", base.utf8().data(), sessionID)); Same comment about String::format. > Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm:92 > + privateSession(sessionID) = nullptr; Really need to share all this code. > Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm:103 > + for (const auto& i : sessionMap()) Good names are particularly helpful for auto variables. What is "i", is it a "sessionIterator"? > Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm:105 > + if (i.value) > + WKSetHTTPCookieAcceptPolicy(i.value->cookieStorage().get(), policy); There are two lines of code here, so the block needs braces. I'd personally add a variable for i.value, too. This is not WTF code :)
Created attachment 221491 [details] Fleshed-out sessions
Created attachment 221493 [details] rebase
Comment on attachment 221491 [details] Fleshed-out sessions View in context: https://bugs.webkit.org/attachment.cgi?id=221491&action=review Looks almost ready to land to me. Let's also check EWS results for the patch you uploaded. > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:165 > - RemoteNetworkingContext::ensurePrivateBrowsingSession(); > + RemoteNetworkingContext::ensurePrivateBrowsingSession(SessionTracker::legacyPrivateSessionID); Longer term, I think that we should remove parameters.privateBrowsingEnabled, and just send an explicit message after process initialization. That will let us remove the knowledge of legacy private browsing session from WebProcess and NetworkProcess. Please add a FIXME to this effect if you agree. > Source/WebKit2/NetworkProcess/mac/RemoteNetworkingContext.mm:33 > +#import <wtf/HashMap.h> This seems unneeded now. > Source/WebKit2/NetworkProcess/mac/RemoteNetworkingContext.mm:36 > +#import <wtf/text/CString.h> Ditto. > Source/WebKit2/NetworkProcess/mac/RemoteNetworkingContext.mm:65 > NetworkStorageSession& RemoteNetworkingContext::storageSession() const > { > if (m_privateBrowsingEnabled) { > - NetworkStorageSession* privateSession = privateBrowsingStorageSession().get(); > + NetworkStorageSession* privateSession = SessionTracker::session(SessionTracker::legacyPrivateSessionID).get(); An I correct to understand that the next patch will make NetworkingContext hold a session ID (or take it from Frame in other subclasses)? The knowledge of legacy private browsing session should be expunged from lower levels if WebKit. > Source/WebKit2/NetworkProcess/mac/RemoteNetworkingContext.mm:86 > +void RemoteNetworkingContext::ensurePrivateBrowsingSession(uint64_t sessionID) I'm not sure if I understand why ensurePrivateBrowsingSession() stays in this class. All this code does is call through to SessionTracker. Is this to make keep platform dependent code as it is now? We should probably move it to SessionTracker anyway longer term (please add a FIXME). > Source/WebKit2/Shared/SessionTracker.h:29 > +#include <WebCore/NetworkStorageSession.h> This doesn't seem needed, can we forward declare NetworkStorageSession? > Source/WebKit2/Shared/SessionTracker.h:32 > +#include <wtf/text/CString.h> This is not needed. > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:137 > + if (!WebProcess::shared().networkConnection()->connection()->sendSync(Messages::NetworkConnectionToWebProcess::CookiesForDOM(session.isPrivateBrowsingSession() ? SessionTracker::legacyPrivateSessionID : SessionTracker::defaultSessionID, firstParty, url), Messages::NetworkConnectionToWebProcess::CookiesForDOM::Reply(result), 0)) Can this copy/pasted code be encapsulated somewhere? session.isPrivateBrowsingSession() ? SessionTracker::legacyPrivateSessionID : SessionTracker::defaultSessionID I'd say SessionTracker::sessionID(session). > Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm:41 > +#include <wtf/HashMap.h> > #include <wtf/NeverDestroyed.h> > +#include <wtf/text/CString.h> Unneeded includes here, too. > Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm:68 > + for (const auto& sessionPair : SessionTracker::getSessionMap()) { Why is this a pair? I thought it was an iterator.
Created attachment 221502 [details] address comments
Comment on attachment 221502 [details] address comments View in context: https://bugs.webkit.org/attachment.cgi?id=221502&action=review Looks good to me. Please fix failing builds (gtk-wk2 is red, it's a shame that we are no longer informed about such failures immediately, and have to poll). Please also run all tests in a local debug build. > Source/WebKit2/NetworkProcess/mac/RemoteNetworkingContext.mm:90 > + SessionTracker::session(sessionID) = std::move(NetworkStorageSession::createPrivateBrowsingSession(SessionTracker::getIdentifierBase() + '.' + String::number(sessionID))); Is this std::move needed? > Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm:55 > + SessionTracker::session(sessionID) = std::move(NetworkStorageSession::createPrivateBrowsingSession(base + '.' + String::number(sessionID))); Is this std::move needed?
Created attachment 221512 [details] address comments + soup build fixes
Created attachment 221516 [details] more soup fixes
Comment on attachment 221516 [details] more soup fixes r=me
Comment on attachment 221516 [details] more soup fixes Clearing flags on attachment: 221516 Committed r162237: <http://trac.webkit.org/changeset/162237>
All reviewed patches have been landed. Closing bug.
This broke 63 API tests on debug bots: http://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK2%20(Tests)/builds/1747/steps/run-api-tests/logs/stdio
Note also this build fix: <http://trac.webkit.org/changeset/162241> that I'm also rolling out.
Re-opened since this is blocked by bug 127216
The bot was seeing crashes like this in NetworkProcess: Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebKit2 0x0000000102657953 WebKit::storageSession(unsigned long long) + 15 (memory:2666) 1 com.apple.WebKit2 0x00000001026579f2 WebKit::NetworkConnectionToWebProcess::cookieRequestHeaderFieldValue(unsigned long long, WebCore::URL const&, WebCore::URL const&, WTF::String&) + 32 (NetworkConnectionToWebProcess.cpp:203) 2 com.apple.WebKit2 0x0000000102659469 void IPC::handleMessage<Messages::NetworkConnectionToWebProcess::CookieRequestHeaderFieldValue, WebKit::NetworkConnectionToWebProcess, void (WebKit::NetworkConnectionToWebProcess::*)(unsigned long long, WebCore::URL const&, WebCore::URL const&, WTF::String&)>(IPC::MessageDecoder&, IPC::MessageEncoder&, WebKit::NetworkConnectionToWebProcess*, void (WebKit::NetworkConnectionToWebProcess::*)(unsigned long long, WebCore::URL const&, WebCore::URL const&, WTF::String&)) + 158 (ArgumentEncoder.h:55)
Locally, I'm seeing this in WebProcess: Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebKit2 0x000000010f1a8d4c WTF::RefPtr<WebKit::WebPreferences>::operator!() const + 12 (RefPtr.h:66) 1 com.apple.WebKit2 0x000000010f1a8115 WebKit::WebPageGroup::preferences() const + 37 (WebPageGroup.cpp:119) 2 com.apple.WebKit2 0x000000010f0705b7 WebKit::WebContext::createWebPage(WebKit::PageClient&, WebKit::WebPageGroup*, WebKit::WebPageProxy*) + 71 (WebContext.cpp:765) 3 com.apple.WebKit2 0x000000010f3a052d -[WKView(Private) initWithFrame:contextRef:pageGroupRef:relatedToPage:] + 1613 (WKView.mm:2864) 4 com.apple.WebKit2 0x000000010f39fed9 -[WKView(Private) initWithFrame:contextRef:pageGroupRef:] + 153 (WKView.mm:2834) 5 TestWebKitAPI 0x000000010cbc5975 TestWebKitAPI::PlatformWebView::PlatformWebView(OpaqueWKContext const*, OpaqueWKPageGroup const*) + 277 (PlatformWebViewMac.mm:47) 6 TestWebKitAPI 0x000000010cbc5855 TestWebKitAPI::PlatformWebView::PlatformWebView(OpaqueWKContext const*, OpaqueWKPageGroup const*) + 37 (PlatformWebViewMac.mm:56) 7 TestWebKitAPI 0x000000010caaf7cf TestWebKitAPI::WebKit2_AboutBlankLoad_Test::TestBody() + 63 (AboutBlankLoad.cpp:44)
Err, in UIProcess.
So the crash from comment 25 is easy to fix, we just need to handle null pageGroups. But crash in comment 24 is worse. These NetworkProcess crashes correspond to layout test regressions that were only occurring on one bot, Apple Mavericks Release: http/tests/media/pdf-served-as-pdf.html http/tests/security/contentSecurityPolicy/media-src-blocked.html Rolling out fixed these regressions, so they are definitely caused by this patch.
Oh I see, that's because cookie related IPC calls are also made in CookieStorageShim, and that was not updated to use sessionID integer instead of privateBrowsing boolean. The code in CookieStorageShim is pretty horribly broken, as it doesn't respect private browsing! It's also quite unfortunate that we don't catch NetworkProcess crashes when running tests.
(In reply to comment #28) > Oh I see, that's because cookie related IPC calls are also made in CookieStorageShim, and that was not updated to use sessionID integer instead of privateBrowsing boolean. > > The code in CookieStorageShim is pretty horribly broken, as it doesn't respect private browsing! > > It's also quite unfortunate that we don't catch NetworkProcess crashes when running tests. I'm working on updating the CookieStorageShim to fix an unrelated issue in bug #127207. I adopted SessionTracker between when this was committed and when it was rolled out. We should definitely figure out how to signal to the shim to use private browsing mode, but it will be difficult to associate a specific session ID to a specific cookie request. We should track that in a new bug.
Thanks for weighing in! So the fix for this issue is to simply hardcode the default session instead of "false" in the cookie shim for now.
Created attachment 221559 [details] test fixes + EFL build fix
Comment on attachment 221559 [details] test fixes + EFL build fix View in context: https://bugs.webkit.org/attachment.cgi?id=221559&action=review > Source/WebKit2/UIProcess/WebContext.cpp:765 > + return createWebPage(pageClient, pageGroup, pageGroup && pageGroup->preferences() && pageGroup->preferences()->privateBrowsingEnabled() ? API::Session::legacyPrivateSession() : API::Session::defaultSession(), relatedPage); I don't think that using the default session is right. The function above does a different thing: return process->createWebPage(pageClient, pageGroup ? *pageGroup : m_defaultPageGroup.get(), session);
Created attachment 221564 [details] address comment
Comment on attachment 221564 [details] address comment The patch doesn't build: /Volumes/Data/EWS/WebKit/Source/WebKit2/UIProcess/WebContext.cpp:765:37: error: incompatible operand types ('WebKit::WebPageGroup *' and 'WebKit::WebPageGroup') WebPageGroup* group = pageGroup ? pageGroup : m_defaultPageGroup.get();
Created attachment 221565 [details] missing deref (where did it go!?)
Comment on attachment 221565 [details] missing deref (where did it go!?) OK, this patch definitely builds :)
Comment on attachment 221565 [details] missing deref (where did it go!?) Clearing flags on attachment: 221565 Committed r162271: <http://trac.webkit.org/changeset/162271>