Allow MojoAsyncResourceHandler::OnWillRead to complete asyncronously

content/browser/loader/mojo_async_resource_handler_unittest.cc

Index: content/browser/loader/mojo_async_resource_handler_unittest.cc
diff --git a/content/browser/loader/mojo_async_resource_handler_unittest.cc b/content/browser/loader/mojo_async_resource_handler_unittest.cc
index d9f474e24414e9f63e24913a389ef0ef504ef6be..4eb7961f0660139860ac0ade3bbe66c9020c9787 100644
--- a/content/browser/loader/mojo_async_resource_handler_unittest.cc
+++ b/content/browser/loader/mojo_async_resource_handler_unittest.cc
@@ -603,8 +603,7 @@ TEST_F(MojoAsyncResourceHandlerTest,
     contents.append(buffer, read_size);
   }
   EXPECT_EQ(data, contents);
-  EXPECT_EQ(MockResourceLoader::Status::CALLBACK_PENDING,
-            mock_loader_->status());
+  EXPECT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->status());
 }
 
 TEST_F(MojoAsyncResourceHandlerTest,
@@ -741,8 +740,10 @@ TEST_F(MojoAsyncResourceHandlerTest, OutOfBandCancelDuringBodyTransmission) {
 
   ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
   std::string data(mock_loader_->io_buffer_size(), 'a');
-  ASSERT_EQ(MockResourceLoader::Status::CALLBACK_PENDING,
+  ASSERT_EQ(MockResourceLoader::Status::IDLE,
             mock_loader_->OnReadCompleted(data));
+  ASSERT_EQ(MockResourceLoader::Status::CALLBACK_PENDING,
+            mock_loader_->OnWillRead());
   url_loader_client_.RunUntilResponseBodyArrived();
   ASSERT_TRUE(url_loader_client_.response_body().is_valid());
 
@@ -782,45 +783,112 @@ TEST_F(MojoAsyncResourceHandlerTest, BeginWriteFailsOnWillRead) {
 
 TEST_F(MojoAsyncResourceHandlerTest, BeginWriteReturnsShouldWaitOnWillRead) {
   handler_->set_begin_write_expectation(MOJO_RESULT_SHOULD_WAIT);
+
   ASSERT_TRUE(CallOnWillStartAndOnResponseStarted());
-  ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
+  EXPECT_EQ(0, rdh_.num_in_flight_requests_for_testing());
+
+  // Bytes are read one-at-a-time, and each OnWillRead() call completes
+  // asynchronously. Note that this loop runs 4 times (once for the terminal
+  // '\0').
+  const char kReadData[] = "ABC";
+  for (const char read_char : kReadData) {
+    handler_->set_begin_write_expectation(MOJO_RESULT_SHOULD_WAIT);
+    ASSERT_EQ(MockResourceLoader::Status::CALLBACK_PENDING,
+              mock_loader_->OnWillRead());
+    EXPECT_EQ(1, rdh_.num_in_flight_requests_for_testing());
+
+    handler_->ResetBeginWriteExpectation();
+    handler_->OnWritableForTesting();
+    EXPECT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->status());
+
+    ASSERT_EQ(MockResourceLoader::Status::IDLE,
+              mock_loader_->OnReadCompleted(std::string(1, read_char)));
+    url_loader_client_.RunUntilResponseBodyArrived();
+
+    char buffer[16];
+    uint32_t read_size = sizeof(buffer);
+    MojoResult result =
+        mojo::ReadDataRaw(url_loader_client_.response_body(), buffer,
+                          &read_size, MOJO_READ_DATA_FLAG_NONE);
+    if (result == MOJO_RESULT_SHOULD_WAIT) {

[Randy Smith (Not in Mondays), 2017/03/17 20:06:06]

Under what circumstances would this happen?  And if we've written the data on
the other side of the pipe, shouldn't it eventually show up?  The 'continue'
below seems wrong in that case--there's a character we're never checking.

[mmenke, 2017/03/17 22:29:39]

You're absolutely right that this code is completely broken, and that it
shouldn't be skipping those final two checks.  You're not quite right about how
it's broken.

MOJO_RESULT_SHOULD_WAIT is not the same as ERR_IO_PENDING, I believe.  There are
(at least) three ways for async read interfaces to work:

1)  "If I don't have read data now, I'll populate this buffer later and call you
back when you have.  You have to keep the buffer alive in the meantime."  This
is how ERR_IO_PENDING works.

2)  "If I don't have read data now, I'll tell you when I do, and then you can
try and read again, with the same buffer or a shiny new one."  This is how
stream_socket_posix works (can't remember which way the windows one works -
Windows has socket APIs that work both ways).  This lets you avoid having to
keep a big ol' smelly buffer around for however long there's no data available.

3)  It's the pipe's buffer, so it can keep the buffer around or be smarter about
it.

The way this Mojo interface works is 2), so the way this should work is that it
tells us to call it again later.  We then spin the message loop (Hopefully
letting the other end of the socket catch up), and then we try reading again... 
We should not be calling OnWillRead again, or incrementing out position in the
test input, however.

Thanks so much for catching this!

+      base::RunLoop().RunUntilIdle();
+      continue;
+    }
+    ASSERT_EQ(1u, read_size);
+    EXPECT_EQ(read_char, buffer[0]);
+  }
+
+  // Should only count as one in-flight request.
+  EXPECT_EQ(1, rdh_.num_in_flight_requests_for_testing());
+
+  ASSERT_EQ(MockResourceLoader::Status::IDLE,
+            mock_loader_->OnResponseCompleted(
+                net::URLRequestStatus::FromError(net::OK)));
+
+  url_loader_client_.RunUntilComplete();
+  EXPECT_TRUE(url_loader_client_.has_received_completion());
+
+  handler_.reset();
+  EXPECT_EQ(0, rdh_.num_in_flight_requests_for_testing());
 }
 
