[Webkit-unassigned] [Bug 200076] [SOUP] Move SoupNetworkSession ownership from NetworkStorageSession to NetworkSession

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=200076

--- Comment #7 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190724/adf5f449/attachment.html>