Update MojoAsyncResourceHandler. Always send response's body datapipe.

Instead of creating the response's body data pipe during the first call to
OnWillRead(), it is now done in OnWillStart(). The data pipe
consumer handle is available in OnResponseStarted().

It allows the MojoAsyncResourceHandler to call:
 * URLLoader::OnReceiveResponse(response_headers)
 * URLLoader::OnStartLoadingResponseBody(response_body)
at the same time.

The goal is to guarantee the response's body is always sent, even if it doesn't
contains any data.

Bug: 831155, 826868
Change-Id: I85d84d05ad3fd362d96394834e14297d550f5f13
Reviewed-on: https://chromium-review.googlesource.com/c/1352254Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612137}

content/browser/loader/mojo_async_resource_handler.cc

@@ -154,7 +154,7 @@ void MojoAsyncResourceHandler::OnRequestRedirected(
   // Unlike OnResponseStarted, OnRequestRedirected will NOT be preceded by
   // OnWillRead.
   DCHECK(!has_controller());
-  DCHECK(!shared_writer_);
+  DCHECK(shared_writer_);
 
   request()->LogBlockedBy("MojoAsyncResourceHandler");
   HoldController(std::move(controller));
@@ -204,6 +204,9 @@ void MojoAsyncResourceHandler::OnResponseStarted(
         std::vector<uint8_t>(data, data + metadata->size()));
   }
 
