Bug 127255 – Session API

• Component: WebKit2
• Status: RESOLVED FIXED

[Martin Hock]

Created attachment 221592 [details]
patch

Session API

[Martin Hock]

Created attachment 221593 [details]
fix typo

[Sam Weinig]

Why does WebCore::Page need a sessionID?

[Alexey Proskuryakov]

Comment on attachment 221593 [details]
fix typo

View in context: https://bugs.webkit.org/attachment.cgi?id=221593&action=review

> Source/WebCore/page/Page.h:421
> +    uint64_t getSessionID() { return m_sessionID; }

WebKit style is to not have a "get" prefix on getters. We use this prefix for functions that return result via an argument, e.g. 

bool getFoobar(Foobar& result);

> Source/WebKit2/UIProcess/API/C/WKSessionRef.h:39
>  WK_EXPORT bool WKSessionGetEphemeral(WKSessionRef session);

As previously commented, this should be WKSessionIsEphemeral, not WKSessionGetEphemeral. The name WKSessionGetEphemeral means that we are getting and returning an ephemeral session.

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm:107
> +        if (frame()->page()->getSessionID())
> +            return *SessionTracker::session(frame()->page()->getSessionID());
>  
> +        if (frame()->settings().privateBrowsingEnabled())
> +            return *SessionTracker::session(SessionTracker::legacyPrivateSessionID);

This is not the right approach. The knowledge about legacy session IDs should be isolated as close to API layer as possible, most of the code should simply use sessions.

Having separate code paths for sessions and for private browsing everywhere we used to check for privateBrowsingEnabled() is very fragile and confusing.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:878
> +    uint64_t m_sessionID;

I wouldn't duplicate the knowledge in this class, WebCore already knows what session it is.

[Alexey Proskuryakov]

EWS is too shy to say so already, but mac-wk2 is seeing many crashes.

[Martin Hock]

Created attachment 221790 [details]
more plumbing

[WebKit Commit Bot]

Attachment 221790 [details] did not pass style-queue:


ERROR: Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 3 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Hock]

Created attachment 221803 [details]
no need to change WebCore + settings fix

[Alexey Proskuryakov]

Comment on attachment 221803 [details]
no need to change WebCore + settings fix

View in context: https://bugs.webkit.org/attachment.cgi?id=221803&action=review

This looks pretty much right to me. I'd like to make another pass later when I have a bit more time.

One thing I didn't see was how we create sessions after NetworkProcess crash and relaunch. Don't we need to iterate over all sessions and tell NetworkProcess to create them? Is this ever an issue for WebProcess, as well?