+// Same as above, but after th efirst OnWriteable() call, BeginWrite() indicates

[Randy Smith (Not in Mondays), 2017/03/17 20:06:06]

nit: "th efirst"

[mmenke, 2017/03/17 22:29:39]

Done.

+// should wait again. Unclear if this can happen in practice, but seems best to
+// support it.
 TEST_F(MojoAsyncResourceHandlerTest,
-       BeginWriteReturnsShouldWaitOnWillReadAndThenReturnsOK) {
+       BeginWriteReturnsShouldWaitTwiceOnWillRead) {
   handler_->set_begin_write_expectation(MOJO_RESULT_SHOULD_WAIT);
+
   ASSERT_TRUE(CallOnWillStartAndOnResponseStarted());
-  size_t written = 0;
-  while (true) {
-    ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
-    MockResourceLoader::Status result = mock_loader_->OnReadCompleted(
-        std::string(mock_loader_->io_buffer_size(), 'X'));
-    written += mock_loader_->io_buffer_size();
-    if (result == MockResourceLoader::Status::CALLBACK_PENDING)
-      break;
-    ASSERT_EQ(MockResourceLoader::Status::IDLE, result);
-  }
+  EXPECT_EQ(0, rdh_.num_in_flight_requests_for_testing());
 
-  url_loader_client_.RunUntilResponseBodyArrived();
-  ASSERT_TRUE(url_loader_client_.response_body().is_valid());
-  handler_->ResetBeginWriteExpectation();
-  handler_->OnWritableForTesting();
+  // Bytes are read one-at-a-time, and each OnWillRead() call completes
+  // asynchronously. Note that this loop runs 4 times (once for the terminal
+  // '\0').
+  const char kReadData[] = "ABC";
+  for (const char read_char : kReadData) {
+    handler_->set_begin_write_expectation(MOJO_RESULT_SHOULD_WAIT);
+    ASSERT_EQ(MockResourceLoader::Status::CALLBACK_PENDING,
+              mock_loader_->OnWillRead());
+    EXPECT_EQ(1, rdh_.num_in_flight_requests_for_testing());
 
-  std::string actual;
-  while (actual.size() < written) {
-    char buf[16];
-    uint32_t read_size = sizeof(buf);
+    handler_->OnWritableForTesting();
+    EXPECT_EQ(MockResourceLoader::Status::CALLBACK_PENDING,
+              mock_loader_->status());
+
+    handler_->ResetBeginWriteExpectation();
+    handler_->OnWritableForTesting();
+    EXPECT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->status());
+
+    ASSERT_EQ(MockResourceLoader::Status::IDLE,
+              mock_loader_->OnReadCompleted(std::string(1, read_char)));
+    url_loader_client_.RunUntilResponseBodyArrived();
+
+    char buffer[16];
+    uint32_t read_size = sizeof(buffer);
     MojoResult result =
-        mojo::ReadDataRaw(url_loader_client_.response_body(), buf, &read_size,
-                          MOJO_READ_DATA_FLAG_NONE);
-    ASSERT_TRUE(result == MOJO_RESULT_OK || result == MOJO_RESULT_SHOULD_WAIT);
-    if (result == MOJO_RESULT_OK)
-      actual.append(buf, read_size);
-    base::RunLoop().RunUntilIdle();
+        mojo::ReadDataRaw(url_loader_client_.response_body(), buffer,
+                          &read_size, MOJO_READ_DATA_FLAG_NONE);
+    if (result == MOJO_RESULT_SHOULD_WAIT) {
+      base::RunLoop().RunUntilIdle();
+      continue;
+    }
+    ASSERT_EQ(1u, read_size);
+    EXPECT_EQ(read_char, buffer[0]);
   }
 
-  EXPECT_EQ(std::string(written, 'X'), actual);
-  EXPECT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->status());
+  // Should only count as one in-flight request.
+  EXPECT_EQ(1, rdh_.num_in_flight_requests_for_testing());
+
+  ASSERT_EQ(MockResourceLoader::Status::IDLE,
+            mock_loader_->OnResponseCompleted(
+                net::URLRequestStatus::FromError(net::OK)));
+
+  url_loader_client_.RunUntilComplete();
+  EXPECT_TRUE(url_loader_client_.has_received_completion());
+
+  handler_.reset();
+  EXPECT_EQ(0, rdh_.num_in_flight_requests_for_testing());
 }
 
 TEST_F(MojoAsyncResourceHandlerTest,
@@ -887,7 +955,7 @@ TEST_F(MojoAsyncResourceHandlerTest,
   mock_loader_->WaitUntilIdleOrCanceled();
   EXPECT_FALSE(url_loader_client_.has_received_completion());
   EXPECT_EQ(MockResourceLoader::Status::CANCELED, mock_loader_->status());
-  EXPECT_EQ(net::ERR_FAILED, mock_loader_->error_code());
+  EXPECT_EQ(net::ERR_INSUFFICIENT_RESOURCES, mock_loader_->error_code());
 }
 
 TEST_F(MojoAsyncResourceHandlerUploadTest, UploadProgressHandling) {
@@ -960,20 +1028,18 @@ TEST_F(MojoAsyncResourceHandlerTest, SetPriority) {
 TEST_P(MojoAsyncResourceHandlerWithAllocationSizeTest,
        OnWillReadWithLongContents) {
   ASSERT_TRUE(CallOnWillStartAndOnResponseStarted());
-  ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
+
   std::string expected;
   for (int i = 0; i < 3 * mock_loader_->io_buffer_size() + 2; ++i)
     expected += ('A' + i % 26);
 
-  ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnReadCompleted(0));

[mmenke, 2017/03/10 22:03:45]

I assume that this was to skip past the initial non-mojo buffer read.  The new
code DCHECKs if there's another OnWillRead call after OnReadCompleted(0) (Since
buffer_ is non-NULL).  Could change that, but I think it's reasonable to DCHECK
when that happens.

-
   size_t written = 0;
   std::string actual;
   while (actual.size() < expected.size()) {
     while (written < expected.size() &&
            mock_loader_->status() == MockResourceLoader::Status::IDLE) {
       ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
-      const size_t to_be_written =
+      size_t to_be_written =
           std::min(static_cast<size_t>(mock_loader_->io_buffer_size()),
                    expected.size() - written);
 
@@ -1011,10 +1077,16 @@ TEST_P(MojoAsyncResourceHandlerWithAllocationSizeTest,
   ASSERT_TRUE(CallOnWillStartAndOnResponseStarted());
   ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
 
+  // Whether the next OnReadCompleted call or OnWillRead returns the error
+  // depends on whether or not an intermediary buffer is being used by the
+  // MojoAsyncResourceHandler.
   handler_->set_begin_write_expectation(MOJO_RESULT_UNKNOWN);
-  ASSERT_EQ(MockResourceLoader::Status::CANCELED,
-            mock_loader_->OnReadCompleted(
-                std::string(mock_loader_->io_buffer_size(), 'A')));
+  MockResourceLoader::Status result = mock_loader_->OnReadCompleted(
+      std::string(mock_loader_->io_buffer_size(), 'A'));
+  if (result == MockResourceLoader::Status::CANCELED)
+    return;
+  ASSERT_EQ(MockResourceLoader::Status::IDLE, result);
+  ASSERT_EQ(MockResourceLoader::Status::CANCELED, mock_loader_->OnWillRead());
 }
 
 TEST_P(MojoAsyncResourceHandlerWithAllocationSizeTest,
@@ -1022,21 +1094,32 @@ TEST_P(MojoAsyncResourceHandlerWithAllocationSizeTest,
   ASSERT_TRUE(CallOnWillStartAndOnResponseStarted());
   ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
 
+  // Whether the next OnReadCompleted call or OnWillRead call completes
+  // asynchronously depends on whether or not an intermediary buffer is being
+  // used by the MojoAsyncResourceHandler.
   handler_->set_begin_write_expectation(MOJO_RESULT_SHOULD_WAIT);
+  MockResourceLoader::Status result = mock_loader_->OnReadCompleted(
+      std::string(mock_loader_->io_buffer_size() - 1, 'A'));
+  if (result == MockResourceLoader::Status::CALLBACK_PENDING)
+    return;
+
   ASSERT_EQ(MockResourceLoader::Status::CALLBACK_PENDING,
-            mock_loader_->OnReadCompleted(
-                std::string(mock_loader_->io_buffer_size(), 'A')));
+            mock_loader_->OnWillRead());
 }
 
 TEST_P(MojoAsyncResourceHandlerWithAllocationSizeTest,
        BeginWriteFailsOnResume) {
   ASSERT_TRUE(CallOnWillStartAndOnResponseStarted());
-  ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
-  ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnReadCompleted(0));
 
   while (true) {
-    ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
-    MockResourceLoader::Status result = mock_loader_->OnReadCompleted(
+    // Whether the next OnReadCompleted call or OnWillRead call completes
+    // asynchronously depends on whether or not an intermediary buffer is being
+    // used by the MojoAsyncResourceHandler.
+    MockResourceLoader::Status result = mock_loader_->OnWillRead();
+    if (result == MockResourceLoader::Status::CALLBACK_PENDING)
+      break;
+    ASSERT_EQ(MockResourceLoader::Status::IDLE, result);
+    result = mock_loader_->OnReadCompleted(
         std::string(mock_loader_->io_buffer_size(), 'A'));
     if (result == MockResourceLoader::Status::CALLBACK_PENDING)
       break;
@@ -1057,16 +1140,25 @@ TEST_P(MojoAsyncResourceHandlerWithAllocationSizeTest,
     base::RunLoop().RunUntilIdle();
   }
 
+  if (mock_loader_->status() == MockResourceLoader::Status::IDLE)
+    EXPECT_EQ(MockResourceLoader::Status::CANCELED, mock_loader_->OnWillRead());
+
   EXPECT_FALSE(url_loader_client_.has_received_completion());
-  EXPECT_EQ(net::ERR_FAILED, mock_loader_->error_code());
+  EXPECT_EQ(net::ERR_INSUFFICIENT_RESOURCES, mock_loader_->error_code());
 }
 
 TEST_P(MojoAsyncResourceHandlerWithAllocationSizeTest, CancelWhileWaiting) {
   ASSERT_TRUE(CallOnWillStartAndOnResponseStarted());
 
   while (true) {
-    ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
-    MockResourceLoader::Status result = mock_loader_->OnReadCompleted(
+    // Whether the next OnReadCompleted call or OnWillRead call completes
+    // asynchronously depends on whether or not an intermediary buffer is being
+    // used by the MojoAsyncResourceHandler.
+    MockResourceLoader::Status result = mock_loader_->OnWillRead();
+    if (result == MockResourceLoader::Status::CALLBACK_PENDING)
+      break;
+    ASSERT_EQ(MockResourceLoader::Status::IDLE, result);
+    result = mock_loader_->OnReadCompleted(
         std::string(mock_loader_->io_buffer_size(), 'A'));
     if (result == MockResourceLoader::Status::CALLBACK_PENDING)
       break;