Bug 127255

Summary: Session API
Product: WebKit Reporter: Martin Hock <mhock>
Component: WebKit2Assignee: Martin Hock <mhock>
Status: RESOLVED FIXED    
Severity: Enhancement CC: ap, commit-queue, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
fix typo
none
more plumbing
none
no need to change WebCore + settings fix
none
address comments + correct type
none
address comments none

Description Martin Hock 2014-01-19 11:30:53 PST
Created attachment 221592 [details]
patch

Session API
Comment 1 Martin Hock 2014-01-19 11:32:41 PST
Created attachment 221593 [details]
fix typo
Comment 2 Sam Weinig 2014-01-19 11:51:32 PST
Why does WebCore::Page need a sessionID?
Comment 3 Alexey Proskuryakov 2014-01-19 12:28:50 PST
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.
Comment 4 Alexey Proskuryakov 2014-01-19 23:04:50 PST
EWS is too shy to say so already, but mac-wk2 is seeing many crashes.
Comment 5 Martin Hock 2014-01-21 14:38:28 PST
Created attachment 221790 [details]
more plumbing
Comment 6 WebKit Commit Bot 2014-01-21 14:40:37 PST
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.
Comment 7 Martin Hock 2014-01-21 16:12:10 PST
Created attachment 221803 [details]
no need to change WebCore + settings fix
Comment 8 Alexey Proskuryakov 2014-01-21 19:08:00 PST
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.
Comment 9 Martin Hock 2014-01-22 11:03:17 PST
(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.
Comment 10 Martin Hock 2014-01-22 13:40:45 PST
Created attachment 221900 [details]
address comments + correct type
Comment 11 Alexey Proskuryakov 2014-01-22 14:06:44 PST
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.
Comment 12 Martin Hock 2014-01-22 14:25:36 PST
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.
Comment 13 Martin Hock 2014-01-22 14:54:03 PST
Created attachment 221908 [details]
address comments
Comment 14 WebKit Commit Bot 2014-01-22 16:23:55 PST
Comment on attachment 221908 [details]
address comments

Clearing flags on attachment: 221908

Committed r162568: <http://trac.webkit.org/changeset/162568>
Comment 15 WebKit Commit Bot 2014-01-22 16:23:57 PST
All reviewed patches have been landed.  Closing bug.