Propagate the "first party for cookies" from WebKit through the resource...

Propagate the "first party for cookies" from WebKit to the network stack
when we follow a redirect, because WebKit's
MainResourceLoader::willSendRequest method may change the "first party for
cookies" URL of the resource request.

R=abarth
BUG=25133
TEST=In Options menu, change cookie policy to "Accept cookies only from
sites I visit" and then follow the instructions in issue 25133 comment 20.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=31951

[wtc, 2009-11-11 00:13:31]

Here is the second solution: propagate "first party for cookies"
from WebKit, through the resource dispatcher host, to the
network stack.  The first solution is
http://codereview.chromium.org/387013.  Please let me know
which approach you prefer, and I'll add the unit test.

In some way I prefer this solution, which gets the first
party for cookies from WebKit, the authoritative source,
rather than relying on a heuristic in URLRequest.  But this
solution is so much bigger!

When you review this CL for the first time, just read the
following files:
  resource_dispatcher_host.{h,cc}
  resource_message_internal.h
  resource_dispatcher.cc
  url_request.cc
  resource_loader_bridge.h
  weburlloader_impl.cc

Please read resource_dispatcher.cc and see if there is a
better way to get the first party for cookies in
ResourceDispatcher::OnReceivedRedirect.  I get that info
from request_info.peer, but that requires adding a
GetFirstPartyForCookies method to the
ResourceLoaderBridge::Peer interface, which causes all
classes that implement that interface to change.  I hope
there is a better way to get the first party for cookies
in ResourceDispatcher::OnReceivedRedirect.

[wtc, 2009-11-11 00:13:49]

Reviewers: abarth,

Message:
Here is the second solution: propagate "first party for cookies"
from WebKit, through the resource dispatcher host, to the
network stack.  The first solution is
http://codereview.chromium.org/387013.  Please let me know
which approach you prefer, and I'll add the unit test.

In some way I prefer this solution, which gets the first
party for cookies from WebKit, the authoritative source,
rather than relying on a heuristic in URLRequest.  But this
solution is so much bigger!

When you review this CL for the first time, just read the
following files:
   resource_dispatcher_host.{h,cc}
   resource_message_internal.h
   resource_dispatcher.cc
   url_request.cc
   resource_loader_bridge.h
   weburlloader_impl.cc

Please read resource_dispatcher.cc and see if there is a
better way to get the first party for cookies in
ResourceDispatcher::OnReceivedRedirect.  I get that info
from request_info.peer, but that requires adding a
GetFirstPartyForCookies method to the
ResourceLoaderBridge::Peer interface, which causes all
classes that implement that interface to change.  I hope
there is a better way to get the first party for cookies
in ResourceDispatcher::OnReceivedRedirect.

Description:
Propagate the "first party for cookies" from WebKit through the resource
dispatcher host to the network stack when we follow a redirect, because
WebKit's MainResourceLoader::willSendRequest may change the "first party
for cookies" of the resource request.

R=abarth
BUG=25133
TEST=In Options menu, change cookie policy to "Accept cookies only from
sites I visit" and then follow the instructions in issue 25133 comment 20.

Please review this at http://codereview.chromium.org/385024

SVN Base: svn://chrome-svn/chrome/trunk/src/

Affected files:
   M     chrome/browser/renderer_host/resource_dispatcher_host.h
   M     chrome/browser/renderer_host/resource_dispatcher_host.cc
   M     chrome/browser/renderer_host/safe_browsing_resource_handler.cc
   M     chrome/common/render_messages_internal.h
   M     chrome/common/resource_dispatcher.cc
   M     chrome/common/security_filter_peer.h
   M     chrome/common/security_filter_peer.cc
   M     chrome/plugin/chrome_plugin_host.cc
   M     net/url_request/url_request.cc
   M     webkit/glue/media/buffered_data_source.h
   M     webkit/glue/media/simple_data_source.h
   M     webkit/glue/media/simple_data_source.cc
   M     webkit/glue/resource_loader_bridge.h
   M     webkit/glue/weburlloader_impl.cc

[abarth-chromium, 2009-11-11 08:35:16]

Unfortunately, this is a better way to get the information.  Only WebKit knows
what the first party for cookies is.  Making the change in URLRequest is a
layering violation (unfortunately).  I don't know of a better way to get the
information out of WebKit than what you're doing, but I'm not that familiar with
this part of the code.  You might want to ask Darin.

[abarth-chromium, 2009-11-11 08:35:35]

