Plumb redirect info out of net, through content, and into child processes.

Plumb redirect info out of net, through content, and into child processes.

This saves the logic in PluginURLFetcher and WebURLLoader that has to replicate
the method-munging, referrer-munging, and first-party-URL-munging logic that is
already done in net/.

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

[davidben, 2014-07-16 21:12:18]

Monster CL time. Matt, this is the thing we were talking about a month ago. I'm
going to let you look at it first, and then I'll toss in some appropriate
OWNERS. Most of it's mechanical. The things along the path from net/url_request
to content/child are the interesting bits.

[mmenke, 2014-07-17 16:55:10]

https://codereview.chromium.org/398903002/diff/40001/chrome/renderer/security...
File chrome/renderer/security_filter_peer.h (right):

https://codereview.chromium.org/398903002/diff/40001/chrome/renderer/security...
chrome/renderer/security_filter_peer.h:17: // NOTE: subclasses should insure
they delete themselves at the end of the
nit:  While you're here, this should be ensure.

https://codereview.chromium.org/398903002/diff/40001/content/browser/download...
File content/browser/download/download_resource_handler.cc (right):

https://codereview.chromium.org/398903002/diff/40001/content/browser/download...
content/browser/download/download_resource_handler.cc:146: bool* defer) {
Was code you removed redundant, or should we be setting the policy somewhere for
downloads?

Hrm...Wonder if there's a test for this.  Suspect not.

https://codereview.chromium.org/398903002/diff/40001/content/child/npapi/plug...
File content/child/npapi/plugin_url_fetcher.cc (right):

https://codereview.chromium.org/398903002/diff/40001/content/child/npapi/plug...
content/child/npapi/plugin_url_fetcher.cc:215: method_ = redirect_info.method;
I don't think we need method_ any more - we only needed to track the old method
for ComputerMethodForRedirect.

https://codereview.chromium.org/398903002/diff/40001/content/child/npapi/plug...
content/child/npapi/plugin_url_fetcher.cc:219: // block cross origin 307 POST
redirects.
Hrm...Should we be allowing other cross origin redirects of POSTs?  I guess so?

https://codereview.chromium.org/398903002/diff/40001/content/child/npapi/plug...
File content/child/npapi/plugin_url_fetcher.h (right):

https://codereview.chromium.org/398903002/diff/40001/content/child/npapi/plug...
content/child/npapi/plugin_url_fetcher.h:19: struct RedirectInfo;
Hrm...In most cases, you aren't bothering to forward declare this if a parent
class has it as a member of its interface.

https://codereview.chromium.org/398903002/diff/40001/content/child/web_url_lo...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/398903002/diff/40001/content/child/web_url_lo...
content/child/web_url_loader_impl.cc:488: referrer_policy_);
Should we keep the isEmpty check?

https://codereview.chromium.org/398903002/diff/40001/content/child/web_url_lo...
content/child/web_url_loader_impl.cc:504: // CrossSiteResourceHandler in the
browser. Assert that Blink didn't try to
Is this still accurate?

https://codereview.chromium.org/398903002/diff/40001/net/ocsp/nss_ocsp.cc
File net/ocsp/nss_ocsp.cc (right):

https://codereview.chromium.org/398903002/diff/40001/net/ocsp/nss_ocsp.cc#new...
net/ocsp/nss_ocsp.cc:287: const net::RedirectInfo& redirect_info,
"net::" doesn't look to be needed.

https://codereview.chromium.org/398903002/diff/40001/net/url_request/redirect...
File net/url_request/redirect_info.h (right):

https://codereview.chromium.org/398903002/diff/40001/net/url_request/redirect...
net/url_request/redirect_info.h:39: std::string referrer;
Bit of a pain, but I suggest prefixing all of these (Except status_code) with
"new_" to be clear that they're the values after the redirect (Assuming it's
followed).

https://codereview.chromium.org/398903002/diff/40001/net/url_request/url_requ...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/398903002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_job.cc:781: redirect_info.url = new_location;
Could avoid this copy by getting rid of new_location.  Could also avoid an extra
copy in the copy fragment case by only copying location  directly to
redirect_info.url when not copying the fragment.

In the scheme of things, getting rid of those copies really doesn't matter, but
if we're going to have comments about just how we're avoiding an extra copy from
ref, seems like we may as well minimize copies in other cases, too....

