Move TransferNavigationResourceThrottle into CrossSiteResourceHandler.

Move TransferNavigationResourceThrottle into CrossSiteResourceHandler.

We now transfer requests to a new process when they are ready to commit, rather than each time a redirect occurs.  This simplifies the ResourceDispatcherHost logic, and it prepares for a future CL to intercept all navigations in the browser process.

BUG=238331
TEST=No more than one process swap for repeated redirects.

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

[Charlie Reis, 2013-05-21 00:16:58]

Darin, can you take a high-level look at this draft?  It's not passing tests yet
and still has some open questions, but it's where I'm headed with the refactor. 
Thanks!

[Charlie Reis, 2013-06-14 23:32:48]

Ok, this change is much simpler now that http://crrev.com/206354 has landed.

Matt, can you take a look?  I'm especially interested if you have suggestions
for how to fix resource_dispatcher_host_unittest.cc, since I can't figure out
how to get those tests to simulate OnResponseStarted after the server redirect
(which is where the transfer now happens).

Note: I haven't yet included Darin's suggestion to short-circuit the transfer,
using a new API to send the response directly to an unexpecting RenderViewImpl
(similar to WebFrame::loadData).  I was thinking about doing that in a follow-up
CL, since the API would be hard to test without this CL.

[Matt Perry, 2013-06-18 22:58:51]

Mostly looks good.

https://codereview.chromium.org/15476003/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/15476003/diff/45001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_unittest.cc:1754:
transfer_request_msg, second_filter.get(), &msg_was_ok);
Why doesn't this result in another OnResponseStarted? This is where
ResourceDispatcherHost::BeginRequest gets called. I'd expect that to start the
new request.

https://codereview.chromium.org/15476003/diff/45001/content/browser/web_conte...
File content/browser/web_contents/render_view_host_manager.h (right):

https://codereview.chromium.org/15476003/diff/45001/content/browser/web_conte...
content/browser/web_contents/render_view_host_manager.h:254: GURL new_url;
Could you document these fields? Also, it looks like new_url is only used for
redirect transfers. Maybe redirect_url would be a better name? Alternatively, it
might be clearer to add a boolean as to whether this refers to a redirect
transfer, rather than having a non-empty new_url imply it.

https://codereview.chromium.org/15476003/diff/45001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/15476003/diff/45001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:141: //   to a new process is
needed, based on the stored pending_nav_params_.
This comment confuses me. In the previous bullet point, it talks about whether a
swap is needed. Is "transfer to a new process" here referring to the same swap?

[darin (slow to review), 2013-06-18 23:03:35]

I reviewed this patch and didn't have any concerns.  LGTM once Matt is happy.

[Charlie Reis, 2013-06-19 00:17:01]

Thanks!  Still stuck on the test, but maybe it's a bit clearer now.

https://codereview.chromium.org/15476003/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/15476003/diff/45001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_unittest.cc:1754:
transfer_request_msg, second_filter.get(), &msg_was_ok);
On 2013/06/18 22:58:51, Matt Perry wrote:
> Why doesn't this result in another OnResponseStarted? This is where
> ResourceDispatcherHost::BeginRequest gets called. I'd expect that to start the
> new request.

We do get to BeginRequest in this call, but the deferred loader isn't marked as
transferring.  (it->second->is_transferring() is false, so we kill the renderer
with BadMessageReceived.)

It's not marked as transferring because we never get to CSRH::OnResponseStarted
(or even ResourceLoader::OnResponseStarted) at any point during the test.

Somehow we need to make the http://other.com/blech request get to
OnResponseStarted to set up the transfer, but I haven't been able to figure out
how to do that.

https://codereview.chromium.org/15476003/diff/45001/content/browser/web_conte...
File content/browser/web_contents/render_view_host_manager.h (right):

https://codereview.chromium.org/15476003/diff/45001/content/browser/web_conte...
content/browser/web_contents/render_view_host_manager.h:254: GURL new_url;
On 2013/06/18 22:58:51, Matt Perry wrote:
> Could you document these fields? Also, it looks like new_url is only used for
> redirect transfers. Maybe redirect_url would be a better name? Alternatively,
it
> might be clearer to add a boolean as to whether this refers to a redirect
> transfer, rather than having a non-empty new_url imply it.

Good call.  I went with transfer_url (since we'll be using this for more than
redirects in the future) and added a is_transfer boolean.

