Move TransferNavigationResourceThrottle into CrossSiteResourceHandler.

content/browser/loader/resource_dispatcher_host_unittest.cc

Index: content/browser/loader/resource_dispatcher_host_unittest.cc
diff --git a/content/browser/loader/resource_dispatcher_host_unittest.cc b/content/browser/loader/resource_dispatcher_host_unittest.cc
index 13895b0c61a99531564f87829e6b3a481cb0210b..c5f26de6e0309d1060a6c36decb66f92d8990515 100644
--- a/content/browser/loader/resource_dispatcher_host_unittest.cc
+++ b/content/browser/loader/resource_dispatcher_host_unittest.cc
@@ -1723,7 +1723,23 @@ TEST_F(ResourceDispatcherHostTest, TransferNavigation) {
 
   MakeTestRequest(render_view_id, request_id, GURL("http://example.com/blah"));
 
-  // Restore.
+  // Now that we're blocked on the redirect, update the response and unblock by
+  // telling the AsyncResourceHandler to follow the redirect.
+  const std::string kResponseBody = "hello world";
+  SetResponse("HTTP/1.1 200 OK\n"
+              "Content-Type: text/plain\n\n",
+              kResponseBody);
+  ResourceHostMsg_FollowRedirect redirect_msg(
+      render_view_id, request_id, false, GURL());
+  bool msg_was_ok;
+  host_.OnMessageReceived(redirect_msg, filter_.get(), &msg_was_ok);
+  base::MessageLoop::current()->RunUntilIdle();

[Matt Perry, 2013/07/08 21:38:57]

deprecated - use RunLoop::RunUntilIdle()

[Charlie Reis, 2013/07/08 22:49:31]

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?

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

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

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

[Charlie Reis, 2013/07/09 00:08:44]

Done.

+
+  // Flush all the pending requests to get the response through the
+  // BufferedResourceHandler.
+  while (net::URLRequestTestJob::ProcessOnePendingMessage()) {}
+
+  // Restore, now that we've set up a transfer.
   SetBrowserClientForTesting(old_client);
 
   // This second filter is used to emulate a second process.
@@ -1733,11 +1749,6 @@ TEST_F(ResourceDispatcherHostTest, TransferNavigation) {
   int new_render_view_id = 1;
   int new_request_id = 2;
 
-  const std::string kResponseBody = "hello world";
-  SetResponse("HTTP/1.1 200 OK\n"
-              "Content-Type: text/plain\n\n",
-              kResponseBody);
-
   ResourceHostMsg_Request request =
       CreateResourceRequest("GET", ResourceType::MAIN_FRAME,
                             GURL("http://other.com/blech"));
@@ -1748,7 +1759,6 @@ TEST_F(ResourceDispatcherHostTest, TransferNavigation) {
   child_ids_.insert(second_filter->child_id());
   ResourceHostMsg_RequestResource transfer_request_msg(
       new_render_view_id, new_request_id, request);
-  bool msg_was_ok;
   host_.OnMessageReceived(
       transfer_request_msg, second_filter.get(), &msg_was_ok);
   base::MessageLoop::current()->RunUntilIdle();
@@ -1760,11 +1770,15 @@ TEST_F(ResourceDispatcherHostTest, TransferNavigation) {
   ResourceIPCAccumulator::ClassifiedMessages msgs;
   accum_.GetClassifiedMessages(&msgs);
 
-  ASSERT_EQ(1U, msgs.size());
-  CheckSuccessfulRequest(msgs[0], kResponseBody);
+  // We expect fewer messages in this response than CheckSuccessfulRequest looks

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

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.

[Matt Perry, 2013/07/08 21:38:57]

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:31]

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?
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]

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?
(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:44]

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?
Ah, I get it.  I added a check for ReceivedRedirect.  Thanks.

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

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]

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?

+  // for, since most were handled before the transfer.
+  ASSERT_EQ(2U, msgs.size());
+  ASSERT_EQ(2U, msgs[1].size());
+  EXPECT_EQ(ResourceMsg_ReceivedResponse::ID, msgs[1][0].type());
+  EXPECT_EQ(ResourceMsg_RequestComplete::ID, msgs[1][1].type());
 }
 
