[Webkit-unassigned] [Bug 166967] [SOUP] Simplify cookie storage handling

[bugzilla-daemon at webkit.org]

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

--- Comment #7 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #6)
> Comment on attachment 298670 [details]
> Try to fix EFL build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298670&action=review
> 
> I have a couple questions.
> 
> Also, please look at bug #166029.
> 
> > Source/WebCore/ChangeLog:11
> > +        NetworkStorageSession and removing all create() methods from SoupNetworkSession. This patch also removes the
> > +        default SoupNetworkSession in favor of using the default NetworkStorageSession.
> 
> That's a big change to hide in a patch named "Simplify cookie storage
> handling". Then again, I'm probably not one to complain, having just tried
> to sneak WebKitSecurityOrigin into a notifications patch... but consider
> landing in two parts if you have time.

I tried to do it separately at first, but I ended up doing it together because it was not that easy. I don't think it's such a big change, though, we currently have two ways to get the default soup session, and this is removing one of them. So, it's indeed a simplification.

> But I'm curious: what was the motivation for this change?

Simplification and avoid misusing the SoupNetworkSession::defaultSession() when you don't really want the default, but the current session one.

Same happens for the cookies, we have default cookies storage in different places, you can use soupCookieJar() or SoupNetworkSession::defaultSession().cookieJar(). That's quite confusing, and it's not exactly the same thing.

> > Source/WebCore/ChangeLog:41
> > +        (WebCore::NetworkStorageSession::cookieStorage): Return the cookie sotrage from the SoupNetworkSession if we
> 
> sotrage -> storage
> 
> > Source/WebCore/platform/network/soup/DNSSoup.cpp:82
> > -    g_object_get(SoupNetworkSession::defaultSession().soupSession(), "proxy-resolver", &resolver.outPtr(), nullptr);
> > +    g_object_get(NetworkStorageSession::defaultStorageSession().soupNetworkSession().soupSession(), "proxy-resolver", &resolver.outPtr(), nullptr);
> 
> Shame it's a lot more typing to get the SoupNetworkSession for the default
> NetworkStorageSession than it was to just get the default
> SoupNetworkSession. Hence why I'm curious to know your reasoning for
> changing this.

This is not a matter of typing, that's not what i wanted to simplify, but if that's a problem we can always add a convenient getter in NetworkStorageSession to the the SoupSession directly.

Again the reasoning is to have only one way to get the default session, and it's from the NetworkStorageSession. The same for the cookie storage.

> > Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:110
> > +SoupCookieJar* NetworkStorageSession::cookieStorage() const
> > +{
> > +    if (m_session) {
> > +        ASSERT(m_session->cookieJar());
> > +        return m_session->cookieJar();
> > +    }
> > +
> > +    if (!m_cookieStorage) {
> > +        m_cookieStorage = adoptGRef(soup_cookie_jar_new());
> > +        soup_cookie_jar_set_accept_policy(m_cookieStorage.get(), SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY);
> > +    }
> > +    return m_cookieStorage.get();
> >  }
> 
> What? What is the advantage of having the NetworkStorageSession and
> SoupNetworkSession store separate pointers to the SoupCookieJar? It looks
> like you intentionally allow a SoupNetworkSession to "override" the
> SoupCookieJar of the NetworkStorageSession. Why is that?

Note that the behavior hasn't changed here, except for one thing. In current code we have:

soupCookieJar() -> The default shared cookie storage
SoupNetworkSession::defaultSession() -> The default shared cookie storage

It's the same, this is what the current code does:

SoupNetworkSession::defaultSession().setCookieJar(jar.get());
WebCore::setSoupCookieJar(jar.get());

So, it indeed has to be set twice. The patch is doing the opposite, it makes NetworkStorageSession::cookieStorage() the only way to get the cookie jar. Of course you can use SoupNetworkSession::cookieJar(), but you know that's the jar used by that particular soup session. 

Note that the default NetworkStorageSession is always created without a SoupNetworkSession, which is specially interesting in the web process because most of the times we don't even need a soup session at all there. If you ask for the cookie storage of the default NetworkStorageSession we don't need to have a default SoupNetworkSession at all, the same way it happens with current code when you call soupCookieJar(). And the only difference in behavior is that now the default SoupNetworkSession is created on demand when asked to the default NetworkStorageSession and the default cookie storage is set in that case, but only stored in NetworkStorageSession (of course the feature is set in the soup session). It's true that for every session, if the cookie storage is asked before the soup session is created, when the soup session is created we have two references of the cookie jar, we could probably clear the NetworkStorageSession one in that case. That can only happen for the default sessi

> This has a serious disadvantage that you have to create a SoupCookieJar in
> two different places, and now have the default accept policy hardcoded in
> two different places. If you really want to do this, then you should
> introduce some global defaultCookieAcceptPolicy() function somewhere that
> returns SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY. But even avoiding that
> problem, I don't understand why you've done this. That the implementation of
> what should be a simple const getter function is so convoluted should be a
> warning sign.

Again, no change here. In current code there are 2 places where a cookie jar is set and default policy is hardcoded:

soupCookieJar()
createPrivateBrowsingCookieJar()

We have 3 different create methods for SoupNetworkSession where the only difference is the cookie jar passed to the constructor, using createPrivateBrowsingCookieJar() which is confusing because it's exactly the same as the shared one, the cookie jar itself has nothing of private. So, this is simplified in this patch by removing all create() methods, and simply allowing a nullptr jar in the constructor, meaning don't use the given one (the shared one normally), but create your own.

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