Bug 200076 – [SOUP] Move SoupNetworkSession ownership from NetworkStorageSession to NetworkSession

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

NetworkStorageSession should only own the cookie jar, since it's the only thing it handles from the session.

[Carlos Garcia Campos]

Created attachment 374771 [details]
Patch

[Michael Catanzaro]

/Volumes/Data/EWS/WebKit/Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:204:30: error: variable 'modifiedSince' cannot be implicitly captured in a lambda with no capture-default specified
                cache->clear(modifiedSince, [aggregator = aggregator.copyRef()] () { });
                             ^
/Volumes/Data/EWS/WebKit/Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:194:46: note: 'modifiedSince' declared here
void NetworkProcess::clearDiskCache(WallTime modifiedSince, CompletionHandler<void()>&& completionHandler)
                                             ^
/Volumes/Data/EWS/WebKit/Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:202:31: note: lambda expression begins here
        forEachNetworkSession([&aggregator](NetworkSession& session) {
                              ^
/Volumes/Data/EWS/WebKit/Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:200:112: error: lambda capture 'modifiedSince' is not used [-Werror,-Wunused-lambda-capture]
    dispatch_group_async(group, dispatch_get_main_queue(), makeBlockPtr([this, protectedThis = makeRef(*this), modifiedSince, completionHandler = WTFMove(completionHandler)] () mutable {
                                                                                                               ^
2 errors generated.

[Carlos Garcia Campos]

Created attachment 374774 [details]
Patch

[Carlos Garcia Campos]

Created attachment 374775 [details]
Patch

[Michael Catanzaro]

Comment on attachment 374771 [details]
Patch

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

Nice, just don't break Mac please.

Also it needs an owner for the changes in NetworkConnectionToWebProcess.cpp, NetworkProcess.cpp, NetworkProcess.h, NetworkProcessCocoa.mm, and NetworkProcessIOS.mm.

> Source/WebCore/platform/network/NetworkStorageSession.h:99
> -    WEBCORE_EXPORT NetworkStorageSession(PAL::SessionID, std::unique_ptr<SoupNetworkSession>&&);
> +    WEBCORE_EXPORT NetworkStorageSession(PAL::SessionID);

explicit

> Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:58
> +    , m_cookieStorage(adoptGRef(soup_cookie_jar_new()))

It's slightly unfortunate that we create this empty SoupCookieJar and then blow it away shortly thereafter in the usual case when setCookieStorage is called right after it's constructed, from the NetworkSessionSoup.cpp constructor. But that seems hard to avoid, so I suppose this is fine.

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:-131
> -        SOUP_SESSION_ADD_FEATURE, jar.get(),

Important: where does this happen now?

If this is totally gone, then surely cookies won't work anymore?

I'm confused.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:692
> +    networkProcess().forEachNetworkSession([statistics = WTFMove(statistics) ](NetworkSession& networkSession) mutable {

No space before the ]

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:602
> +#if !USE(SOUP)
>      ASSERT(sessionID != PAL::SessionID::defaultSessionID());
> +#endif

Really needs a comment since people reading the code have no chance to know why soup is different here. "Refer to the comment in NetworkProcessMainSoup.cpp for why this needs to be destroyed" would suffice.

Actually, a comment would be bare minimum, but perhaps it would be better to remove this ASSERT altogether. There's no reason to prevent destroying the default NetworkSession on other ports if we allow it for soup, because the cross-platform code has to allow it now. I think the ASSERT is here because a bazillion functions will likely crash if the default NetworkSession is destroyed, so it's a little risky to remove, but I understand that the SoupSession must be destroyed no matter what, and if it's owned by the NetworkSession that means the NetworkSession must be too, and if it's done late enough it should be OK.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2166
> -    for (auto& session : m_networkSessions)
> -        session.value->storageManager().resume();
> +    forEachNetworkSession([](NetworkSession& session) {
> +        session.storageManager().resume();
> +    });

I'll let owners decide whether this sort of transformation is valuable. On the one hand, now that you've added forEachNetworkSession it makes sense to use it wherever possible, and seeing the name of the function at callsites makes it very clear that you are doing a thing for each NetworkSession. On the other hand, within NetworkProcess.cpp itself the for loop syntax is shorter. Either way is fine IMO.

[Carlos Garcia Campos]

Created attachment 374777 [details]
Patch

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #5)
> Comment on attachment 374771 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374771&action=review
> 
> Nice, just don't break Mac please.
> 
> Also it needs an owner for the changes in NetworkConnectionToWebProcess.cpp,
> NetworkProcess.cpp, NetworkProcess.h, NetworkProcessCocoa.mm, and
> NetworkProcessIOS.mm.

Sure.

> > Source/WebCore/platform/network/NetworkStorageSession.h:99
> > -    WEBCORE_EXPORT NetworkStorageSession(PAL::SessionID, std::unique_ptr<SoupNetworkSession>&&);
> > +    WEBCORE_EXPORT NetworkStorageSession(PAL::SessionID);
> 
> explicit

Ok.

> > Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:58
> > +    , m_cookieStorage(adoptGRef(soup_cookie_jar_new()))
> 
> It's slightly unfortunate that we create this empty SoupCookieJar and then
> blow it away shortly thereafter in the usual case when setCookieStorage is
> called right after it's constructed, from the NetworkSessionSoup.cpp
> constructor. But that seems hard to avoid, so I suppose this is fine.

There's no change in behavior here, we were creating the same jar in the SoupNetworkSession constructor before. This ensures there's always a valid cookie storage in the network storage session.

> > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:-131
> > -        SOUP_SESSION_ADD_FEATURE, jar.get(),
> 
> Important: where does this happen now?

In NetworkSession constructor we call setCookieStorage right after the SoupNetworkSession is created.

> If this is totally gone, then surely cookies won't work anymore?
> 
> I'm confused.
> 
> > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:692
> > +    networkProcess().forEachNetworkSession([statistics = WTFMove(statistics) ](NetworkSession& networkSession) mutable {
> 
> No space before the ]

Oops.

> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:602
> > +#if !USE(SOUP)
> >      ASSERT(sessionID != PAL::SessionID::defaultSessionID());
> > +#endif
> 
> Really needs a comment since people reading the code have no chance to know
> why soup is different here. "Refer to the comment in
> NetworkProcessMainSoup.cpp for why this needs to be destroyed" would suffice.
> 
> Actually, a comment would be bare minimum, but perhaps it would be better to
> remove this ASSERT altogether. There's no reason to prevent destroying the
> default NetworkSession on other ports if we allow it for soup, because the
> cross-platform code has to allow it now. I think the ASSERT is here because
> a bazillion functions will likely crash if the default NetworkSession is
> destroyed, so it's a little risky to remove, but I understand that the
> SoupSession must be destroyed no matter what, and if it's owned by the
> NetworkSession that means the NetworkSession must be too, and if it's done
> late enough it should be OK.

I'll add a comment for now.

> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2166
> > -    for (auto& session : m_networkSessions)
> > -        session.value->storageManager().resume();
> > +    forEachNetworkSession([](NetworkSession& session) {
> > +        session.storageManager().resume();
> > +    });
> 
> I'll let owners decide whether this sort of transformation is valuable. On
> the one hand, now that you've added forEachNetworkSession it makes sense to
> use it wherever possible, and seeing the name of the function at callsites
> makes it very clear that you are doing a thing for each NetworkSession. On
> the other hand, within NetworkProcess.cpp itself the for loop syntax is
> shorter. Either way is fine IMO.

[Carlos Garcia Campos]

Created attachment 374778 [details]
Patch for landing

[Carlos Garcia Campos]

Youenn or Alex, could you check the cross-platform changes in this patch, please?

[Carlos Garcia Campos]

Created attachment 374954 [details]
Rebased patch for landing

Ping owners

[Carlos Garcia Campos]

Created attachment 374955 [details]
Patch for landing

[youenn fablet]

Comment on attachment 374955 [details]
Patch for landing

LGTM, the review flag is not set though.

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

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:699
> +    networkProcess().forEachNetworkSession([statistics = WTFMove(statistics)](NetworkSession& networkSession) mutable {

auto&
See also https://bugs.webkit.org/show_bug.cgi?id=200225

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:278
> +    forEachNetworkSession([](NetworkSession& networkSession) {

auto& here and below.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1372
> +            fetchDiskCacheEntries(session.cache(), sessionID, fetchOptions, [callbackAggregator = WTFMove(callbackAggregator)](auto entries) mutable {

There seems to be a preexisting bug here in case there are more than one session as we move clearTasksHandler several times potentially.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1532
> +            clearDiskCacheEntries(session.cache(), originDatas, [clearTasksHandler = WTFMove(clearTasksHandler)] { });

There seems to be a preexisting bug here in case there are more than one session as we move clearTasksHandler several times potentially.

> Source/WebKit/NetworkProcess/NetworkStorageSessionProvider.h:57
> +    SoupSession* soupSession() const final

This seems only called in one place right now.
Maybe instead, we could simply add a cross-platform NetworkSession* networkSession() const getter and soup specific code would do the static cast.

[youenn fablet]

I'll fix the potential use-after-move in a separate bug.

[Carlos Garcia Campos]

Comment on attachment 374955 [details]
Patch for landing

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

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:278
>> +    forEachNetworkSession([](NetworkSession& networkSession) {
> 
> auto& here and below.

Ok.

>> Source/WebKit/NetworkProcess/NetworkStorageSessionProvider.h:57
>> +    SoupSession* soupSession() const final
> 
> This seems only called in one place right now.
> Maybe instead, we could simply add a cross-platform NetworkSession* networkSession() const getter and soup specific code would do the static cast.

Yes, it's a temporary solution, it will be removed in bug #200162.

[Carlos Garcia Campos]

(In reply to youenn fablet from comment #12)
> Comment on attachment 374955 [details]
> Patch for landing
> 
> LGTM, the review flag is not set though.

Because it was already reviewed by Michael, I only needed the owners approval for the cross-platform changes.

[Carlos Garcia Campos]

Created attachment 375157 [details]
Patch for landing

[Carlos Garcia Campos]

Committed r248010: <https://trac.webkit.org/changeset/248010>