I also reordered the arguments here and on OnCrossSiteResponse to better reflect
which ones are gated by is_transfer.

https://codereview.chromium.org/15476003/diff/45001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/15476003/diff/45001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:141: //   to a new process is
needed, based on the stored pending_nav_params_.
On 2013/06/18 22:58:51, Matt Perry wrote:
> This comment confuses me. In the previous bullet point, it talks about whether
a
> swap is needed. Is "transfer to a new process" here referring to the same
swap?

Ah.  A swap is needed if either we're doing a transfer or we knew it was a
cross-process navigation from the outset (e.g., cross-site URL in the omnibox,
etc).  That corresponds to the should_transfer and swap_needed variables in
CrossSiteResourceHandler::OnResponseStarted.

I clarified it a bit in this bullet, and I also made it clearer that the "if a
swap is needed" phrase is explained in the previous bullet.

[Matt Perry, 2013-06-19 00:28:41]

Thanks, things are looking cleaner.

https://codereview.chromium.org/15476003/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/15476003/diff/45001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_unittest.cc:1754:
transfer_request_msg, second_filter.get(), &msg_was_ok);
On 2013/06/19 00:17:01, creis wrote:
> On 2013/06/18 22:58:51, Matt Perry wrote:
> > Why doesn't this result in another OnResponseStarted? This is where
> > ResourceDispatcherHost::BeginRequest gets called. I'd expect that to start
the
> > new request.
> 
> We do get to BeginRequest in this call, but the deferred loader isn't marked
as
> transferring.  (it->second->is_transferring() is false, so we kill the
renderer
> with BadMessageReceived.)
> 
> It's not marked as transferring because we never get to
CSRH::OnResponseStarted
> (or even ResourceLoader::OnResponseStarted) at any point during the test.
> 
> Somehow we need to make the http://other.com/blech request get to
> OnResponseStarted to set up the transfer, but I haven't been able to figure
out
> how to do that.

Looks like ResourceDispatcherHostTest::Factory is what actually creates the
URLRequestJob for the request. Find out which test job is being created, and
make sure that guy calls URLRequest::NotifyHeadersComplete. I think...

[Charlie Reis, 2013-06-19 01:09:24]

https://codereview.chromium.org/15476003/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/15476003/diff/45001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_unittest.cc:1754:
transfer_request_msg, second_filter.get(), &msg_was_ok);
On 2013/06/19 00:28:42, Matt Perry wrote:
> On 2013/06/19 00:17:01, creis wrote:
> > On 2013/06/18 22:58:51, Matt Perry wrote:
> > > Why doesn't this result in another OnResponseStarted? This is where
> > > ResourceDispatcherHost::BeginRequest gets called. I'd expect that to start
> the
> > > new request.
> > 
> > We do get to BeginRequest in this call, but the deferred loader isn't marked
> as
> > transferring.  (it->second->is_transferring() is false, so we kill the
> renderer
> > with BadMessageReceived.)
> > 
> > It's not marked as transferring because we never get to
> CSRH::OnResponseStarted
> > (or even ResourceLoader::OnResponseStarted) at any point during the test.
> > 
> > Somehow we need to make the http://other.com/blech request get to
> > OnResponseStarted to set up the transfer, but I haven't been able to figure
> out
> > how to do that.
> 
> Looks like ResourceDispatcherHostTest::Factory is what actually creates the
> URLRequestJob for the request. Find out which test job is being created, and
> make sure that guy calls URLRequest::NotifyHeadersComplete. I think...

Making progress!  It's a URLRequestTestJob, and we do get to
URLRequest::NotifyHeadersComplete.  That sees IsRedirectResponse return true,
though, so it calls SetBlockedOnDelegate() and returns early rather than getting
to the request_->NotifyResponseStarted() line.

I'm thinking we could move the SetResponse call a little higher (just below the
MakeTestRequest call), and then do something to unblock the URLRequest so it
starts reading the response?  Can we do that?  (Just calling MakeTestRequest
again doesn't work, since it creates a whole new request and hits the same crash
as before.)

[Matt Perry, 2013-06-19 01:20:56]

