Bug 126918 – Identifier for sessions

• Component: WebKit2
• Status: RESOLVED FIXED

[Martin Hock]

Sessions should be identified by a uint64 (much like web pages).  The uint64 can be passed among processes.

[Martin Hock]

Created attachment 221064 [details]
patch

[WebKit Commit Bot]

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.

[Martin Hock]

Created attachment 221065 [details]
style

[Alexey Proskuryakov]

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.

[Martin Hock]

Created attachment 221223 [details]
WIP sessions

[Sam Weinig]

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.

[Martin Hock]

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.

[Sam Weinig]

(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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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 :)

[Martin Hock]

Created attachment 221491 [details]
Fleshed-out sessions

[Martin Hock]

Created attachment 221493 [details]
rebase

[Alexey Proskuryakov]

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.

[Martin Hock]

Created attachment 221502 [details]
address comments

[Alexey Proskuryakov]

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?

[Martin Hock]

Created attachment 221512 [details]
address comments + soup build fixes

[Martin Hock]

Created attachment 221516 [details]
more soup fixes

[Alexey Proskuryakov]

Comment on attachment 221516 [details]
more soup fixes

r=me

[WebKit Commit Bot]

Comment on attachment 221516 [details]
more soup fixes

Clearing flags on attachment: 221516

Committed r162237: <http://trac.webkit.org/changeset/162237>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

Note also this build fix: <http://trac.webkit.org/changeset/162241> that I'm also rolling out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 127216

[Alexey Proskuryakov]

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)

[Alexey Proskuryakov]

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)

[Alexey Proskuryakov]

Err, in UIProcess.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Jer Noble]

(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.

[Alexey Proskuryakov]

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.

[Martin Hock]

Created attachment 221559 [details]
test fixes + EFL build fix

[Alexey Proskuryakov]

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);

[Martin Hock]

Created attachment 221564 [details]
address comment

[Alexey Proskuryakov]

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();

[Martin Hock]

Created attachment 221565 [details]
missing deref (where did it go!?)

[Martin Hock]

Comment on attachment 221565 [details]
missing deref (where did it go!?)

OK, this patch definitely builds :)

[WebKit Commit Bot]

Comment on attachment 221565 [details]
missing deref (where did it go!?)

Clearing flags on attachment: 221565

Committed r162271: <http://trac.webkit.org/changeset/162271>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.