Fix a bug where redirect chain gets lost on process swap.

Fix a bug where redirect chain gets lost on process swap.

BUG=79520
TEST=

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

[Matt Perry, 2011-11-23 02:51:17]

creis: just FYI, this is the approach I'm planning on attempting. It "works",
but contains a mess of hacks to avoid plumbing

battre: if you get ambitious, you can try to finish this up before i get back on
monday. i tried to outline the necessary steps - start at "step 1" in
resource_dispatcher_host.cc. don't worry if you don't have time. though it might
be good to ping darin/willchan to see if they think my plan could work.

http://codereview.chromium.org/8669014/diff/1/chrome/browser/ui/views/extensi...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

http://codereview.chromium.org/8669014/diff/1/chrome/browser/ui/views/extensi...
chrome/browser/ui/views/extensions/extension_popup.cc:44: // a GetTC and Close
method?
ignore this file

http://codereview.chromium.org/8669014/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/render_view_host_notification_task.h (right):

http://codereview.chromium.org/8669014/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_view_host_notification_task.h:96: }
this stuff is only necessary for step 1 of the hack in RDH

http://codereview.chromium.org/8669014/diff/1/content/renderer/render_view_im...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8669014/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.cc:1993: if (!frame->parent() &&
(is_content_initiated) &&
this change makes it so that the renderer no longer decides to swap processes
for a redirect. it's now decided by the browser.

http://codereview.chromium.org/8669014/diff/1/net/http/http_response_headers.cc
File net/http/http_response_headers.cc (right):

http://codereview.chromium.org/8669014/diff/1/net/http/http_response_headers....
net/http/http_response_headers.cc:357: //DCHECK_EQ('\0',
raw_headers_[raw_headers_.size() - 1]);
i was hitting this DCHECK with https-e installed due to MergeHeaders. i didn't
have time to investigate.

[Matt Perry, 2011-12-02 21:40:03]

jam, ben: OWNERS approval
creis: please review

I'm hoping to check this in today to make the m17 cutoff.

[Charlie Reis, 2011-12-02 22:38:11]

I feel like this is a complex enough change that we really need tests for it... 
I can't say that I fully understand it enough to know it's correct.

Also, please update the CL description if we're getting close to finished.

http://codereview.chromium.org/8669014/diff/8001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/transfer_navigation_resource_handler.cc
(right):

http://codereview.chromium.org/8669014/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/transfer_navigation_resource_handler.cc:18: // the
logic used by the renderer.
Please do not duplicate this code, especially if it has temporary workarounds in
it.  I'm afraid it will get stale and lead to bugs.

Can we move it to common?

http://codereview.chromium.org/8669014/diff/8001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/transfer_navigation_resource_handler.h
(right):

http://codereview.chromium.org/8669014/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/transfer_navigation_resource_handler.h:29: // form
a chrome-extension:// URL that is handled in a different
s/form/from/
s/is handled in/should be handled by/

We intend for this to eventually handle more than just swaps on
chrome-extension:// URLs, correct?  Can you add a TODO and/or file a bug to that
effect?

http://codereview.chromium.org/8669014/diff/8001/chrome/browser/ui/browser_na...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/8669014/diff/8001/chrome/browser/ui/browser_na...
chrome/browser/ui/browser_navigator.cc:562: if
(params->transferred_global_request_id != GlobalRequestID()) {
Perhaps we should abstract out this three-clause if (here and above) into a
helper function that takes in the controller and params?

http://codereview.chromium.org/8669014/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/8669014/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:469: //
TODO(mpcomplete): Step 3. (See steps 1-2 below).
This comment needs updating and should probably have the TODO removed.

http://codereview.chromium.org/8669014/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:1240: // to a new tab.
When would a request be transferred from one tab to another?  Wouldn't it be
from one RVH to another in the same tab, even for navigations in a new window?

http://codereview.chromium.org/8669014/diff/8001/content/browser/tab_contents...
File content/browser/tab_contents/navigation_entry.h (right):

http://codereview.chromium.org/8669014/diff/8001/content/browser/tab_contents...
content/browser/tab_contents/navigation_entry.h:467: // is stored until the
ResourceDispatcher receives the a navigation from the
nit: the a

Also, please clarify that this gets removed once the transfer is complete, so
this value is not persisted.

http://codereview.chromium.org/8669014/diff/8001/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/8669014/diff/8001/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:584: 
Typo?

http://codereview.chromium.org/8669014/diff/8001/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8669014/diff/8001/content/renderer/render_view...
content/renderer/render_view_impl.cc:2061: // ping-ponging.  See
http://crbug.com/72380.
Remove TODO?

http://codereview.chromium.org/8669014/diff/8001/net/http/http_response_heade...
File net/http/http_response_headers.cc (right):

http://codereview.chromium.org/8669014/diff/8001/net/http/http_response_heade...
net/http/http_response_headers.cc:357: //DCHECK_EQ('\0',
raw_headers_[raw_headers_.size() - 1]);
Needs cleanup.  Why isn't this true anymore?

[Matt Perry, 2011-12-03 00:14:24]

still no tests. I'll work on that as it runs through the trybots

http://codereview.chromium.org/8669014/diff/8001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/transfer_navigation_resource_handler.h
(right):

http://codereview.chromium.org/8669014/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/transfer_navigation_resource_handler.h:29: // form
a chrome-extension:// URL that is handled in a different
On 2011/12/02 22:38:11, creis wrote:
> s/form/from/
> s/is handled in/should be handled by/
> 
> We intend for this to eventually handle more than just swaps on
> chrome-extension:// URLs, correct?  Can you add a TODO and/or file a bug to
that
> effect?