https://codereview.chromium.org/15476003/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/15476003/diff/45001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_unittest.cc:1754:
transfer_request_msg, second_filter.get(), &msg_was_ok);
On 2013/06/19 01:09:24, creis wrote:
> On 2013/06/19 00:28:42, Matt Perry wrote:
> > On 2013/06/19 00:17:01, creis wrote:
> > > On 2013/06/18 22:58:51, Matt Perry wrote:
> > > > Why doesn't this result in another OnResponseStarted? This is where
> > > > ResourceDispatcherHost::BeginRequest gets called. I'd expect that to
start
> > the
> > > > new request.
> > > 
> > > We do get to BeginRequest in this call, but the deferred loader isn't
marked
> > as
> > > transferring.  (it->second->is_transferring() is false, so we kill the
> > renderer
> > > with BadMessageReceived.)
> > > 
> > > It's not marked as transferring because we never get to
> > CSRH::OnResponseStarted
> > > (or even ResourceLoader::OnResponseStarted) at any point during the test.
> > > 
> > > Somehow we need to make the http://other.com/blech request get to
> > > OnResponseStarted to set up the transfer, but I haven't been able to
figure
> > out
> > > how to do that.
> > 
> > Looks like ResourceDispatcherHostTest::Factory is what actually creates the
> > URLRequestJob for the request. Find out which test job is being created, and
> > make sure that guy calls URLRequest::NotifyHeadersComplete. I think...
> 
> Making progress!  It's a URLRequestTestJob, and we do get to
> URLRequest::NotifyHeadersComplete.  That sees IsRedirectResponse return true,
> though, so it calls SetBlockedOnDelegate() and returns early rather than
getting
> to the request_->NotifyResponseStarted() line.
> 
> I'm thinking we could move the SetResponse call a little higher (just below
the
> MakeTestRequest call), and then do something to unblock the URLRequest so it
> starts reading the response?  Can we do that?  (Just calling MakeTestRequest
> again doesn't work, since it creates a whole new request and hits the same
crash
> as before.)

Not sure how to unblock. I'm assuming ResourceLoader is the URLRequest::Delegate
in this case? It has a ResourceLoader::Resume() method which seems to undo the
defer that happens in ResourceLoader::OnReceivedRedirect.

[Charlie Reis, 2013-07-08 18:28:43]

Hey Matt-- can you take another look?  I was able to get the test to pass after
figuring out it was blocked on the AsyncResourceHandler for the redirect, and
just needed a FollowRedirect message.

The main question is whether I've reached the right state at the end of the
test.

https://codereview.chromium.org/15476003/diff/76001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/15476003/diff/76001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_unittest.cc:1773: // We expect
fewer messages in this response than CheckSuccessfulRequest looks
I'm not 100% sure on this.  It's suspicious to me that we can't call
CheckSuccessfulRequest, but there's definitely only a ReceivedResponse and
RequestComplete in there.  I also haven't been able to figure out what
msgs[0][0] is.

https://codereview.chromium.org/15476003/diff/76001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_unittest.cc:1856: // We expect
fewer messages in this response than CheckSuccessfulRequest looks
Again, uncertain if this is the right way to end.

[Matt Perry, 2013-07-08 21:38:56]

https://codereview.chromium.org/15476003/diff/76001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/15476003/diff/76001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_unittest.cc:1736:
base::MessageLoop::current()->RunUntilIdle();
deprecated - use RunLoop::RunUntilIdle()

https://codereview.chromium.org/15476003/diff/76001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_unittest.cc:1773: // We expect
fewer messages in this response than CheckSuccessfulRequest looks
On 2013/07/08 18:28:43, creis wrote:
> I'm not 100% sure on this.  It's suspicious to me that we can't call
> CheckSuccessfulRequest, but there's definitely only a ReceivedResponse and
> RequestComplete in there.  I also haven't been able to figure out what
> msgs[0][0] is.

I don't know much about it, but is it a bug that there's no shared memory data
sent for transferred redirects?

Also, what is msgs[0][0].type() ? The high 16 bits represent the message class
(see ipc_message_start.h), the low 16 bits are the line number within the
message header file where the message is declared. (see ipc_message_macros.h)

[Charlie Reis, 2013-07-08 22:49:30]

Thanks, though I'm still a bit stuck.  Trying to wrap my head around how
ResourceLoader works is a bit tricky.

https://codereview.chromium.org/15476003/diff/76001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/15476003/diff/76001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_unittest.cc:1736:
base::MessageLoop::current()->RunUntilIdle();
On 2013/07/08 21:38:57, Matt Perry wrote:
> deprecated - use RunLoop::RunUntilIdle()

Is that recent?  I don't see any other tests using that, besides in a callback
in user_cloud_policy_manager_chromeos_unittest.cc.  What's the intended
practice?  Creating a new RunLoop?

