Bug 77341 – [soup] Add support for multiple SoupSessions.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Raphael Kubo da Costa (:rakuco)]

[soup] Add support for multiple SoupSessions.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 124558 [details]
Patch

[WebKit Review Bot]

Attachment 124558 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.h:447:  Extra space between SoupSession and *session  [whitespace/declaration] [3]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Raphael Kubo da Costa (:rakuco)]

This is a WIP patch supposed to carry on the multi-SoupSession work started in bug 22624.

I'd like some feedback on the WebCore and GTK+ changes: I tried to keep the existing behavior as much as possible, so that the changes are transparent to users. The WebKit2 part has not been touched, as there currently does not seem to be an easy way to pass SoupSessions around between processes (and the CookieJar calls called by WK2 assume there is only one cookie jar).

I'm not sure if there are ownership and lifetime issues in the way I'm using the SoupSession or WebKitWeb{Frame,View} pointers, so help in this part is also appreciated.

[Sergio Villar Senin]

May I ask what is this change required for?

[Gustavo Noronha (kov)]

Comment on attachment 124558 [details]
Patch

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

Interesting. I'm not sure we would like to use that API for WebKitGTK+ though, we may have to discuss it a bit more, I think it's probably material for wk2 in any case - where we have the WebContext object which we could use to split sessions instead of having the API user assign sessions to each webview.

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:82
> -    SoupCookieJar* jar = defaultCookieJar();
> +    const NetworkingContext* context = networkingContext(document);
> +    if (!context)
> +        return;
> +

Shouldn't you use the default cookie jar in case no network context exists? It's a bit weird that you have a networkingContext method that you only use to obtain the cookie jar, I'd recommend going straight to the jar.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:163
> +    else {
> +        if (session == ResourceHandle::defaultSession())

Make this an else if and do away with the braces =)

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #4)
> May I ask what is this change required for?

