Bug 200076 - [SOUP] Move SoupNetworkSession ownership from NetworkStorageSession to NetworkSession
Summary: [SOUP] Move SoupNetworkSession ownership from NetworkStorageSession to Networ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2019-07-24 06:00 PDT by Carlos Garcia Campos
Modified: 2019-07-30 09:31 PDT (History)
4 users (show)

See Also:


Attachments
Patch (44.36 KB, patch)
2019-07-24 06:22 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (44.38 KB, patch)
2019-07-24 07:33 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (44.40 KB, patch)
2019-07-24 07:47 PDT, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff
Patch (44.39 KB, patch)
2019-07-24 07:59 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch for landing (44.56 KB, patch)
2019-07-24 08:09 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased patch for landing (44.49 KB, patch)
2019-07-26 01:25 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch for landing (44.39 KB, patch)
2019-07-26 01:42 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch for landing (42.86 KB, patch)
2019-07-30 07:41 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2019-07-24 06:00:10 PDT
NetworkStorageSession should only own the cookie jar, since it's the only thing it handles from the session.
Comment 1 Carlos Garcia Campos 2019-07-24 06:22:29 PDT
Created attachment 374771 [details]
Patch
Comment 2 Michael Catanzaro 2019-07-24 07:21:39 PDT
/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.
Comment 3 Carlos Garcia Campos 2019-07-24 07:33:52 PDT
Created attachment 374774 [details]
Patch
Comment 4 Carlos Garcia Campos 2019-07-24 07:47:51 PDT
Created attachment 374775 [details]
Patch
Comment 5 Michael Catanzaro 2019-07-24 07:53:58 PDT
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.
Comment 6 Carlos Garcia Campos 2019-07-24 07:59:45 PDT
Created attachment 374777 [details]
Patch
Comment 7 Carlos Garcia Campos 2019-07-24 08:05:00 PDT
(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.
Comment 8 Carlos Garcia Campos 2019-07-24 08:09:00 PDT
Created attachment 374778 [details]
Patch for landing
Comment 9 Carlos Garcia Campos 2019-07-25 00:30:44 PDT
Youenn or Alex, could you check the cross-platform changes in this patch, please?
Comment 10 Carlos Garcia Campos 2019-07-26 01:25:02 PDT
Created attachment 374954 [details]
Rebased patch for landing

Ping owners
Comment 11 Carlos Garcia Campos 2019-07-26 01:42:34 PDT
Created attachment 374955 [details]
Patch for landing
Comment 12 youenn fablet 2019-07-29 09:15:48 PDT
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.
Comment 13 youenn fablet 2019-07-29 09:19:54 PDT
I'll fix the potential use-after-move in a separate bug.
Comment 14 Carlos Garcia Campos 2019-07-30 07:36:49 PDT
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.
Comment 15 Carlos Garcia Campos 2019-07-30 07:38:41 PDT
(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.
Comment 16 Carlos Garcia Campos 2019-07-30 07:41:10 PDT
Created attachment 375157 [details]
Patch for landing
Comment 17 Carlos Garcia Campos 2019-07-30 09:31:12 PDT
Committed r248010: <https://trac.webkit.org/changeset/248010>