Unfortunately, this is a better way to get the information.  Only WebKit  
knows
what the first party for cookies is.  Making the change in URLRequest is a
layering violation (unfortunately).  I don't know of a better way to get the
information out of WebKit than what you're doing, but I'm not that familiar  
with
this part of the code.  You might want to ask Darin.

http://codereview.chromium.org/385024

[wtc, 2009-11-11 23:06:51]

darin is on vacation, so I asked eroman to review this CL.

eroman suggested that instead of adding the
GetFirstPartyForCookies method to the
ResourceLoaderBridge::Peer interface, we can add an output
parameter "GURL* first_party_for_cookies" to the
OnReceivedRedirect method.  I think this is a good idea.

[eroman, 2009-11-12 10:24:37]

Overall this looks good.
My main comments are on the organization of the code, and testing.

Testing:

This is a pretty important change, and I feel it should be accompanied by some
tests.
While I will not block checking this in on tests, please file a follow-up bug if
you aren't going to do them right away.

As I see it, there are two good testing strategies for this:

  (a) layout tests
  (b) ui tests

I would recommend (b) since:

  (1) The interesting bits of this change are in the webkit integration. A
layout test would give us coverage of the test_shell paths which are very
different than the chrome IPC ones, and at the end of the day we want to be
verifying chrome.

  (2) You will likely find it easier to write a ui test. For one thing you wont'
have any problems getting different domains for the redirect since we mock the
host resolving. Whereas with layout tests you would have to use localhost for
the domains, and can only vary the port.

http://codereview.chromium.org/385024/diff/47/6024
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/385024/diff/47/6024#newcode838
Line 838: const GURL& first_party_for_cookies) {
nit: Per my usual, I suggest using a ridiculously long name:
new_first_party_for_cookies, to emphasize that this represents a *change*.

http://codereview.chromium.org/385024/diff/47/6024#newcode846
Line 846: if (first_party_for_cookies.is_valid()) {
I am not really excited about overloading !is_valid() to mean "don't update",
since it prevents it being used as an actual policy URL.

I would suggest having the parameter be a |const GURL*| instead, and using NULL
to indicate "don't update".

http://codereview.chromium.org/385024/diff/47/6024#newcode848
Line 848: // the DCHECK(!is_pending_) assertion from
set_first_party_for_cookies().
This comment won't make much sense after this check-in ... I wouldn't mention
it.

http://codereview.chromium.org/385024/diff/47/6025
File chrome/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/385024/diff/47/6025#newcode147
Line 147: const GURL& first_party_for_cookies);
Can you add an explanation in the comment for what |first_party_for_cookies| is,
and what values it can take.

For example that invalid GURL has special meaning.

http://codereview.chromium.org/385024/diff/47/6030
File chrome/plugin/chrome_plugin_host.cc (right):

http://codereview.chromium.org/385024/diff/47/6030#newcode126
Line 126: NOTREACHED();
These null implementations of GetFirstPartyForCookies() are a bit of an eye-sore
... Please see my suggestion on getting rid of Peer::GetFirstPartyForCookies().

http://codereview.chromium.org/385024/diff/47/6021
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/385024/diff/47/6021#newcode407
Line 407: client_->willSendRequest(loader_, new_request, response);
This reordering is a pretty subtle change, why is it needed?

http://codereview.chromium.org/385024/diff/47/6032
File webkit/tools/test_shell/simple_resource_loader_bridge.cc (right):

http://codereview.chromium.org/385024/diff/47/6032#newcode156
Line 156: if (peer_ && peer_->OnReceivedRedirect(new_url, info)) {
Rather than adding a GetFirstPartyForCookies() method, it seems cleaner to me to
add an out parameter to OnReceivedRedirect().

I favor that approach for the following reasons:

 (1) Makes it clear to consumers that they should remember to do something about
the first party for cookies.

 (2) Code which doesn't care just does nothing (no need for NOTREACHED() and
returning GURL()).

 (3) The pattern is consistent with what we do elsewhere. See for example the
out parameter on OnReceivedRedirect() we use in the browser-side code:

  http://codereview.chromium.org/155897/diff/64/1101

 (4) [minor] Simple implementations of Peer wouldn't need to cache the policy
just so that GetFirstPartyForCookies() could return it in a separate function
call -- they would "return it" when deciding the redirect.

[wtc, 2009-11-13 02:54:44]

eroman: thanks a lot for your review!

I made all the changes you suggested except:
- adding a unit test
- using a NULL GURL* pointer instead of an empty GURL()
I will do those in follow-up CLs.  Empty URLs have a special
meaning in cookie policies, so I agree it's best to avoid
them.

I added url_request.h and buffered_data_source.cc to the CL.

http://codereview.chromium.org/385024/diff/47/6021
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/385024/diff/47/6021#newcode407
Line 407: client_->willSendRequest(loader_, new_request, response);
On 2009/11/12 10:24:37, eroman wrote:
> This reordering is a pretty subtle change, why is it needed?

The second argument to client_->willSendRequest() is an
in/out argument.  So we need to wait until we are done
modifying new_request before we do the
  request_ = new_request;
assignment, otherwise request_ may get out of date.

[eroman, 2009-11-13 03:19:52]

LGTM

thanks for digging into this!

http://codereview.chromium.org/385024/diff/6059/69
File chrome/common/resource_dispatcher_unittest.cc (right):

http://codereview.chromium.org/385024/diff/6059/69#newcode39
Line 39: *new_first_party_for_cookies = GURL();
Can you change all of these no-op implementations to simply not write to
|new_first_party_for_cookies| ?

I think it should be defined that if you don't mutate
new_first_party_for_cookies, then it means you don't want to change it.

http://codereview.chromium.org/385024/diff/6059/68
File chrome/plugin/chrome_plugin_host.cc (right):

http://codereview.chromium.org/385024/diff/6059/68#newcode75
Line 75: *new_first_party_for_cookies = GURL();
same comment.

http://codereview.chromium.org/385024/diff/6059/72
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/385024/diff/6059/72#newcode239
Line 239: *new_first_party_for_cookies = GURL();
same comment.

http://codereview.chromium.org/385024/diff/6059/56
File webkit/glue/media/simple_data_source.cc (right):

http://codereview.chromium.org/385024/diff/6059/56#newcode128
Line 128: *new_first_party_for_cookies = GURL();
same comment.

[abarth-chromium, 2009-11-13 04:23:39]

Wan-Teh, please try to get this into Mstone-4.  I think this might be the fix
for a number of mysterious bugs related to third-party cookie blocking.

[darin (slow to review), 2009-11-16 17:59:50]

http://codereview.chromium.org/385024/diff/9001/8005
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/385024/diff/9001/8005#newcode120
Line 120: GURL* new_first_party_for_cookies) = 0;
nit: down below, this is called the "policy_url" ...