https://codereview.chromium.org/15476003/diff/76001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_unittest.cc:1773: // We expect
fewer messages in this response than CheckSuccessfulRequest looks
On 2013/07/08 21:38:57, Matt Perry wrote:
> On 2013/07/08 18:28:43, creis wrote:
> > I'm not 100% sure on this.  It's suspicious to me that we can't call
> > CheckSuccessfulRequest, but there's definitely only a ReceivedResponse and
> > RequestComplete in there.  I also haven't been able to figure out what
> > msgs[0][0] is.
> 
> I don't know much about it, but is it a bug that there's no shared memory data
> sent for transferred redirects?

Entirely possible.  It looks like we usually send the SetDataBuffer message in
AsyncResourceHandler::OnReadCompleted.  That's being called with bytes_read==0
after the transfer (line 1763 above), since ResourceLoader::StartReading is
reading 0 bytes in ReadMore.

Maybe this is because we read the bytes in the old handler, but then transferred
to a new handler?  The handler doesn't keep track of the bytes, though, does it?

> 
> Also, what is msgs[0][0].type() ? The high 16 bits represent the message class
> (see ipc_message_start.h), the low 16 bits are the line number within the
> message header file where the message is declared. (see ipc_message_macros.h)

msgs[0][0].type() is 1769699.

That's useful to know how the type is computed, but it's not making sense to me.
 ResourceMsg_ReceivedResponse is 1769680, and ResourceMsg_RequestComplete is
1769740, but those messages are defined on lines 206 and 263 of
resource_messages.h, respectively.

[Matt Perry, 2013-07-08 22:59:34]

https://codereview.chromium.org/15476003/diff/76001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/15476003/diff/76001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_unittest.cc:1736:
base::MessageLoop::current()->RunUntilIdle();
On 2013/07/08 22:49:31, creis wrote:
> On 2013/07/08 21:38:57, Matt Perry wrote:
> > deprecated - use RunLoop::RunUntilIdle()
> 
> Is that recent?  I don't see any other tests using that, besides in a callback
> in user_cloud_policy_manager_chromeos_unittest.cc.  What's the intended
> practice?  Creating a new RunLoop?

Somewhat recent. See message_loop.h for the deprecation comment.

I've seen people do RunLoop().RunUntilIdle();

https://codereview.chromium.org/15476003/diff/76001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_unittest.cc:1773: // We expect
fewer messages in this response than CheckSuccessfulRequest looks
On 2013/07/08 22:49:31, creis wrote:
> On 2013/07/08 21:38:57, Matt Perry wrote:
> > On 2013/07/08 18:28:43, creis wrote:
> > > I'm not 100% sure on this.  It's suspicious to me that we can't call
> > > CheckSuccessfulRequest, but there's definitely only a ReceivedResponse and
> > > RequestComplete in there.  I also haven't been able to figure out what
> > > msgs[0][0] is.
> > 
> > I don't know much about it, but is it a bug that there's no shared memory
data
> > sent for transferred redirects?
> 
> Entirely possible.  It looks like we usually send the SetDataBuffer message in
> AsyncResourceHandler::OnReadCompleted.  That's being called with bytes_read==0
> after the transfer (line 1763 above), since ResourceLoader::StartReading is
> reading 0 bytes in ReadMore.
> 
> Maybe this is because we read the bytes in the old handler, but then
transferred
> to a new handler?  The handler doesn't keep track of the bytes, though, does
it?

I guess for a redirect, you're going to get a new payload after the redirect. So
the fact that you're not saving the old bytes is correct. Maybe you need another
call to SetResponse() after following the redirect?

> 
> > 
> > Also, what is msgs[0][0].type() ? The high 16 bits represent the message
class
> > (see ipc_message_start.h), the low 16 bits are the line number within the
> > message header file where the message is declared. (see
ipc_message_macros.h)
> 
> msgs[0][0].type() is 1769699.
> 
> That's useful to know how the type is computed, but it's not making sense to
me.
>  ResourceMsg_ReceivedResponse is 1769680, and ResourceMsg_RequestComplete is
> 1769740, but those messages are defined on lines 206 and 263 of
> resource_messages.h, respectively.

(1769699 & 0xffff0000) >> 16 gives you the message class (27, or
ResourceMsgStart, in this case).
(1769699 & 0x0000ffff) gives you the line number (227). That's
ResourceMsg_ReceivedRedirect.

