Bug 160173 – Move NetworkStorageSession management to WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Move NetworkStorageSession management to WebCore

[Alex Christensen]

Created attachment 284527 [details]
Patch

[Alex Christensen]

Created attachment 284528 [details]
Patch

[Alex Christensen]

Created attachment 284529 [details]
Patch

[Alex Christensen]

Created attachment 284530 [details]
Patch

[Alex Christensen]

Created attachment 284531 [details]
Patch

[Alex Christensen]

Created attachment 284534 [details]
Patch

[Alex Christensen]

Created attachment 284537 [details]
Patch

[Alex Christensen]

Created attachment 284538 [details]
Patch

[Brady Eidson]

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

[Alex Christensen]

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

[Alex Christensen]

Created attachment 284541 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Sam Weinig]

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.

[Alex Christensen]

Created attachment 285680 [details]
Patch

[Alex Christensen]

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

[Brady Eidson]

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

[Sam Weinig]

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

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 285690 [details]
Patch

[Alex Christensen]

Created attachment 285715 [details]
Patch

[Alex Christensen]

https://trac.webkit.org/changeset/204327

[Csaba Osztrogonác]

(In reply to comment #24)
> https://trac.webkit.org/changeset/204327

It broke rhe WinCairo build.

[Alex Christensen]

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

[Csaba Osztrogonác]

(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