Bug 160173

Summary: Move NetworkStorageSession management to WebCore
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, buildbot, ossy, pvollan, rniwa, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews101 for mac-yosemite
none
Archive of layout-test-results from ews115 for mac-yosemite
none
Patch
none
Patch
none
Patch none

Description Alex Christensen 2016-07-25 15:00:51 PDT
Move NetworkStorageSession management to WebCore
Comment 1 Alex Christensen 2016-07-25 15:08:29 PDT
Created attachment 284527 [details]
Patch
Comment 2 Alex Christensen 2016-07-25 15:20:19 PDT
Created attachment 284528 [details]
Patch
Comment 3 Alex Christensen 2016-07-25 15:23:29 PDT
Created attachment 284529 [details]
Patch
Comment 4 Alex Christensen 2016-07-25 15:26:58 PDT
Created attachment 284530 [details]
Patch
Comment 5 Alex Christensen 2016-07-25 15:35:36 PDT
Created attachment 284531 [details]
Patch
Comment 6 Alex Christensen 2016-07-25 15:45:03 PDT
Created attachment 284534 [details]
Patch
Comment 7 Alex Christensen 2016-07-25 15:50:40 PDT
Created attachment 284537 [details]
Patch
Comment 8 Alex Christensen 2016-07-25 15:56:21 PDT
Created attachment 284538 [details]
Patch
Comment 9 Brady Eidson 2016-07-25 16:06:00 PDT
Comment on attachment 284538 [details]
Patch

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

After all of these changes, does SessionTracker need to remain "a thing"?

> Source/WebCore/ChangeLog:8
> +        No new tests.  No change in behaviour.

Great, but...

> Source/WebCore/ChangeLog:22
> +        (WebCore::NetworkStorageSession::forEach):
> +        Moved from forEachNetworkStorageSession and fixed the FIXME.  This function now includes the default NetworkStorageSession.

...This is a behavior change?

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:86
> +static HashMap<SessionID, std::unique_ptr<NetworkStorageSession>>& globalSessionMap()
> +{
> +    static NeverDestroyed<HashMap<SessionID, std::unique_ptr<NetworkStorageSession>>> map;
> +    return map;
> +}
> +
> +NetworkStorageSession* NetworkStorageSession::storageSession(SessionID sessionID)
> +{
> +    if (sessionID == SessionID::defaultSessionID())
> +        return &defaultStorageSession();
> +    return globalSessionMap().get(sessionID);
> +}

CFNet versions, but then....

> Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:67
> +static HashMap<SessionID, std::unique_ptr<NetworkStorageSession>>& globalSessionMap()
> +{
> +    static NeverDestroyed<HashMap<SessionID, std::unique_ptr<NetworkStorageSession>>> map;
> +    return map;
> +}
> +
> +NetworkStorageSession* NetworkStorageSession::storageSession(SessionID sessionID)
> +{
> +    if (sessionID == SessionID::defaultSessionID())
> +        return &defaultStorageSession();
> +    return globalSessionMap().get(sessionID);
> +}

...Soup versions?

Can't these be in shared code?

> Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:94
> +void NetworkStorageSession::destroySession(SessionID sessionID)
> +{
> +    ASSERT(globalSessionMap().contains(sessionID));
> +    globalSessionMap().remove(sessionID);
> +}
> +
> +void NetworkStorageSession::forEach(std::function<void(const WebCore::NetworkStorageSession&)> functor)
> +{
> +    functor(defaultStorageSession());
> +    for (auto& storageSession : globalSessionMap().values())
> +        functor(*storageSession);

And these, etc etc...
Comment 10 Alex Christensen 2016-07-25 16:07:19 PDT
(In reply to comment #9)
> Comment on attachment 284538 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284538&action=review
> 
> After all of these changes, does SessionTracker need to remain "a thing"?
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests.  No change in behaviour.
> 
> Great, but...
> 
> > Source/WebCore/ChangeLog:22
> > +        (WebCore::NetworkStorageSession::forEach):
> > +        Moved from forEachNetworkStorageSession and fixed the FIXME.  This function now includes the default NetworkStorageSession.
> 
> ...This is a behavior change?
This would be a behavior change if I didn't also change the call sites to not do the same thing to the default NetworkStorageSession.  With those changes it is not a behavior change.
Comment 11 Alex Christensen 2016-07-25 16:19:20 PDT
Created attachment 284541 [details]
Patch
Comment 12 Build Bot 2016-07-25 16:49:14 PDT
Comment on attachment 284541 [details]
Patch

Attachment 284541 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1753385

New failing tests:
http/tests/cookies/private-cookie-storage.html
http/tests/security/contentSecurityPolicy/report-same-origin-no-cookies-when-private-browsing-toggled.php
http/tests/security/private-browsing-http-auth.html
Comment 13 Build Bot 2016-07-25 16:49:17 PDT
Created attachment 284545 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 14 Build Bot 2016-07-25 17:24:02 PDT
Comment on attachment 284541 [details]
Patch

Attachment 284541 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1753459

Number of test failures exceeded the failure limit.
Comment 15 Build Bot 2016-07-25 17:24:05 PDT
Created attachment 284553 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 16 Sam Weinig 2016-08-01 13:16:15 PDT
I have a vision for Sessions, and I am curious if this gets us closer to it or further away. 

My vision is that we stop passing SessionIDs around, and invert the equation.  Add a WebCore::Session object (name TBD) and have it own instances of all the things SessionIDs are currently used to track.  For instance, it would have a MemoryCache instance, rather than MemoryCache having a mapping of SessionIDs to CachedResourceMaps.   Additionally, the WebCore::Session could contain an NetworkStorageSession, or,  NetworkStorageSession could be folded into it.
Comment 17 Alex Christensen 2016-08-09 15:25:08 PDT
Created attachment 285680 [details]
Patch
Comment 18 Alex Christensen 2016-08-09 15:26:07 PDT
(In reply to comment #16)
> I have a vision for Sessions, and I am curious if this gets us closer to it
> or further away. 
This patch gets us closer to that.  We will still need to have SessionID's, though, because we need to pass them across IPC.
Comment 19 Brady Eidson 2016-08-09 16:06:42 PDT
(In reply to comment #16)
> I have a vision for Sessions, and I am curious if this gets us closer to it
> or further away. 
> 
> My vision is that we stop passing SessionIDs around...

But then...

(In reply to comment #18)
> This patch gets us closer to that.  We will still need to have SessionID's,
> though, because we need to pass them across IPC.

These two comments are at odds.
Comment 20 Sam Weinig 2016-08-09 16:43:41 PDT
(In reply to comment #19)
> (In reply to comment #16)
> > I have a vision for Sessions, and I am curious if this gets us closer to it
> > or further away. 
> > 
> > My vision is that we stop passing SessionIDs around...
> 
> But then...
> 
> (In reply to comment #18)
> > This patch gets us closer to that.  We will still need to have SessionID's,
> > though, because we need to pass them across IPC.
> 
> These two comments are at odds.

Not really. Having WebCore invert the relationship to use a Session object that contains a Session identifier is fine.  Just as we have Page from which we can get a Page identifier for IPC purposes.
Comment 21 Alex Christensen 2016-08-09 16:51:25 PDT
Moving the NetworkStorageSession management to WebCore helps us progress towards the existence of a WebCore::Session because before that can be done, everything that a SessionID is used for must be moved to WebCore.
Comment 22 Alex Christensen 2016-08-09 16:53:27 PDT
Created attachment 285690 [details]
Patch
Comment 23 Alex Christensen 2016-08-09 22:55:40 PDT
Created attachment 285715 [details]
Patch
Comment 24 Alex Christensen 2016-08-10 00:03:19 PDT
https://trac.webkit.org/changeset/204327
Comment 25 Csaba Osztrogonác 2016-08-10 00:08:06 PDT
(In reply to comment #24)
> https://trac.webkit.org/changeset/204327

It broke rhe WinCairo build.
Comment 26 Alex Christensen 2016-08-10 18:17:22 PDT
Comment on attachment 285715 [details]
Patch

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

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm:-74
> -    if (RetainPtr<CFHTTPCookieStorageRef> cookieStorage = NetworkStorageSession::defaultStorageSession().cookieStorage())

This null check was removed in this change.

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm:73
>          CFHTTPCookieStorageSetCookieAcceptPolicy(networkStorageSession.cookieStorage().get(), policy);

This needs a null check.  See https://bugs.webkit.org/show_bug.cgi?id=160758
Comment 27 Csaba Osztrogonác 2016-08-11 02:53:28 PDT
(In reply to comment #25)
> (In reply to comment #24)
> > https://trac.webkit.org/changeset/204327
> 
> It broke rhe WinCairo build.

just to document, fixed by https://trac.webkit.org/changeset/204366