-TEST_F(ResourceDispatcherHostTest, TransferNavigationAndThenRedirect) {
+TEST_F(ResourceDispatcherHostTest, TransferNavigationWithTwoRedirects) {
   EXPECT_EQ(0, host_.pending_requests());
 
   int render_view_id = 0;
@@ -1784,6 +1798,30 @@ TEST_F(ResourceDispatcherHostTest, TransferNavigationAndThenRedirect) {
 
   MakeTestRequest(render_view_id, request_id, GURL("http://example.com/blah"));
 
+  // Now that we're blocked on the redirect, simulate hitting another redirect.
+  SetResponse("HTTP/1.1 302 Found\n"
+              "Location: http://other.com/blerg\n\n");
+  ResourceHostMsg_FollowRedirect redirect_msg(
+      render_view_id, request_id, false, GURL());
+  bool msg_was_ok;
+  host_.OnMessageReceived(redirect_msg, filter_.get(), &msg_was_ok);
+  base::MessageLoop::current()->RunUntilIdle();
+
+  // Now that we're blocked on the second redirect, update the response and
+  // unblock by telling the AsyncResourceHandler to follow the redirect.
+  const std::string kResponseBody = "hello world";
+  SetResponse("HTTP/1.1 200 OK\n"
+              "Content-Type: text/plain\n\n",
+              kResponseBody);
+  ResourceHostMsg_FollowRedirect redirect_msg2(
+      render_view_id, request_id, false, GURL());
+  host_.OnMessageReceived(redirect_msg2, filter_.get(), &msg_was_ok);
+  base::MessageLoop::current()->RunUntilIdle();
+
+  // Flush all the pending requests to get the response through the
+  // BufferedResourceHandler.
+  while (net::URLRequestTestJob::ProcessOnePendingMessage()) {}
+
   // Restore.
   SetBrowserClientForTesting(old_client);
 
@@ -1794,13 +1832,6 @@ TEST_F(ResourceDispatcherHostTest, TransferNavigationAndThenRedirect) {
   int new_render_view_id = 1;
   int new_request_id = 2;
 
-  // Delay the start of the next request so that we can setup the response for
-  // the next URL.
-  SetDelayedStartJobGeneration(true);
-
-  SetResponse("HTTP/1.1 302 Found\n"
-              "Location: http://other.com/blerg\n\n");
-
   ResourceHostMsg_Request request =
       CreateResourceRequest("GET", ResourceType::MAIN_FRAME,
                             GURL("http://other.com/blech"));
@@ -1811,31 +1842,10 @@ TEST_F(ResourceDispatcherHostTest, TransferNavigationAndThenRedirect) {
   child_ids_.insert(second_filter->child_id());
   ResourceHostMsg_RequestResource transfer_request_msg(
       new_render_view_id, new_request_id, request);
-  bool msg_was_ok;
   host_.OnMessageReceived(
       transfer_request_msg, second_filter.get(), &msg_was_ok);
   base::MessageLoop::current()->RunUntilIdle();
 
-  // Response data for "http://other.com/blerg":
-  const std::string kResponseBody = "hello world";
-  SetResponse("HTTP/1.1 200 OK\n"
-              "Content-Type: text/plain\n\n",
-              kResponseBody);
-
-  // OK, let the redirect happen.
-  SetDelayedStartJobGeneration(false);
-  CompleteStartRequest(second_filter.get(), new_request_id);
-  base::MessageLoop::current()->RunUntilIdle();
-
-  // Flush all the pending requests.
-  while (net::URLRequestTestJob::ProcessOnePendingMessage()) {}
-
-  // Now, simulate the renderer choosing to follow the redirect.
-  ResourceHostMsg_FollowRedirect redirect_msg(
-      new_render_view_id, new_request_id, false, GURL());
-  host_.OnMessageReceived(redirect_msg, second_filter.get(), &msg_was_ok);
-  base::MessageLoop::current()->RunUntilIdle();
-
   // Flush all the pending requests.
   while (net::URLRequestTestJob::ProcessOnePendingMessage()) {}
 
@@ -1843,12 +1853,12 @@ TEST_F(ResourceDispatcherHostTest, TransferNavigationAndThenRedirect) {
   ResourceIPCAccumulator::ClassifiedMessages msgs;
   accum_.GetClassifiedMessages(&msgs);
 
-  ASSERT_EQ(1U, msgs.size());
-
-  // We should have received a redirect followed by a "normal" payload.
-  EXPECT_EQ(ResourceMsg_ReceivedRedirect::ID, msgs[0][0].type());
-  msgs[0].erase(msgs[0].begin());
-  CheckSuccessfulRequest(msgs[0], kResponseBody);
+  // We expect fewer messages in this response than CheckSuccessfulRequest looks

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

Again, uncertain if this is the right way to end.

+  // for, since most were handled before the transfer.
+  ASSERT_EQ(2U, msgs.size());
+  ASSERT_EQ(2U, msgs[1].size());
+  EXPECT_EQ(ResourceMsg_ReceivedResponse::ID, msgs[1][0].type());
+  EXPECT_EQ(ResourceMsg_RequestComplete::ID, msgs[1][1].type());
 }
 
 TEST_F(ResourceDispatcherHostTest, UnknownURLScheme) {