+  url_loader_client_->OnStartLoadingResponseBody(
+      std::move(response_body_consumer_handle_));
+
   if (url_loader_options_ &
       network::mojom::kURLLoadOptionPauseOnResponseStarted) {
     did_defer_on_response_started_ = true;
@@ -219,6 +222,25 @@ void MojoAsyncResourceHandler::OnResponseStarted(
 void MojoAsyncResourceHandler::OnWillStart(
     const GURL& url,
     std::unique_ptr<ResourceController> controller) {
+  // Create the response's body datapipe.
+  MojoCreateDataPipeOptions options;
+  options.struct_size = sizeof(MojoCreateDataPipeOptions);
+  options.flags = MOJO_CREATE_DATA_PIPE_FLAG_NONE;
+  options.element_num_bytes = 1;
+  options.capacity_num_bytes = g_allocation_size;
+  mojo::ScopedDataPipeProducerHandle response_body_producer_handle_;
+  if (mojo::CreateDataPipe(&options, &response_body_producer_handle_,
+                           &response_body_consumer_handle_) != MOJO_RESULT_OK) {
+    controller->CancelWithError(net::ERR_INSUFFICIENT_RESOURCES);
+    return;
+  }
+
+  shared_writer_ = new SharedWriter(std::move(response_body_producer_handle_));
+  handle_watcher_.Watch(
+      shared_writer_->writer(), MOJO_HANDLE_SIGNAL_WRITABLE,
+      base::BindRepeating(&MojoAsyncResourceHandler::OnWritable,
+                          base::Unretained(this)));
+
   if (GetRequestInfo()->is_upload_progress_enabled() &&
       request()->has_upload()) {
     upload_progress_tracker_ = CreateUploadProgressTracker(
@@ -245,33 +267,6 @@ void MojoAsyncResourceHandler::OnWillRead(
     return;
   }
 
-  bool first_call = false;
-  if (!shared_writer_) {
-    first_call = true;
-    MojoCreateDataPipeOptions options;
-    options.struct_size = sizeof(MojoCreateDataPipeOptions);
-    options.flags = MOJO_CREATE_DATA_PIPE_FLAG_NONE;
-    options.element_num_bytes = 1;
-    options.capacity_num_bytes = g_allocation_size;
-    mojo::ScopedDataPipeProducerHandle producer;
-    mojo::ScopedDataPipeConsumerHandle consumer;
-
-    MojoResult result = mojo::CreateDataPipe(&options, &producer, &consumer);
-    if (result != MOJO_RESULT_OK) {
-      controller->CancelWithError(net::ERR_INSUFFICIENT_RESOURCES);
-      return;
-    }
-    DCHECK(producer.is_valid());
-    DCHECK(consumer.is_valid());
-
-    response_body_consumer_handle_ = std::move(consumer);
-    shared_writer_ = new SharedWriter(std::move(producer));
-    handle_watcher_.Watch(shared_writer_->writer(), MOJO_HANDLE_SIGNAL_WRITABLE,
-                          base::Bind(&MojoAsyncResourceHandler::OnWritable,
-                                     base::Unretained(this)));
-    handle_watcher_.ArmOrNotify();
-  }
-
   bool defer = false;
   if (!AllocateWriterIOBuffer(&buffer_, &defer)) {
     controller->CancelWithError(net::ERR_INSUFFICIENT_RESOURCES);
@@ -288,11 +283,15 @@ void MojoAsyncResourceHandler::OnWillRead(
     return;
   }
 
+  if (!did_check_for_intermediary_buffer_) {
+    did_check_for_intermediary_buffer_ = true;
+
     // The first call to OnWillRead must return a buffer of at least
     // kMinAllocationSize. If the Mojo buffer is too small, need to allocate an
     // intermediary buffer.
-  if (first_call && static_cast<size_t>(buffer_->size()) < kMinAllocationSize) {
-    // The allocated buffer is too small, so need to create an intermediary one.
+    if (static_cast<size_t>(buffer_->size()) < kMinAllocationSize) {
+      // The allocated buffer is too small, so need to create an intermediary
+      // one.
       if (EndWrite(0) != MOJO_RESULT_OK) {
         controller->CancelWithError(net::ERR_INSUFFICIENT_RESOURCES);
         return;
@@ -301,6 +300,7 @@ void MojoAsyncResourceHandler::OnWillRead(
       is_using_io_buffer_not_from_writer_ = true;
       buffer_ = base::MakeRefCounted<net::IOBufferWithSize>(kMinAllocationSize);
     }
+  }
 
   *buf = buffer_;
   *buf_size = buffer_->size();
@@ -356,13 +356,6 @@ void MojoAsyncResourceHandler::OnReadCompleted(
     EnsureTransferSizeUpdate();
   }
 
-  if (response_body_consumer_handle_.is_valid()) {
-    // Send the data pipe on the first OnReadCompleted call.
-    url_loader_client_->OnStartLoadingResponseBody(
-        std::move(response_body_consumer_handle_));
-    response_body_consumer_handle_.reset();
-  }
-
   if (is_using_io_buffer_not_from_writer_) {
     // Couldn't allocate a large enough buffer on the data pipe in OnWillRead.
     DCHECK_EQ(0u, buffer_bytes_read_);

content/browser/loader/mojo_async_resource_handler.h

@@ -155,6 +155,7 @@ class CONTENT_EXPORT MojoAsyncResourceHandler
   bool did_defer_on_writing_ = false;
   bool did_defer_on_redirect_ = false;
   bool did_defer_on_response_started_ = false;
+  bool did_check_for_intermediary_buffer_ = false;
 
   int64_t total_written_bytes_ = 0;
 

content/browser/loader/mojo_async_resource_handler_unittest.cc

@@ -1275,7 +1275,7 @@ TEST_P(
 
   ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
 
-  ASSERT_FALSE(url_loader_client_.response_body().is_valid());
+  ASSERT_TRUE(url_loader_client_.response_body().is_valid());
 
   ASSERT_EQ(MockResourceLoader::Status::IDLE,
             mock_loader_->OnReadCompleted("A"));
@@ -1328,7 +1328,7 @@ TEST_P(
   ASSERT_EQ(MockResourceLoader::Status::IDLE,
             mock_loader_->OnReadCompleted("B"));
 
-  ASSERT_FALSE(url_loader_client_.response_body().is_valid());
+  ASSERT_TRUE(url_loader_client_.response_body().is_valid());
   url_loader_client_.RunUntilResponseBodyArrived();
   ASSERT_TRUE(url_loader_client_.response_body().is_valid());
 
@@ -1542,8 +1542,6 @@ TEST_F(MojoAsyncResourceHandlerTest,
 
   // Prepare for loader read complete.
   ASSERT_TRUE(CallOnWillStartAndOnResponseStarted());
-  EXPECT_EQ(MockResourceLoader::Status::IDLE,
-            mock_loader_->OnWillStart(request_->url()));
   EXPECT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
   // Only headers are read by the time the response is started.
   mock_loader_->OnReadCompleted(kResponseHeaders);