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.
Created attachment 228307 [details] Temp fix working for wk1 but not for wk2
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.
Created attachment 228506 [details] Fixing WK2
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 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.
(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.
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?
(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.
Created attachment 228595 [details] Adding private session cookie policy setting
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 on attachment 228595 [details] Adding private session cookie policy setting This looks sane to me, yeah
Created attachment 228812 [details] Improved style
Comment on attachment 228812 [details] Improved style Awesome! Thanks!
Comment on attachment 228812 [details] Improved style Clearing flags on attachment: 228812 Committed r166924: <http://trac.webkit.org/changeset/166924>
All reviewed patches have been landed. Closing bug.