https://codereview.chromium.org/398903002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_job.cc:785:
URLRequest::UPDATE_FIRST_PARTY_URL_ON_REDIRECT) {
Sure there are integration tests for this (At least I *HOPE* there are), but now
that it's in net/, should have net/ tests for this as well.

https://codereview.chromium.org/398903002/diff/40001/net/url_request/url_requ...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/398903002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_unittest.cc:4615: virtual void
OnReceivedRedirect(net::URLRequest* request,
While you're here, mind getting rid of these net::'s?

[davidben, 2014-07-17 22:03:56]

https://codereview.chromium.org/398903002/diff/40001/chrome/renderer/security...
File chrome/renderer/security_filter_peer.h (right):

https://codereview.chromium.org/398903002/diff/40001/chrome/renderer/security...
chrome/renderer/security_filter_peer.h:17: // NOTE: subclasses should insure
they delete themselves at the end of the
On 2014/07/17 16:55:08, mmenke wrote:
> nit:  While you're here, this should be ensure.

Done.

https://codereview.chromium.org/398903002/diff/40001/content/browser/download...
File content/browser/download/download_resource_handler.cc (right):

https://codereview.chromium.org/398903002/diff/40001/content/browser/download...
content/browser/download/download_resource_handler.cc:146: bool* defer) {
On 2014/07/17 16:55:09, mmenke wrote:
> Was code you removed redundant, or should we be setting the policy somewhere
for
> downloads?
> 
> Hrm...Wonder if there's a test for this.  Suspect not.

Hrm, oh it's not quite redundant because requests may begin with
DownloadResourceHandler attached. I've moved it to RDH::BeginDownload. (If this
download began life at the BRH, it would never see a redirect, so that's the
only case.)

https://codereview.chromium.org/398903002/diff/40001/content/child/npapi/plug...
File content/child/npapi/plugin_url_fetcher.cc (right):

https://codereview.chromium.org/398903002/diff/40001/content/child/npapi/plug...
content/child/npapi/plugin_url_fetcher.cc:215: method_ = redirect_info.method;
On 2014/07/17 16:55:09, mmenke wrote:
> I don't think we need method_ any more - we only needed to track the old
method
> for ComputerMethodForRedirect.

Done.

https://codereview.chromium.org/398903002/diff/40001/content/child/npapi/plug...
content/child/npapi/plugin_url_fetcher.cc:219: // block cross origin 307 POST
redirects.
On 2014/07/17 16:55:09, mmenke wrote:
> Hrm...Should we be allowing other cross origin redirects of POSTs?  I guess
so?

I'm not sure. I suspect that actually should include status code 308 now. (Or
maybe checking if method_ changes (hah, maybe we do want it)) Though even if we
then fix that, in the other case, the plugin also internally reimplements
ComputeMethodForRedirect and also wouldn't be aware of 308. I figured I'd leave
it for now.

I dunno. None of this code is long for this world, and I think this was
originally for Flash which now implements redirect notification and is using
PPAPI anyway.

https://codereview.chromium.org/398903002/diff/40001/content/child/npapi/plug...
File content/child/npapi/plugin_url_fetcher.h (right):

https://codereview.chromium.org/398903002/diff/40001/content/child/npapi/plug...
content/child/npapi/plugin_url_fetcher.h:19: struct RedirectInfo;
On 2014/07/17 16:55:09, mmenke wrote:
> Hrm...In most cases, you aren't bothering to forward declare this if a parent
> class has it as a member of its interface.

Done.

https://codereview.chromium.org/398903002/diff/40001/content/child/web_url_lo...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/398903002/diff/40001/content/child/web_url_lo...
content/child/web_url_loader_impl.cc:488: referrer_policy_);
On 2014/07/17 16:55:09, mmenke wrote:
> Should we keep the isEmpty check?

WebURLRequest internally does a check.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/398903002/diff/40001/content/child/web_url_lo...
content/child/web_url_loader_impl.cc:504: // CrossSiteResourceHandler in the
browser. Assert that Blink didn't try to
On 2014/07/17 16:55:09, mmenke wrote:
> Is this still accurate?

Fixed.

https://codereview.chromium.org/398903002/diff/40001/net/ocsp/nss_ocsp.cc
File net/ocsp/nss_ocsp.cc (right):

https://codereview.chromium.org/398903002/diff/40001/net/ocsp/nss_ocsp.cc#new...
net/ocsp/nss_ocsp.cc:287: const net::RedirectInfo& redirect_info,
On 2014/07/17 16:55:09, mmenke wrote:
> "net::" doesn't look to be needed.

Done.

https://codereview.chromium.org/398903002/diff/40001/net/url_request/redirect...
File net/url_request/redirect_info.h (right):