Done.

http://codereview.chromium.org/8669014/diff/8001/chrome/browser/ui/browser_na...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/8669014/diff/8001/chrome/browser/ui/browser_na...
chrome/browser/ui/browser_navigator.cc:562: if
(params->transferred_global_request_id != GlobalRequestID()) {
On 2011/12/02 22:38:11, creis wrote:
> Perhaps we should abstract out this three-clause if (here and above) into a
> helper function that takes in the controller and params?

Done.

http://codereview.chromium.org/8669014/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/8669014/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:469: //
TODO(mpcomplete): Step 3. (See steps 1-2 below).
On 2011/12/02 22:38:11, creis wrote:
> This comment needs updating and should probably have the TODO removed.

Done.

http://codereview.chromium.org/8669014/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:1240: // to a new tab.
On 2011/12/02 22:38:11, creis wrote:
> When would a request be transferred from one tab to another?  Wouldn't it be
> from one RVH to another in the same tab, even for navigations in a new window?

I moved this block to a more isolated location, and fixed the comment.

http://codereview.chromium.org/8669014/diff/8001/content/browser/tab_contents...
File content/browser/tab_contents/navigation_entry.h (right):

http://codereview.chromium.org/8669014/diff/8001/content/browser/tab_contents...
content/browser/tab_contents/navigation_entry.h:467: // is stored until the
ResourceDispatcher receives the a navigation from the
On 2011/12/02 22:38:11, creis wrote:
> nit: the a
> 
> Also, please clarify that this gets removed once the transfer is complete, so
> this value is not persisted.

Done.

http://codereview.chromium.org/8669014/diff/8001/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/8669014/diff/8001/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:584: 
On 2011/12/02 22:38:11, creis wrote:
> Typo?

Done.

http://codereview.chromium.org/8669014/diff/8001/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8669014/diff/8001/content/renderer/render_view...
content/renderer/render_view_impl.cc:2061: // ping-ponging.  See
http://crbug.com/72380.
On 2011/12/02 22:38:11, creis wrote:
> Remove TODO?

Done.

http://codereview.chromium.org/8669014/diff/8001/net/http/http_response_heade...
File net/http/http_response_headers.cc (right):

http://codereview.chromium.org/8669014/diff/8001/net/http/http_response_heade...
net/http/http_response_headers.cc:357: //DCHECK_EQ('\0',
raw_headers_[raw_headers_.size() - 1]);
On 2011/12/02 22:38:11, creis wrote:
> Needs cleanup.  Why isn't this true anymore?

Oops, that was for an earlier bug that has been fixed. Removed.

[jam, 2011-12-03 00:15:30]

I looked at content files to make sure the are no violations to content module,
and it looks good. I'll defer to others for full review, but here's my OWNERS
lgtm

[Matt Perry, 2011-12-03 00:30:22]

http://codereview.chromium.org/8669014/diff/13003/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/webrequest/test_blocking.js (left):

http://codereview.chromium.org/8669014/diff/13003/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webrequest/test_blocking.js:131: // starts
a new one instead. See http://crbug.com/79520.
Regarding new tests, come to think of it, this is exactly the sort of test I
would write for this change. Maybe it's enough?

[Ben Goodger (Google), 2011-12-03 00:32:25]

lgtm

[Charlie Reis, 2011-12-03 01:39:56]

I would still like to see the two comments below addressed, but since the
feature's disabled for the time being, I'll grant my LGTM.  Thanks for the quick
turnaround!

http://codereview.chromium.org/8669014/diff/8001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/transfer_navigation_resource_handler.cc
(right):

http://codereview.chromium.org/8669014/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/transfer_navigation_resource_handler.cc:18: // the
logic used by the renderer.
On 2011/12/02 22:38:11, creis wrote:
> Please do not duplicate this code, especially if it has temporary workarounds
in
> it.  I'm afraid it will get stale and lead to bugs.
> 
> Can we move it to common?

This still concerns me.  You can file a bug and add a TODO if we're short on
time.

http://codereview.chromium.org/8669014/diff/13003/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/webrequest/test_blocking.js (left):

http://codereview.chromium.org/8669014/diff/13003/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webrequest/test_blocking.js:131: // starts
a new one instead. See http://crbug.com/79520.
On 2011/12/03 00:30:22, Matt Perry wrote:
> Regarding new tests, come to think of it, this is exactly the sort of test I
> would write for this change. Maybe it's enough?

This does help as a high-level regression test.  I was thinking something like
the tests in resource_dispatcher_host_unittest.cc to help document how a
cross-process redirect is supposed to look.  If you want to wait to add that
until we turn on the feature, I suppose that's ok.

[Matt Perry, 2011-12-03 01:46:31]

http://codereview.chromium.org/8669014/diff/8001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/transfer_navigation_resource_handler.cc
(right):

http://codereview.chromium.org/8669014/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/transfer_navigation_resource_handler.cc:18: // the
logic used by the renderer.
On 2011/12/03 01:39:56, creis wrote:
> On 2011/12/02 22:38:11, creis wrote:
> > Please do not duplicate this code, especially if it has temporary
workarounds
> in
> > it.  I'm afraid it will get stale and lead to bugs.
> > 
> > Can we move it to common?
> 
> This still concerns me.  You can file a bug and add a TODO if we're short on
> time.

Doh, sorry, I forgot about this. I'll add a TODO for now.