[Charlie Reis, 2013-07-09 00:08:43]

Note: I tracked down one silly bug in render_view_host_manager.cc (checking for
the wrong value of is_transfer), but that didn't affect the test we're
discussing.

https://codereview.chromium.org/15476003/diff/76001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/15476003/diff/76001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_unittest.cc:1736:
base::MessageLoop::current()->RunUntilIdle();
On 2013/07/08 22:59:34, Matt Perry wrote:
> On 2013/07/08 22:49:31, creis wrote:
> > On 2013/07/08 21:38:57, Matt Perry wrote:
> > > deprecated - use RunLoop::RunUntilIdle()
> > 
> > Is that recent?  I don't see any other tests using that, besides in a
callback
> > in user_cloud_policy_manager_chromeos_unittest.cc.  What's the intended
> > practice?  Creating a new RunLoop?
> 
> Somewhat recent. See message_loop.h for the deprecation comment.
> 
> I've seen people do RunLoop().RunUntilIdle();
> 

Done.

https://codereview.chromium.org/15476003/diff/76001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_unittest.cc:1773: // We expect
fewer messages in this response than CheckSuccessfulRequest looks
On 2013/07/08 22:59:34, Matt Perry wrote:
> On 2013/07/08 22:49:31, creis wrote:
> > On 2013/07/08 21:38:57, Matt Perry wrote:
> > > On 2013/07/08 18:28:43, creis wrote:
> > > > I'm not 100% sure on this.  It's suspicious to me that we can't call
> > > > CheckSuccessfulRequest, but there's definitely only a ReceivedResponse
and
> > > > RequestComplete in there.  I also haven't been able to figure out what
> > > > msgs[0][0] is.
> > > 
> > > I don't know much about it, but is it a bug that there's no shared memory
> data
> > > sent for transferred redirects?
> > 
> > Entirely possible.  It looks like we usually send the SetDataBuffer message
in
> > AsyncResourceHandler::OnReadCompleted.  That's being called with
bytes_read==0
> > after the transfer (line 1763 above), since ResourceLoader::StartReading is
> > reading 0 bytes in ReadMore.
> > 
> > Maybe this is because we read the bytes in the old handler, but then
> transferred
> > to a new handler?  The handler doesn't keep track of the bytes, though, does
> it?
> 
> I guess for a redirect, you're going to get a new payload after the redirect.
So
> the fact that you're not saving the old bytes is correct. Maybe you need
another
> call to SetResponse() after following the redirect?

Calling SetResponse again before sending the transfer_request_msg doesn't have
an effect.

Here's what I'm concerned about: this CL makes it so that the transfer doesn't
happen until OnResponseStarted, and that doesn't take place until after the
redirect and after the original BufferedResourceHandler has read the response
and sniffed it.  Then we replace the whole handler (including the
BufferedResourceHandler) with a new one, which has to go through
OnResponseStarted again.  That seems like it could lose the bytes from the first
one.

It seems to be working in practice, though, even for small pages that just have
one sentence in them.  Thus, I'm not sure I'm understanding this correctly.  Do
we need to preserve the old handler instead of replacing it?  Or if not, maybe
we can verify that the buffered data is still around somewhere and can be
delivered to the new process?

> 
> > 
> > > 
> > > Also, what is msgs[0][0].type() ? The high 16 bits represent the message
> class
> > > (see ipc_message_start.h), the low 16 bits are the line number within the
> > > message header file where the message is declared. (see
> ipc_message_macros.h)
> > 
> > msgs[0][0].type() is 1769699.
> > 
> > That's useful to know how the type is computed, but it's not making sense to
> me.
> >  ResourceMsg_ReceivedResponse is 1769680, and ResourceMsg_RequestComplete is
> > 1769740, but those messages are defined on lines 206 and 263 of
> > resource_messages.h, respectively.
> 
> (1769699 & 0xffff0000) >> 16 gives you the message class (27, or
> ResourceMsgStart, in this case).
> (1769699 & 0x0000ffff) gives you the line number (227). That's
> ResourceMsg_ReceivedRedirect.

Ah, I get it.  I added a check for ReceivedRedirect.  Thanks.

[Matt Perry, 2013-07-09 00:22:33]

I don't know enough to say whether the unittest is exposing a bug, so LGTM.