https://codereview.chromium.org/398903002/diff/40001/net/url_request/redirect...
net/url_request/redirect_info.h:39: std::string referrer;
On 2014/07/17 16:55:09, mmenke wrote:
> Bit of a pain, but I suggest prefixing all of these (Except status_code) with
> "new_" to be clear that they're the values after the redirect (Assuming it's
> followed).

You're really mean. :-P Done.

https://codereview.chromium.org/398903002/diff/40001/net/url_request/url_requ...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/398903002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_job.cc:781: redirect_info.url = new_location;
On 2014/07/17 16:55:09, mmenke wrote:
> Could avoid this copy by getting rid of new_location.  Could also avoid an
extra
> copy in the copy fragment case by only copying location  directly to
> redirect_info.url when not copying the fragment.
> 
> In the scheme of things, getting rid of those copies really doesn't matter,
but
> if we're going to have comments about just how we're avoiding an extra copy
from
> ref, seems like we may as well minimize copies in other cases, too....

Done.

https://codereview.chromium.org/398903002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_job.cc:785:
URLRequest::UPDATE_FIRST_PARTY_URL_ON_REDIRECT) {
On 2014/07/17 16:55:09, mmenke wrote:
> Sure there are integration tests for this (At least I *HOPE* there are), but
now
> that it's in net/, should have net/ tests for this as well.

Done.

https://codereview.chromium.org/398903002/diff/40001/net/url_request/url_requ...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/398903002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_unittest.cc:4615: virtual void
OnReceivedRedirect(net::URLRequest* request,
On 2014/07/17 16:55:09, mmenke wrote:
> While you're here, mind getting rid of these net::'s?

Done.

https://codereview.chromium.org/398903002/diff/60001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/398903002/diff/60001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:538:
net::URLRequest::UPDATE_FIRST_PARTY_URL_ON_REDIRECT);
This is from download_resource_handler. Given that those requests never get
first-party URLs set to begin with, I suspect this is a no-op, modulo
differences between empty first-party URLs and first-party requests. (From when
I tried to make first-party request <=> empty first-party URL and avoid this
policy bit.)

[mmenke, 2014-07-18 15:18:36]

LGTM

https://codereview.chromium.org/398903002/diff/60001/content/browser/loader/c...
File content/browser/loader/certificate_resource_handler.h (right):

https://codereview.chromium.org/398903002/diff/60001/content/browser/loader/c...
content/browser/loader/certificate_resource_handler.h:17: #include "url/gurl.h"
Since you're getting rid of url_, don't think this is needed.  Don't believe
it's even needed in the cc file, though I could be missing something.

https://codereview.chromium.org/398903002/diff/60001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/398903002/diff/60001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:538:
net::URLRequest::UPDATE_FIRST_PARTY_URL_ON_REDIRECT);
On 2014/07/17 22:03:55, David Benjamin wrote:
> This is from download_resource_handler. Given that those requests never get
> first-party URLs set to begin with, I suspect this is a no-op, modulo
> differences between empty first-party URLs and first-party requests. (From
when
> I tried to make first-party request <=> empty first-party URL and avoid this
> policy bit.)

They don't get first party URLs?  What if we don't know its a download until we
get (Or sniff) the mime-type?  Speaking of that case...  What about when
BufferedResourceHandler begins a download?  It doesn't go through this code, I
believe...  Though I suppose at that point, there won't be any redirects, and
the we can set the policy, anyways, since the request is still pending...

[davidben, 2014-07-18 16:14:58]

+darin, nasko for everything and IPC OWNERS, respectively, and probably some
amount of review.

Most of this is mechanical; everything that consumes a redirect now takes a
net::RedirectInfo which has a summary of what changes in the new request. I
believe the interesting places are:

net/ code to assemble this struct:
net/{redirect_info,url_request,url_request_job}

content/loader code to consume this struct and push to child:
content/browser/loader/{resource_loader,resource_dispatcher_host,async_resource_handler}

content/browser/loader/cross_site_resource_handler.cc which no longer has to
implement first-party URL redirect policy before the redirects actually happen.
(Right now the URLRequest ends up in this odd state where all the accessors
correspond to the previous leg, but first_party_for_cookies switches to
corresponding to the next leg.)

