Bug 131026

Summary: [SOUP] Control cookie management according ResourceRequest.allowCookies()
Product: WebKit Reporter: youenn fablet <youennf>
Component: PlatformAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, bunhere, cdumez, cgarcia, commit-queue, danw, gns, gyuyoung.kim, mrobinson, sergio, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 130963    
Attachments:
Description Flags
Temp fix working for wk1 but not for wk2
none
Fixing WK2
none
Adding private session cookie policy setting
none
Improved style none

Description youenn fablet 2014-04-01 05:05:19 PDT
ResourceRequest.allowCookies() is used to control the sending and setting of cookies by the HTTP layer.
For instance, cookies are disabled for cross-origin requests for which stored credentials are not allowed.

The soup layer does not seem to handle that correctly.
One reason may be that cookies handling is set at the soup session level.

Test http/tests/xmlhttprequest/cross-origin-cookie-storage.html is probably related to that issue.
Comment 1 youenn fablet 2014-04-01 12:41:15 PDT
Created attachment 228307 [details]
Temp fix working for wk1 but not for wk2
Comment 2 youenn fablet 2014-04-01 12:52:58 PDT
The temp fix passes http/tests/xmlhttprequest/cross-origin-cookie-storage.html on WK1 (EFL and GTK) but not WK2 (EFL and GTK also).

http/tests/xmlhttprequest/cross-origin-cookie-storage.html uses testRunner.setAlwaysAcceptCookies to allow all cookies.

On WK1, the cookie jar policy of the soup session is correctly set to SOUP_COOKIE_JAR_ACCEPT_ALWAYS .
On WK2, the cookie jar policy remains to SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY, defeating the purpose of the test.
Comment 3 youenn fablet 2014-04-03 07:14:17 PDT
Created attachment 228506 [details]
Fixing WK2
Comment 4 youenn fablet 2014-04-03 07:15:50 PDT
Second patch runs successfully for GTK/WK1, GTK/WK2 and EFL/WK1.
It should also work for EFL/WK2 but I am not able to test it.
Comment 5 Sergio Villar Senin 2014-04-04 04:11:24 PDT
Comment on attachment 228506 [details]
Fixing WK2

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

> Source/WebKit2/WebProcess/Cookies/soup/WebCookieManagerSoup.cpp:64
>  }

I'm not sure about this. Is it really required for the fix?

Also checking mac's implementation, they set that accept policy also for all sessions in SessionTracker. I think we need to research a bit more on this.
Comment 6 youenn fablet 2014-04-04 06:16:36 PDT
(In reply to comment #5)
> (From update of attachment 228506 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228506&action=review
> 
> > Source/WebKit2/WebProcess/Cookies/soup/WebCookieManagerSoup.cpp:64
> >  }
> 
> I'm not sure about this. Is it really required for the fix?

It is needed to pass the test, as the following happens:
1. the default session switches to a testing session, with the creation of a private cookie jar.
2. The cookie policy is changed, changing the default cookie jar but not the cookie jar of the default session (the testing session).
 
> Also checking mac's implementation, they set that accept policy also for all sessions in SessionTracker. I think we need to research a bit more on this.

Mac implementation sets the cookie policy of the default session cookie jar, which is similar to this patch.
Mac implementation apparently has no notion of default cookie jar as for soup.

That said, it makes sense to set the accept policy to all existing sessions, including private sessions. I will upload a new patch accordingly.
Comment 7 youenn fablet 2014-04-04 06:21:17 PDT
Humm, having another look at SoupNetworkSession, is there any reason why private browsing sessions reuse the default cookie jar? Should they not create their own private cookie jar?
Comment 8 Sergio Villar Senin 2014-04-04 07:23:10 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 228506 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=228506&action=review

> Mac implementation sets the cookie policy of the default session cookie jar, which is similar to this patch.
> Mac implementation apparently has no notion of default cookie jar as for soup.

They do. Check WebFrameNetworkingContext:::setCookieAcceptPolicyForAllContexts() in mac code. They set the policy for the shared cookie jar and the default session cookie jar and then all existing sessions.
 
> That said, it makes sense to set the accept policy to all existing sessions, including private sessions. I will upload a new patch accordingly.

We should probably use the same approach as Mac port does, moving that code to the WebFrameNetworkingContext.
Comment 9 youenn fablet 2014-04-04 08:55:23 PDT
Created attachment 228595 [details]
Adding private session cookie policy setting
Comment 10 Sergio Villar Senin 2014-04-07 00:35:41 PDT
Comment on attachment 228595 [details]
Adding private session cookie policy setting

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

Thanks, I believe this is indeed the way to go. I'll let kov and mrobinson to take a look at this before giving the r+ though.

> Source/WebKit2/WebProcess/WebCoreSupport/soup/WebFrameNetworkingContext.cpp:57
> +    SoupCookieJarAcceptPolicy soupPolicy;

Nit: move cookieJar declaration and initialization after the switch as you use it there for the first time. You can also initialize soupPolicy to ACCEPT_ALWAYS here and remove the line bellow.
Comment 11 Gustavo Noronha (kov) 2014-04-07 09:24:03 PDT
Comment on attachment 228595 [details]
Adding private session cookie policy setting

This looks sane to me, yeah
Comment 12 youenn fablet 2014-04-08 00:08:02 PDT
Created attachment 228812 [details]
Improved style
Comment 13 Sergio Villar Senin 2014-04-08 04:39:49 PDT
Comment on attachment 228812 [details]
Improved style

Awesome! Thanks!
Comment 14 WebKit Commit Bot 2014-04-08 05:10:21 PDT
Comment on attachment 228812 [details]
Improved style

Clearing flags on attachment: 228812

Committed r166924: <http://trac.webkit.org/changeset/166924>
Comment 15 WebKit Commit Bot 2014-04-08 05:10:28 PDT
All reviewed patches have been landed.  Closing bug.