[SOUP] Simplify cookie storage handling

• Component: Platform
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

We currently have a global cookie storage, and several create() methods in SoupNetworkSession to create sessions with different cookie jars. This could be simplified by moving the cookie storage handling to NetworkStorageSession and removing all create() methods from SoupNetworkSession.

[Carlos Garcia Campos]

Created attachment 298668 [details]
Patch

[Carlos Garcia Campos]

Created attachment 298670 [details]
Try to fix EFL build

[Sergio Villar Senin]

Comment on attachment 298670 [details]
Try to fix EFL build

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

Nice!

> Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:87
> +    // FIXME: Creating a testing session is losing soup session values set when initializing the network process.

Is this comment still valid?

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 298670 [details]
> Try to fix EFL build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298670&action=review
> 
> Nice!

Thanks!

> > Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:87
> > +    // FIXME: Creating a testing session is losing soup session values set when initializing the network process.
> 
> Is this comment still valid?

Yes, that's what bug #166969 fixes on top of this patch.

[Michael Catanzaro]

(I want to review this too.)

[Michael Catanzaro]

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.

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

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

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

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.

[Carlos Garcia Campos]

(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 session, all others are created with a SoupNetworkSession.

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

[Carlos Garcia Campos]

Committed r210729: <http://trac.webkit.org/changeset/210729>

[Michael Catanzaro]

(In reply to comment #7)
> 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).

OK, that makes sense to me now. The problem I see here is that nothing prevents you from accidentally creating a SoupNetworkSession in the web process or UI process. Indeed, it seems easier than before because just calling NetworkStorageSession::soupNetworkSession has the effect of creating a new SoupNetworkSession. Is there some time when a SoupSession really is needed in the web process? If so, we should add a FIXME to change that, because it prevents future sandboxing. If not, can you add an assertion to ensure NetworkStorageSession::soupNetworkSession is only ever called from the network process?

> 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 session, all others are created with a
> SoupNetworkSession.

OK, I think you should indeed clear the NetworkStorageSession jar and have it return the SoupNetworkSession jar instead. Even if it's the same thing, that would make the code clearer and more robust.

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

I think you should still add some function that returns SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY and use it in the different places. That was the default setting can be changed at just one point if we want to change it in the future.

[Carlos Garcia Campos]

(In reply to comment #9)
> (In reply to comment #7)
> > 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).
> 
> OK, that makes sense to me now. The problem I see here is that nothing
> prevents you from accidentally creating a SoupNetworkSession in the web
> process or UI process. Indeed, it seems easier than before because just
> calling NetworkStorageSession::soupNetworkSession has the effect of creating
> a new SoupNetworkSession. Is there some time when a SoupSession really is
> needed in the web process? If so, we should add a FIXME to change that,
> because it prevents future sandboxing. If not, can you add an assertion to
> ensure NetworkStorageSession::soupNetworkSession is only ever called from
> the network process?

Nothing prevents you from calling SoupNetworkSession::defaultSession() from any process. Unfortunately we haven't moved all the network to the network process yet, we still download the appcache manifest in the web process for example. We also use ResourceHandle to downloads fragments of HLS streams.

[Michael Catanzaro]

(In reply to comment #9)
> > 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 session, all others are created with a
> > SoupNetworkSession.
> 
> OK, I think you should indeed clear the NetworkStorageSession jar and have
> it return the SoupNetworkSession jar instead. Even if it's the same thing,
> that would make the code clearer and more robust.

Um, I see the current code already returns the SoupNetworkSession's pointer to the jar. I think it should also clear the NetworkStorageSession's pointer to the jar, for clarity.

[Michael Catanzaro]

(In reply to comment #11)
> Um, I see the current code already returns the SoupNetworkSession's pointer
> to the jar. I think it should also clear the NetworkStorageSession's pointer
> to the jar, for clarity.

Um, that's what it does. I'm dumb? OK then.