content/child/* which no longer has to duplicate some of the logic.

mojo/* which also no longer has to duplicate some of the logic.

https://codereview.chromium.org/398903002/diff/60001/content/browser/loader/c...
File content/browser/loader/certificate_resource_handler.h (right):

https://codereview.chromium.org/398903002/diff/60001/content/browser/loader/c...
content/browser/loader/certificate_resource_handler.h:17: #include "url/gurl.h"
On 2014/07/18 15:18:36, mmenke wrote:
> Since you're getting rid of url_, don't think this is needed.  Don't believe
> it's even needed in the cc file, though I could be missing something.

Done.

https://codereview.chromium.org/398903002/diff/60001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/398903002/diff/60001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:538:
net::URLRequest::UPDATE_FIRST_PARTY_URL_ON_REDIRECT);
On 2014/07/18 15:18:36, mmenke wrote:
> On 2014/07/17 22:03:55, David Benjamin wrote:
> > This is from download_resource_handler. Given that those requests never get
> > first-party URLs set to begin with, I suspect this is a no-op, modulo
> > differences between empty first-party URLs and first-party requests. (From
> when
> > I tried to make first-party request <=> empty first-party URL and avoid this
> > policy bit.)
> 
> They don't get first party URLs?  What if we don't know its a download until
we
> get (Or sniff) the mime-type?  Speaking of that case...  What about when
> BufferedResourceHandler begins a download?  It doesn't go through this code, I
> believe...  Though I suppose at that point, there won't be any redirects, and
> the we can set the policy, anyways, since the request is still pending...

Sorry, that was unclear. There's two codepaths for downloads.

1. A normal navigation request that the BRH redirects to a
DownloadResourceHandler.

2. A request that began as a download in RDH::BeginDownload.

For 1, neither the old code nor the new code did anything. This codepath doesn't
hit BeginDownload and the DRH never sees the redirect. It gets its first-party
URL and now the first-party URL policy (in the old code, the
CrossSiteResourceHandler implemented that policy) from the same codepath that
sets them for other resources.

For 2, we hit this codepath. The old code never set a first-party URL which is
probably wrong, but I've left it as-is for this CL. It also would go through
RDH::BeginDownload which would implement UPDATE_FIRST_PARTY_URL_ON_REDIRECT, so
this new logic mirrors that. I suspect we actually should be setting these based
on the owning frame (does <a download> go through this codepath?) and never set
UPDATE_FIRST_PARTY_URL_ON_REDIRECT, but I've retained the existing behavior for
now.

[nasko, 2014-07-24 11:57:48]

LGTM

[davidben, 2014-07-25 19:19:05]

https://codereview.chromium.org/398903002/diff/80001/net/url_request/redirect...
File net/url_request/redirect_info.h (right):

https://codereview.chromium.org/398903002/diff/80001/net/url_request/redirect...
net/url_request/redirect_info.h:25: // always fake a set of headers?
mmenke is going to fake a set of headers to fix a host of other bugs with
plugins. So if that goes in first, this field can go away.

[davidben, 2014-08-01 21:23:28]

darin: friendly ping


(Copying summary from previous comment for convenience:

Most of this is mechanical; everything that consumes a redirect now takes a
net::RedirectInfo which has a summary of what changes in the new request. I
believe the interesting places are:

net/ code to assemble this struct:
net/{redirect_info,url_request,url_request_job}

content/loader code to consume this struct and push to child:
content/browser/loader/{resource_loader,resource_dispatcher_host,async_resource_handler}

content/browser/loader/cross_site_resource_handler.cc which no longer has to
implement first-party URL redirect policy before the redirects actually happen.
(Right now the URLRequest ends up in this odd state where all the accessors
correspond to the previous leg, but first_party_for_cookies switches to
corresponding to the next leg.)

content/child/* which no longer has to duplicate some of the logic.

mojo/* which also no longer has to duplicate some of the logic.
)

[darin (slow to review), 2014-08-14 23:54:21]

LGTM

https://codereview.chromium.org/398903002/diff/160001/content/browser/loader/...
File content/browser/loader/resource_handler.h (right):

https://codereview.chromium.org/398903002/diff/160001/content/browser/loader/...
content/browser/loader/resource_handler.h:26: struct RedirectInfo;
nit: list "struct" after "class"?

[davidben, 2014-08-15 17:12:15]

https://codereview.chromium.org/398903002/diff/160001/content/browser/loader/...
File content/browser/loader/resource_handler.h (right):

https://codereview.chromium.org/398903002/diff/160001/content/browser/loader/...
content/browser/loader/resource_handler.h:26: struct RedirectInfo;
On 2014/08/14 23:54:21, darin wrote:
> nit: list "struct" after "class"?

Done.

[davidben, 2014-08-15 17:18:10]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-08-15 17:19:47]

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

[commit-bot: I haz the power, 2014-08-15 20:50:06]

Committed patchset #10 (180001) as 290005