Move first-party cookie URL logic to browser process.

Move first-party cookie URL logic to browser process.

Rather than have the renderer decide the value and inform the browser, the
browser decides and informs the render.

This removes one piece of navigation request redirect policy that needs to come
from the renderer. Redundant logic in Blink's DocumentLoader::willSendRequest to
be removed in a follow-up.

BUG=368813
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270463

[davidben, 2014-05-05 23:32:41]

This may be premature until browser process navigation plans are mapped out
more, but the try jobs are finally happy with it, so I may as well send this out
now.

https://codereview.chromium.org/264613006/diff/110001/content/browser/loader/...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/264613006/diff/110001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:164: // possible this is still
needed while renderer-owned ones exist.
I actually wouldn't be too surprised if this could be dropped right now, but I'm
not sure. It depends on if any renderer-side consumer of a MAIN_FRAME request
reads firstPartyFromCookies(). If we don't do this, the value in the renderer
will be wrong, but it's possible nothing reads it.

This is probably safer until we make it so that renderers never see a pre-commit
MAIN_FRAME request so that renderers don't contain known-wrong state about a
request.

I can also look at all the callers and verify that nothing cares.

[Charlie Reis, 2014-05-08 00:53:20]

Sorry I missed this!  Must have gotten buried in my inbox during yesterday's big
day of meetings.  I'll take a look tomorrow.

[Charlie Reis, 2014-05-08 20:43:20]

I like this.

Are we sure that anything that gets a firstPartyCookie update in
DocumentLoader::willSendRequest also goes through CrossSiteResourceHandler?  I
wonder about same-site navigations and navigations that might not make network
requests (e.g., data:, file:, etc), but perhaps those either don't change the
cookie URL or still hit that path.

Also, have you found which tests would catch a bug if we did this wrong (e.g.,
if we left out the CSRH change)?

https://codereview.chromium.org/264613006/diff/110001/content/browser/loader/...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/264613006/diff/110001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:164: // possible this is still
needed while renderer-owned ones exist.
On 2014/05/05 23:32:41, David Benjamin wrote:
> I actually wouldn't be too surprised if this could be dropped right now, but
I'm
> not sure. It depends on if any renderer-side consumer of a MAIN_FRAME request
> reads firstPartyFromCookies(). If we don't do this, the value in the renderer
> will be wrong, but it's possible nothing reads it.
> 
> This is probably safer until we make it so that renderers never see a
pre-commit
> MAIN_FRAME request so that renderers don't contain known-wrong state about a
> request.
> 
> I can also look at all the callers and verify that nothing cares.

I don't know the uses of that value myself, so it might be nice to check whether
it's used in that way.  Of course, that could be done for a second CL to remove
this.

[davidben, 2014-05-09 22:08:04]

> Are we sure that anything that gets a firstPartyCookie update in
> DocumentLoader::willSendRequest also goes through CrossSiteResourceHandler?  I
> wonder about same-site navigations and navigations that might not make network
> requests (e.g., data:, file:, etc), but perhaps those either don't change the
> cookie URL or still hit that path.

I think so? Neither of those can make redirects anyway. (Or set cookies for that
matter.)

> Also, have you found which tests would catch a bug if we did this wrong (e.g.,
> if we left out the CSRH change)?

CookiePolicyBrowserTest.AllowFirstPartyCookiesRedirect blows up. I haven't
checked which others do.

https://codereview.chromium.org/264613006/diff/110001/content/browser/loader/...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/264613006/diff/110001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:164: // possible this is still
needed while renderer-owned ones exist.
On 2014/05/08 20:43:21, Charlie Reis wrote:
> On 2014/05/05 23:32:41, David Benjamin wrote:
> > I actually wouldn't be too surprised if this could be dropped right now, but
> I'm
> > not sure. It depends on if any renderer-side consumer of a MAIN_FRAME
request
> > reads firstPartyFromCookies(). If we don't do this, the value in the
renderer
> > will be wrong, but it's possible nothing reads it.
> > 
> > This is probably safer until we make it so that renderers never see a
> pre-commit
> > MAIN_FRAME request so that renderers don't contain known-wrong state about a
> > request.
> > 
> > I can also look at all the callers and verify that nothing cares.
> 
> I don't know the uses of that value myself, so it might be nice to check
whether
> it's used in that way.  Of course, that could be done for a second CL to
remove
> this.

Part of the use is for the DCHECK, but now that the DCHECK has cleared the bots,
I can remove both the DCHECK and this bit.

I'm tempted to err on the side of not having wrong data in the renderer, just in
case we miss something; after the rest of the work is done, we'll know we can
remove it because the only case that changes the URL is a top-level navigation
request. If we get to the point that such a request is never attached to a
renderer until it commits, we'll know there's nothing to update.

In the meantime, since we're just pushing data down to the renderer, I would
expect that it wouldn't make the project any more difficult than pushing the new
URL down does, and we'll have to deal with that anyway.

https://codereview.chromium.org/264613006/diff/110001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/264613006/diff/110001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:510:
request_.firstPartyForCookies().string().utf8());
Had to move this to the conditional below. Turns out Blink clears it when
rejecting the redirect in some cases.

[Charlie Reis, 2014-05-12 23:47:13]

Cool, works for me.  content/ LGTM.

You'll need an owner for the chrome/ files, and please have John look at
content/public/child/request_peer.h.

[jam, 2014-05-14 16:11:32]

On 2014/05/12 23:47:13, Charlie Reis(OOO as of May 17) wrote:
> Cool, works for me.  content/ LGTM.
> 
> You'll need an owner for the chrome/ files, and please have John look at
> content/public/child/request_peer.h.

lgtm

[davidben, 2014-05-14 16:31:36]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-05-14 16:31:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/264613006/140001

[commit-bot: I haz the power, 2014-05-14 18:41:27]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[commit-bot: I haz the power, 2014-05-14 20:06:42]

Change committed as 270463