https://codereview.chromium.org/15476003/diff/76001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/15476003/diff/76001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_unittest.cc:1773: // We expect
fewer messages in this response than CheckSuccessfulRequest looks
On 2013/07/09 00:08:44, creis wrote:
> On 2013/07/08 22:59:34, Matt Perry wrote:
> > On 2013/07/08 22:49:31, creis wrote:
> > > On 2013/07/08 21:38:57, Matt Perry wrote:
> > > > On 2013/07/08 18:28:43, creis wrote:
> > > > > I'm not 100% sure on this.  It's suspicious to me that we can't call
> > > > > CheckSuccessfulRequest, but there's definitely only a ReceivedResponse
> and
> > > > > RequestComplete in there.  I also haven't been able to figure out what
> > > > > msgs[0][0] is.
> > > > 
> > > > I don't know much about it, but is it a bug that there's no shared
memory
> > data
> > > > sent for transferred redirects?
> > > 
> > > Entirely possible.  It looks like we usually send the SetDataBuffer
message
> in
> > > AsyncResourceHandler::OnReadCompleted.  That's being called with
> bytes_read==0
> > > after the transfer (line 1763 above), since ResourceLoader::StartReading
is
> > > reading 0 bytes in ReadMore.
> > > 
> > > Maybe this is because we read the bytes in the old handler, but then
> > transferred
> > > to a new handler?  The handler doesn't keep track of the bytes, though,
does
> > it?
> > 
> > I guess for a redirect, you're going to get a new payload after the
redirect.
> So
> > the fact that you're not saving the old bytes is correct. Maybe you need
> another
> > call to SetResponse() after following the redirect?
> 
> Calling SetResponse again before sending the transfer_request_msg doesn't have
> an effect.
> 
> Here's what I'm concerned about: this CL makes it so that the transfer doesn't
> happen until OnResponseStarted, and that doesn't take place until after the
> redirect and after the original BufferedResourceHandler has read the response
> and sniffed it.  Then we replace the whole handler (including the
> BufferedResourceHandler) with a new one, which has to go through
> OnResponseStarted again.  That seems like it could lose the bytes from the
first
> one.
> 
> It seems to be working in practice, though, even for small pages that just
have
> one sentence in them.  Thus, I'm not sure I'm understanding this correctly. 
Do
> we need to preserve the old handler instead of replacing it?  Or if not, maybe
> we can verify that the buffered data is still around somewhere and can be
> delivered to the new process?

I wonder if the bytes have already been transmitted to the renderer? I'm well
out of my realm of expertise, though.

[Charlie Reis, 2013-07-09 23:54:43]

https://codereview.chromium.org/15476003/diff/76001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/15476003/diff/76001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_unittest.cc:1773: // We expect
fewer messages in this response than CheckSuccessfulRequest looks
On 2013/07/09 00:22:33, Matt Perry wrote:
> On 2013/07/09 00:08:44, creis wrote:
> > On 2013/07/08 22:59:34, Matt Perry wrote:
> > > On 2013/07/08 22:49:31, creis wrote:
> > > > On 2013/07/08 21:38:57, Matt Perry wrote:
> > > > > On 2013/07/08 18:28:43, creis wrote:
> > > > > > I'm not 100% sure on this.  It's suspicious to me that we can't call
> > > > > > CheckSuccessfulRequest, but there's definitely only a
ReceivedResponse
> > and
> > > > > > RequestComplete in there.  I also haven't been able to figure out
what
> > > > > > msgs[0][0] is.
> > > > > 
> > > > > I don't know much about it, but is it a bug that there's no shared
> memory
> > > data
> > > > > sent for transferred redirects?
> > > > 
> > > > Entirely possible.  It looks like we usually send the SetDataBuffer
> message
> > in
> > > > AsyncResourceHandler::OnReadCompleted.  That's being called with
> > bytes_read==0
> > > > after the transfer (line 1763 above), since ResourceLoader::StartReading
> is
> > > > reading 0 bytes in ReadMore.
> > > > 
> > > > Maybe this is because we read the bytes in the old handler, but then
> > > transferred
> > > > to a new handler?  The handler doesn't keep track of the bytes, though,
> does
> > > it?
> > > 
> > > I guess for a redirect, you're going to get a new payload after the
> redirect.
> > So
> > > the fact that you're not saving the old bytes is correct. Maybe you need
> > another
> > > call to SetResponse() after following the redirect?
> > 
> > Calling SetResponse again before sending the transfer_request_msg doesn't
have
> > an effect.
> > 
> > Here's what I'm concerned about: this CL makes it so that the transfer
doesn't
> > happen until OnResponseStarted, and that doesn't take place until after the
> > redirect and after the original BufferedResourceHandler has read the
response
> > and sniffed it.  Then we replace the whole handler (including the
> > BufferedResourceHandler) with a new one, which has to go through
> > OnResponseStarted again.  That seems like it could lose the bytes from the
> first
> > one.
> > 
> > It seems to be working in practice, though, even for small pages that just
> have
> > one sentence in them.  Thus, I'm not sure I'm understanding this correctly. 
> Do
> > we need to preserve the old handler instead of replacing it?  Or if not,
maybe
> > we can verify that the buffered data is still around somewhere and can be
> > delivered to the new process?
> 
> I wonder if the bytes have already been transmitted to the renderer? I'm well
> out of my realm of expertise, though.

