Bug 35942 – [GTK] Fails http/tests/plugins/third-party-cookie-accept-policy.html

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Gustavo Noronha (kov)]

Apparently we're accepting to set a cookie when we should not. Likely a soup bug?

[Gustavo Noronha (kov)]

Skipped in r55744.

[Dan Winship]

Created attachment 50363 [details]
fix

"Likely a soup bug?" :-P !

[WebKit Review Bot]

Attachment 50363 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/547002

[Adam Barth]

Comment on attachment 50363 [details]
fix

What does SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY mean?  Getting the proper behavior here (i.e., matching new-Safari) is somewhat subtle.

What cookie policy does Gtk want by default?  Is that cookie policy configurable by clients?

[Adam Barth]

Adding Brady because he added this test.

[Xan Lopez]

(In reply to comment #4)
> (From update of attachment 50363 [details])
> What does SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY mean?  Getting the proper
> behavior here (i.e., matching new-Safari) is somewhat subtle.
> 
> What cookie policy does Gtk want by default?  Is that cookie policy
> configurable by clients?

It means the cookie jar will only accept cookies coming from the "first party", which is supposed to be the URI of the main document we are loading. To figure out who is who the application is supposed to set for each HTTP request its first party URI, so that libsoup can know decide if it's third party or not (by comparing it with the resource URI, which obviously must be set).

About the patch, I'm not sure why it's stopping calling setDefaultCookieJar on the jar it creates, not sure that's right.

[Dan Winship]

(In reply to comment #4)
> (From update of attachment 50363 [details])
> What does SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY mean?  Getting the proper
> behavior here (i.e., matching new-Safari) is somewhat subtle.
> 
> What cookie policy does Gtk want by default?  Is that cookie policy
> configurable by clients?

Sorry, didn't explain in detail because I figured Xan or Gustavo would be the one reviewing it.

The issue here is that the cookie jar returned by defaultCookieJar() is really only used by GtkLauncher and DumpRenderTree, because "real" apps (epiphany, midori, etc) will call setDefaultCookieJar() to override it. So while epiphany and midori were already providing a configurable no-third-party-cookie option, the tests were still getting run with an accept-all-cookies policy.

(In reply to comment #6)
> About the patch, I'm not sure why it's stopping calling setDefaultCookieJar on
> the jar it creates, not sure that's right.

If you look at the old code, if you called defaultCookieJar(), and then called setDefaultCookieJar() with a different cookie jar, the original default would get leaked. The change makes it not leak.

(In reply to comment #3)
> Attachment 50363 [details] did not build on gtk:
> Build output: http://webkit-commit-queue.appspot.com/results/547002

> checking for LIBSOUP_2_29_90... no
> ...
> ../../WebCore/platform/network/soup/CookieJarSoup.cpp:41: error: ‘soup_cookie_jar_set_accept_policy’ was not declared in this scope

so, I guess the build would be fixed by adding some #ifdef HAVE_LIBSOUP_2_29_90 to the patch, but assuming that this is the build that would get used for the tests, the test would still fail because it was using a pre-cookie-policy version of libsoup. So I don't think we can unskip the test until the build/test machines have a newer libsoup.

[Xan Lopez]

(In reply to comment #7)
> (In reply to comment #6)
> > About the patch, I'm not sure why it's stopping calling setDefaultCookieJar on
> > the jar it creates, not sure that's right.
> 
> If you look at the old code, if you called defaultCookieJar(), and then called
> setDefaultCookieJar() with a different cookie jar, the original default would
> get leaked. The change makes it not leak.

Oh, I see, because it has one extra reference right? OK. I was confused because I thought you meant the function didn't try to release it, but that it does.

> 
> (In reply to comment #3)
> > Attachment 50363 [details] [details] did not build on gtk:
> > Build output: http://webkit-commit-queue.appspot.com/results/547002
> 
> > checking for LIBSOUP_2_29_90... no
> > ...
> > ../../WebCore/platform/network/soup/CookieJarSoup.cpp:41: error: ‘soup_cookie_jar_set_accept_policy’ was not declared in this scope
> 
> so, I guess the build would be fixed by adding some #ifdef HAVE_LIBSOUP_2_29_90
> to the patch, but assuming that this is the build that would get used for the
> tests, the test would still fail because it was using a pre-cookie-policy
> version of libsoup. So I don't think we can unskip the test until the
> build/test machines have a newer libsoup.

Keep in mind that the bots without a new libsoup are the EWS ones, our own GTK+ bots do have a recent enough libsoup.

[Dan Winship]

Created attachment 50396 [details]
updated fix which should be out-of-date-build-bot-safe

This just adds an "#ifdef HAVE_LIBSOUP_2_29_90" to fix the build on the EWS bots.

[Gustavo Noronha (kov)]

(In reply to comment #2)
> Created an attachment (id=50363) [details]
> fix
> 
> "Likely a soup bug?" :-P !

\o/ my bad

Another option is to have DRT set the policy, but I agree with making this the default.

[Gustavo Noronha (kov)]

Comment on attachment 50396 [details]
updated fix which should be out-of-date-build-bot-safe

Looks good to me, thanks!

[WebKit Commit Bot]

Comment on attachment 50396 [details]
updated fix which should be out-of-date-build-bot-safe

Rejecting patch 50396 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Gustavo Noronha Silva', '--force']" exit_code: 1
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/platform/gtk/Skipped
Hunk #1 FAILED at 5821.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/gtk/Skipped.rej
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/platform/network/soup/CookieJarSoup.cpp

Full output: http://webkit-commit-queue.appspot.com/results/613031

[Gustavo Noronha (kov)]

Landed as 55894.

[Adam Barth]

I still don't understand this change.  If it's meant just for DRT, then we should have DRT set the policy.

[Gustavo Noronha (kov)]

(In reply to comment #14)
> I still don't understand this change.  If it's meant just for DRT, then we
> should have DRT set the policy.

Let me try to clarify, then. We decided that the default cookie jar for WebKitGTK+ will have the "no third party" policy set. Most non-trivial applications will override the default cookie jar, and set their own policy (Epiphany already does this, for instance).

We thought this would be enough, but there are clearly some tests that need to set third party cookies, so we also implemented the DRT infrastructure to set the cookies acceptance policy: (see https://bugs.webkit.org/show_bug.cgi?id=36056).

The process was a bit convoluted, mostly because I failed to do a more appropriate research on how this was handled by other ports (mea culpa there), but I think we got what we need: our default policy is set to the value we want, applications can override it, the layoutTestController infrastructure is implemented, and is being used by DRT.

[Adam Barth]

IMHO, having the default cookie policy be to block third-party cookies is a mistake.  Blocking third-party cookies is a giant hack that doesn't really buy you much except complexity and flakiness in the web platform.