Allow MojoAsyncResourceHandler::OnWillRead to complete asyncronously

content/browser/loader/mojo_async_resource_handler_unittest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/loader/mojo_async_resource_handler.h"
 
 #include <string.h>
 
 #include <utility>
 #include <vector>
@@ skipping 585 matching lines @@
     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)
       continue;
     ASSERT_EQ(MOJO_RESULT_OK, result);
     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,
        IOBufferFromOnWillReadShouldRemainValidEvenIfHandlerIsGone) {
   ASSERT_TRUE(CallOnWillStartAndOnResponseStarted());
   ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
   // The io_buffer size that the mime sniffer requires implicitly.
   ASSERT_GE(mock_loader_->io_buffer_size(),
             kSizeMimeSnifferRequiresForFirstOnWillRead);
 
@@ skipping 116 matching lines @@
       break;
     ASSERT_TRUE(result == MOJO_RESULT_SHOULD_WAIT || result == MOJO_RESULT_OK);
   }
 }
 
 TEST_F(MojoAsyncResourceHandlerTest, OutOfBandCancelDuringBodyTransmission) {
   ASSERT_TRUE(CallOnWillStartAndOnResponseStarted());
 
   ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
   std::string data(mock_loader_->io_buffer_size(), 'a');
+  ASSERT_EQ(MockResourceLoader::Status::IDLE,
+            mock_loader_->OnReadCompleted(data));
   ASSERT_EQ(MockResourceLoader::Status::CALLBACK_PENDING,
-            mock_loader_->OnReadCompleted(data));
+            mock_loader_->OnWillRead());
   url_loader_client_.RunUntilResponseBodyArrived();
   ASSERT_TRUE(url_loader_client_.response_body().is_valid());
 
   net::URLRequestStatus status(net::URLRequestStatus::FAILED, net::ERR_FAILED);
   ASSERT_EQ(
       MockResourceLoader::Status::IDLE,
       mock_loader_->OnResponseCompletedFromExternalOutOfBandCancel(status));
 
   url_loader_client_.RunUntilComplete();
   EXPECT_TRUE(url_loader_client_.has_received_completion());
@@ skipping 19 matching lines @@
 }
 
 TEST_F(MojoAsyncResourceHandlerTest, BeginWriteFailsOnWillRead) {
   handler_->set_begin_write_expectation(MOJO_RESULT_UNKNOWN);
   ASSERT_TRUE(CallOnWillStartAndOnResponseStarted());
   ASSERT_EQ(MockResourceLoader::Status::CANCELED, mock_loader_->OnWillRead());
 }
 
 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,
+       BeginWriteReturnsShouldWaitTwiceOnWillRead) {
+  handler_->set_begin_write_expectation(MOJO_RESULT_SHOULD_WAIT);
+
+  ASSERT_TRUE(CallOnWillStartAndOnResponseStarted());
+  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_->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(), 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]);
+  }
+
+  // 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,
-       BeginWriteReturnsShouldWaitOnWillReadAndThenReturnsOK) {
-  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);
-  }
-
-  url_loader_client_.RunUntilResponseBodyArrived();
-  ASSERT_TRUE(url_loader_client_.response_body().is_valid());
-  handler_->ResetBeginWriteExpectation();
-  handler_->OnWritableForTesting();
-
-  std::string actual;
-  while (actual.size() < written) {
-    char buf[16];
-    uint32_t read_size = sizeof(buf);
-    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();
-  }
-
-  EXPECT_EQ(std::string(written, 'X'), actual);
-  EXPECT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->status());
-}
-
-TEST_F(MojoAsyncResourceHandlerTest,
        EndWriteFailsOnWillReadWithInsufficientInitialCapacity) {
   MojoAsyncResourceHandler::SetAllocationSizeForTesting(2);
   ASSERT_TRUE(CallOnWillStartAndOnResponseStarted());
   handler_->set_end_write_expectation(MOJO_RESULT_UNKNOWN);
   ASSERT_EQ(MockResourceLoader::Status::CANCELED, mock_loader_->OnWillRead());
 }
 
 TEST_F(MojoAsyncResourceHandlerTest, EndWriteFailsOnReadCompleted) {
   ASSERT_TRUE(CallOnWillStartAndOnResponseStarted());
   ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
@@ skipping 43 matching lines @@
                           MOJO_READ_DATA_FLAG_NONE);
     if (result == MOJO_RESULT_SHOULD_WAIT)
       break;
     ASSERT_EQ(MOJO_RESULT_OK, result);
   }
 
   handler_->set_end_write_expectation(MOJO_RESULT_SHOULD_WAIT);
   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) {
   ASSERT_TRUE(CallOnWillStart());
 
   // Expect no report for no progress.
   set_upload_progress(net::UploadProgress(0, 1000));
   handler_->PollUploadProgress();
   EXPECT_FALSE(url_loader_client_.has_received_upload_progress());
   EXPECT_EQ(0, url_loader_client_.current_upload_position());
@@ skipping 52 matching lines @@
   EXPECT_EQ(net::LOWEST, request_->priority());
 
   handler_->SetPriority(net::RequestPriority::HIGHEST, kIntraPriority);
 
   EXPECT_EQ(net::HIGHEST, request_->priority());
 }
 
 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);
 
       // Request should be resumed or paused.
       ASSERT_NE(MockResourceLoader::Status::CANCELED,
                 mock_loader_->OnReadCompleted(
                     expected.substr(written, to_be_written)));
 
       written += to_be_written;
     }
@@ skipping 17 matching lines @@
     base::RunLoop().RunUntilIdle();
   }
   EXPECT_EQ(expected, actual);
 }
 
 TEST_P(MojoAsyncResourceHandlerWithAllocationSizeTest,
        BeginWriteFailsOnReadCompleted) {
   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,
        BeginWriteReturnsShouldWaitOnReadCompleted) {
   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;
     ASSERT_EQ(MockResourceLoader::Status::IDLE, result);
   }
   url_loader_client_.RunUntilResponseBodyArrived();
   ASSERT_TRUE(url_loader_client_.response_body().is_valid());
 
   handler_->set_begin_write_expectation(MOJO_RESULT_UNKNOWN);
 
   while (mock_loader_->status() != MockResourceLoader::Status::CANCELED) {
     char buf[256];
     uint32_t read_size = sizeof(buf);
     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);
     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;
     ASSERT_EQ(MockResourceLoader::Status::IDLE, result);
   }
 
   url_loader_client_.RunUntilResponseBodyArrived();
   ASSERT_TRUE(url_loader_client_.response_body().is_valid());
 
   net::URLRequestStatus status(net::URLRequestStatus::CANCELED,
@@ skipping 194 matching lines @@
     }
   }
   EXPECT_EQ("B", body);
 }
 
 INSTANTIATE_TEST_CASE_P(MojoAsyncResourceHandlerWithAllocationSizeTest,
                         MojoAsyncResourceHandlerWithAllocationSizeTest,
                         ::testing::Values(8, 32 * 2014));
 }  // namespace
 }  // namespace content