Ugh, I just confirmed there's a real bug here.  The key difference is that this
test uses text/plain, which causes BufferedResourceHandler::ShouldSniffContent()
to return true.  Normal web pages work fine, but doing a transfer navigation to
any content that requires sniffing (like a .txt file) breaks-- we do indeed
forget the buffered data when we switch handlers.

I'll have to investigate how we can fix this.  Maybe it's possible to transfer
the existing handler rather than creating a new one?

[Charlie Reis, 2013-09-30 22:41:58]

On 2013/07/09 23:54:43, creis wrote:
> Ugh, I just confirmed there's a real bug here.  The key difference is that
this
> test uses text/plain, which causes
BufferedResourceHandler::ShouldSniffContent()
> to return true.  Normal web pages work fine, but doing a transfer navigation
to
> any content that requires sniffing (like a .txt file) breaks-- we do indeed
> forget the buffered data when we switch handlers.
> 
> I'll have to investigate how we can fix this.  Maybe it's possible to transfer
> the existing handler rather than creating a new one?

After quite a delay, we've landed http://crrev.com/225263 to fix the
BufferedResourceHandler issue.  (We no longer replace the ResourceHandler chain
after a transfer.)  I've rebased this CL and updated the test expectations
accordingly, so it should be good to go.

Matt, can you take a look at
content/browser/loader/resource_dispatcher_host_unittest.cc specifically?  I
just want to be sure I'm testing the right things now.  (I added versions of
TransferNavigation for both text/html and text/plain to trigger the content
sniffing case, and I went back to using CheckSuccessfulRequest.)

Outside that, there a few bits of cleanup from the rebasing (e.g., how we resume
the response from ResourceLoader::CompleteTransfer), but it's mostly the same.

[Matt Perry, 2013-09-30 23:09:23]

I think this makes sense. LGTM

[commit-bot: I haz the power, 2013-10-01 01:41:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/15476003/137001

[darin (slow to review), 2013-10-01 03:54:15]

https://codereview.chromium.org/15476003/diff/137001/content/browser/loader/r...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/15476003/diff/137001/content/browser/loader/r...
content/browser/loader/resource_loader.cc:174:
info->cross_site_handler()->ResumeResponse();
what happens if the cross_site_handler() is null here? does the ResourceLoader
get cleaned up properly somehow?

[commit-bot: I haz the power, 2013-10-01 12:53:31]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[Charlie Reis, 2013-10-01 17:15:10]

Sorry I missed this last night while it was in the CQ, but it didn't land--
flaky try bots to the rescue.  :)  Darin, can you take another look?

https://codereview.chromium.org/15476003/diff/137001/content/browser/loader/r...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/15476003/diff/137001/content/browser/loader/r...
content/browser/loader/resource_loader.cc:174:
info->cross_site_handler()->ResumeResponse();
On 2013/10/01 03:54:15, darin wrote:
> what happens if the cross_site_handler() is null here? does the ResourceLoader
> get cleaned up properly somehow?

Good question.  This should never happen, since CompleteTransfer is only called
from RDH::BeginRequest after we've found the deferred_loader and run
UpdateRequestForTransfer.  I've added a DCHECK to UpdateRequestForTransfer to
make sure the cross_site_handler exists, and I've removed the null check from
here so that we get a crash report if it ever happens.

[darin (slow to review), 2013-10-01 17:34:46]

LGTM

[commit-bot: I haz the power, 2013-10-01 17:36:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/15476003/164001

[commit-bot: I haz the power, 2013-10-01 20:12:57]

Change committed as 226284