I'm presently doing some downstream work using webkit-efl in which I need different parts using the port to use different SoupSessions, so that they do not share cookie jars, custom soup request handlers, callbacks to SoupSession signals etc.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #5)
> (From update of attachment 124558 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124558&action=review
> 
> Interesting. I'm not sure we would like to use that API for WebKitGTK+ though, we may have to discuss it a bit more, I think it's probably material for wk2 in any case - where we have the WebContext object which we could use to split sessions instead of having the API user assign sessions to each webview.

Would you like me to just make NetworkingContextGtk::soupSession() return ResourceHandle::defaultSession() for now while we put some thought on the GTK+ part?
 
> > Source/WebCore/platform/network/soup/CookieJarSoup.cpp:82
> > -    SoupCookieJar* jar = defaultCookieJar();
> > +    const NetworkingContext* context = networkingContext(document);
> > +    if (!context)
> > +        return;
> > +
> 
> Shouldn't you use the default cookie jar in case no network context exists?

AFAICS, the only way for no NetworkContext to exist is by passing 0 to ResourceHandle::create(), which is only done by webkit_download_start. If there is a Document, there should be a FrameNetworkingContext (I guess).

> It's a bit weird that you have a networkingContext method that you only use to obtain the cookie jar, I'd recommend going straight to the jar.

Can you elaborate? What do you mean by "straight to the jar" in this context?

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:163
> > +    else {
> > +        if (session == ResourceHandle::defaultSession())
> 
> Make this an else if and do away with the braces =)

Will do.

[Gyuyoung Kim]

Comment on attachment 124558 [details]
Patch

Attachment 124558 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11367618

[Gustavo Noronha (kov)]

Comment on attachment 124558 [details]
Patch

Attachment 124558 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11365660

[Gustavo Noronha (kov)]

(In reply to comment #7)
> Would you like me to just make NetworkingContextGtk::soupSession() return ResourceHandle::defaultSession() for now while we put some thought on the GTK+ part?

I think so, yeah, we'll have to think about this feature a bit more.

> > Shouldn't you use the default cookie jar in case no network context exists?
> 
> AFAICS, the only way for no NetworkContext to exist is by passing 0 to ResourceHandle::create(), which is only done by webkit_download_start. If there is a Document, there should be a FrameNetworkingContext (I guess).

Right, but downloads started with webkit_download_start() used to have access to the default cookie jar, so they should keep having, right? =)
 
> > It's a bit weird that you have a networkingContext method that you only use to obtain the cookie jar, I'd recommend going straight to the jar.
> 
> Can you elaborate? What do you mean by "straight to the jar" in this context?

I mean instead of having a method that is given the document and obtains the context and then from that you get the jar you can have a method which is given a document and returns the jar (returning the default jar if there's no context).

[Raphael Kubo da Costa (:rakuco)]

Created attachment 124753 [details]
Appease the bots and do not add new API to the GTK+ port

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #10)
> (In reply to comment #7)
> > > Shouldn't you use the default cookie jar in case no network context exists?
> > 
> > AFAICS, the only way for no NetworkContext to exist is by passing 0 to ResourceHandle::create(), which is only done by webkit_download_start. If there is a Document, there should be a FrameNetworkingContext (I guess).
> 
> Right, but downloads started with webkit_download_start() used to have access to the default cookie jar, so they should keep having, right? =)

As discussed on IRC, this part of the CookieJar API is only used via Document, which doesn't exist if ResourceHandle::create(0, ...) is used, as no NetworkingContext (and no Frame) is used.

[WebKit Review Bot]

Attachment 124753 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
   ab43d00..676824a  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 106362 = ab43d00719700a5da4bc2b60d447b2153c0dbe70
r106363 = 676824ad52096f0bc058e283b7f9071a227cd7a6
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Comment on attachment 124753 [details]
Appease the bots and do not add new API to the GTK+ port

Attachment 124753 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11386040

[Raphael Kubo da Costa (:rakuco)]

Created attachment 125648 [details]
Drop some USE(SOUP) checks now that it depends on bug 77874

[Gyuyoung Kim]

Comment on attachment 125648 [details]
Drop some USE(SOUP) checks now that it depends on bug 77874

Attachment 125648 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11434376

[Leandro Pereira]

Created attachment 126802 [details]
Same as 125648; resubmitted to workaround flaky EWS

Resubmitting patch to workaround flaky EWS.

[Gustavo Noronha (kov)]

Comment on attachment 126802 [details]
Same as 125648; resubmitted to workaround flaky EWS

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

It looks sane to me and shouldn't change anything for GTK+ as long as we discussed, I'm still not sure about the static networkingContext() static function, see bellow:

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:50
> +static NetworkingContext* networkingContext(const Document* document)
> +{
> +    if (!document)
> +        return 0;
> +    const Frame* frame = document->frame();
> +    if (!frame)
> +        return 0;
> +    const FrameLoader* loader = frame->loader();
> +    if (!loader)
> +        return 0;
> +    return loader->networkingContext();
> +}

I still find it very odd that you have this static function to return the networkingContext and in every call you make you just check for null and gets the soup session. Why don't you rename this cookieJarForDocument(const Document* document), null check and get the soup session, and return the cookie jar from it?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #18)
> I still find it very odd that you have this static function to return the networkingContext and in every call you make you just check for null and gets the soup session. Why don't you rename this cookieJarForDocument(const Document* document), null check and get the soup session, and return the cookie jar from it?

You're right, that makes total sense -- I probably just looked at how the NetworkingContext was obtained in platform/qt/CookieJarQt.cpp and did the same without giving it too much thought.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 127192 [details]
Obtain the cookie jar in a saner way in CookieJarSoup.cpp

[WebKit Review Bot]

Attachment 127192 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2

Updating OpenSource
git.webkit.org[0: 17.254.20.231]: errno=Connection refused
fatal: unable to connect a socket (Connection refused)
Died at Tools/Scripts/update-webkit line 162.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Comment on attachment 127192 [details]
Obtain the cookie jar in a saner way in CookieJarSoup.cpp

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

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:52
> +    return SOUP_COOKIE_JAR(soup_session_get_feature(context->soupSession(), SOUP_TYPE_COOKIE_JAR));

Hmm, this could return NULL, so better null check the return of soup_session_get_feature, and only do cast/return after that; looks fine otherwise.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:160
> +        SoupCookieJar* jar = SOUP_COOKIE_JAR(soup_session_get_feature(session, SOUP_TYPE_COOKIE_JAR));

Same here. This code was probably emitting warnings we just haven't seen them in any of our usual suspects.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 127230 [details]
Patch for landing

[Raphael Kubo da Costa (:rakuco)]

Created attachment 127232 [details]
Patch for landing with reviewer set

[Raphael Kubo da Costa (:rakuco)]

Created attachment 127269 [details]
Patch for landing with reviewer, third time is a charm

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #22)
> (From update of attachment 127192 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127192&action=review
> 
> > Source/WebCore/platform/network/soup/CookieJarSoup.cpp:52
> > +    return SOUP_COOKIE_JAR(soup_session_get_feature(context->soupSession(), SOUP_TYPE_COOKIE_JAR));
> 
> Hmm, this could return NULL, so better null check the return of soup_session_get_feature, and only do cast/return after that; looks fine otherwise.
> 
> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:160
> > +        SoupCookieJar* jar = SOUP_COOKIE_JAR(soup_session_get_feature(session, SOUP_TYPE_COOKIE_JAR));
> 
> Same here. This code was probably emitting warnings we just haven't seen them in any of our usual suspects.

For posterity: after talking to kov and Bejanmin Otte on #webkitgtk+, we've decided to ignore these, as SOUP_COOKIE_JAR(0) just returns 0 and doesn't emit any warning. I've submitted a patch without the suggested changes.

[Raphael Kubo da Costa (:rakuco)]

Committed r107854: <http://trac.webkit.org/changeset/107854>

[WebKit Review Bot]

Attachment 127232 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: dataTransfer.types (HTML5 drag & drop) should return DOMStringList
Using index info to reconstruct a base tree...
<stdin>:84: trailing whitespace.
        
<stdin>:333: trailing whitespace.
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
<stdin>:334: trailing whitespace.
 width="300" height="300" onload="runRepaintTest()">
<stdin>:335: trailing whitespace.
<script xlink:href="../../fast/repaint/resources/repaint.js" />
<stdin>:336: trailing whitespace.
<script><![CDATA[
warning: squelched 16 whitespace errors
warning: 21 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 dataTransfer.types (HTML5 drag & drop) should return DOMStringList

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.