it would be good to either name this new_policy_url, or rename policy_url to
first_party_for_cookies.

[wtc, 2009-11-16 20:25:57]

http://codereview.chromium.org/385024/diff/9001/8005
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/385024/diff/9001/8005#newcode120
Line 120: GURL* new_first_party_for_cookies) = 0;
On 2009/11/16 17:59:50, darin wrote:
> nit: down below, this is called the "policy_url" ...
> 
> it would be good to either name this new_policy_url, or rename policy_url to
> first_party_for_cookies.

I will rename policy_url to first_party_for_cookies in
the declaration of the Create() method below, because
the two definitions of the Create() method (in
renderer_glue.cc and simple_resource_loader_bridge.cc)
name this parameter first_party_for_cookies.

I found that both policy_url and first_party_for_cookies
are used in many places.  This inconsistency of parameter
name is unfortunate.

[darin (slow to review), 2009-11-16 22:04:56]

On Mon, Nov 16, 2009 at 12:25 PM, <wtc@chromium.org> wrote:

>
> http://codereview.chromium.org/385024/diff/9001/8005
> File webkit/glue/resource_loader_bridge.h (right):
>
> http://codereview.chromium.org/385024/diff/9001/8005#newcode120
> Line 120: GURL* new_first_party_for_cookies) = 0;
> On 2009/11/16 17:59:50, darin wrote:
>
>> nit: down below, this is called the "policy_url" ...
>>
>
>  it would be good to either name this new_policy_url, or rename
>>
> policy_url to
>
>> first_party_for_cookies.
>>
>
> I will rename policy_url to first_party_for_cookies in
> the declaration of the Create() method below, because
> the two definitions of the Create() method (in
> renderer_glue.cc and simple_resource_loader_bridge.cc)
> name this parameter first_party_for_cookies.
>
>
OK, sounds good.



> I found that both policy_url and first_party_for_cookies
> are used in many places.  This inconsistency of parameter
> name is unfortunate.
>
>

Agreed.

-Darin


>
> http://codereview.chromium.org/385024
>