> Source/WebKit2/NetworkProcess/mac/RemoteNetworkingContext.mm:62
> +    if (m_sessionID) {

Can sessionID be 0?

> Source/WebKit2/Shared/SessionTracker.h:43
> +    // FIXME: getSessionMap()'s returned map does not include default session
>      static const HashMap<uint64_t, std::unique_ptr<WebCore::NetworkStorageSession>>& getSessionMap();

getSessionMap is not a correct name for the function, because it returns the result in a normal way.

As a bigger change, it would be nice to find a way to not expose the map, it should be encapsulated by the tracker. Perhaps it could have a method that applies a functor to all sessions.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2126
> +    if (m_sessionID)

I don't see m_sessionID initialized anywhere unless setSessionID is called.

[Martin Hock]

(In reply to comment #8)
> (From update of attachment 221803 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221803&action=review
> 
> This looks pretty much right to me. I'd like to make another pass later when I have a bit more time.
> 
> One thing I didn't see was how we create sessions after NetworkProcess crash and relaunch. Don't we need to iterate over all sessions and tell NetworkProcess to create them? Is this ever an issue for WebProcess, as well?

I think we are OK because we always specify the session ID in the NetworkResourceLoadParameters.  Then, in NetworkResourceLoader::start, we ensure the existence of the RemoteNetworkingContext with that ID.

> > Source/WebKit2/NetworkProcess/mac/RemoteNetworkingContext.mm:62
> > +    if (m_sessionID) {
> 
> Can sessionID be 0?

Actually, you're right that it can't be here.  Internal to WebPage it can be, but not outside of WebPage.  I'll fix that.

> > Source/WebKit2/Shared/SessionTracker.h:43
> > +    // FIXME: getSessionMap()'s returned map does not include default session
> >      static const HashMap<uint64_t, std::unique_ptr<WebCore::NetworkStorageSession>>& getSessionMap();
> 
> getSessionMap is not a correct name for the function, because it returns the result in a normal way.

OK, I'll name the internal one staticSessionMap() for now.

> As a bigger change, it would be nice to find a way to not expose the map, it should be encapsulated by the tracker. Perhaps it could have a method that applies a functor to all sessions.

I asked Anders about this and he thought we should just expose the const map.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2126
> > +    if (m_sessionID)
> 
> I don't see m_sessionID initialized anywhere unless setSessionID is called.

Good catch.  I'll initialize it in the constructor's initialization list, as it ought to be.

[Martin Hock]

Created attachment 221900 [details]
address comments + correct type

[Alexey Proskuryakov]

Comment on attachment 221900 [details]
address comments + correct type

View in context: https://bugs.webkit.org/attachment.cgi?id=221900&action=review

> Source/WebKit2/NetworkProcess/RemoteNetworkingContext.h:48
> +    // FIXME: stop relying on creating sessions on demand to allow for persistent sessions

I don't think that this is a good place for the FIXME - this is not where the sessions are created, nor where they should be created.

WebKit style is to start sentences with an upper case letter, and to finish with a period.

I'd say something like this (not in this header):

// FIXME: We can create sessions on demand, because we hardcode the fact that all sessions but the default one are ephemeral. We'll need to create them explicitly once sessions have more configuration options.

> Source/WebKit2/NetworkProcess/mac/RemoteNetworkingContext.mm:62
> +    NetworkStorageSession* privateSession = SessionTracker::session(m_sessionID);

Why "privateSession"? I think it's just "session".

> Source/WebKit2/NetworkProcess/mac/RemoteNetworkingContext.mm:65
> +    // Some requests with legacy private browsing mode requested may still be coming shortly after NetworkProcess was told to destroy its session.

This is the case for any session, not just the legacy private browsing one.

> Source/WebKit2/Shared/SessionTracker.cpp:55
> +const HashMap<uint64_t, std::unique_ptr<NetworkStorageSession>>& SessionTracker::sessionMap()
>  {
> -    return sessionMap();
> +    return staticSessionMap();
>  }

I don't understand why we have both.

> Source/WebKit2/Shared/SessionTracker.h:42
> +    // FIXME: getSessionMap()'s returned map does not include default session

The function name need to be updated here, too.

Also, please add a period.

> Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:120
> +    loadParameters.sessionID = webPage ? webPage->sessionID() : 0;

This doesn't look right to me. Won't we just crash NetworkProcess with a sessionID of 0? Previously, we were able to get privateBrowsingEnabled regardless of anything.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:273
> +    loadParameters.sessionID = webPage ? webPage->sessionID() : 0;

Ditto.

[Martin Hock]

Thanks for your suggestions, they look good. I just wanted to comment on one:

(In reply to comment #11)
> > Source/WebKit2/Shared/SessionTracker.cpp:55
> > +const HashMap<uint64_t, std::unique_ptr<NetworkStorageSession>>& SessionTracker::sessionMap()
> >  {
> > -    return sessionMap();
> > +    return staticSessionMap();
> >  }
> 
> I don't understand why we have both.

Because we don't want to expose a non-const HashMap but we do need to use it internally, and due to it being a static object, it needs to be referenced via a function. It's only used internally so if it's a bit awkward, it shouldn't be too problematic.

[Martin Hock]

Created attachment 221908 [details]
address comments

[WebKit Commit Bot]

Comment on attachment 221908 [details]
address comments

Clearing flags on attachment: 221908

Committed r162568: <http://trac.webkit.org/changeset/162568>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.