[Mojo-Loading] OnStartLoadingResponseBody should be called after OnReceiveResponse

content/browser/loader/mojo_async_resource_handler.cc

Index: content/browser/loader/mojo_async_resource_handler.cc
diff --git a/content/browser/loader/mojo_async_resource_handler.cc b/content/browser/loader/mojo_async_resource_handler.cc
index ce87c93285ef0973270c68da037327d9a14db326..3002f25d48c29d0ad6a2482b001c3ebd4de5337f 100644
--- a/content/browser/loader/mojo_async_resource_handler.cc
+++ b/content/browser/loader/mojo_async_resource_handler.cc
@@ -24,7 +24,6 @@
 #include "content/public/common/resource_response.h"
 #include "mojo/public/c/system/data_pipe.h"
 #include "mojo/public/cpp/bindings/message.h"
-#include "mojo/public/cpp/system/data_pipe.h"
 #include "net/base/io_buffer.h"
 #include "net/base/load_flags.h"
 #include "net/base/mime_sniffer.h"
@@ -213,11 +212,12 @@ bool MojoAsyncResourceHandler::OnWillRead(scoped_refptr<net::IOBuffer>* buf,
     options.capacity_num_bytes = g_allocation_size;
     mojo::DataPipe data_pipe(options);
 
-    url_loader_client_->OnStartLoadingResponseBody(
-        std::move(data_pipe.consumer_handle));
-    if (!data_pipe.producer_handle.is_valid())
+    if (!data_pipe.producer_handle.is_valid() ||
+        !data_pipe.consumer_handle.is_valid()) {

[mmenke, 2017/01/18 16:26:12]

Random comments on the mojo API:

C++ constructors that can fail seem a bit weird.  Something like
mojo::CreateDataPipe() that returns a null DataPipe on failure seems a bit more
natural.  (Or bool mojo::CreateDataPipes(options, &producer_handle,
&consumer_handle) WARN_UNUSED_RESULT)

Also, is it really possible for only one of these to be valid?

[mmenke, 2017/01/18 16:26:12]

Do we have any tests where this fails?

[yhirano, 2017/01/19 03:25:49]

It looks impossible:
https://cs.chromium.org/chromium/src/mojo/public/cpp/system/data_pipe.h?sq=pa...
has its own DCHECK. I replaced the branch with DCHECKs.

       return false;
+    }
 
+    response_body_consumer_handle_ = std::move(data_pipe.consumer_handle);
     shared_writer_ = new SharedWriter(std::move(data_pipe.producer_handle));
     handle_watcher_.Start(shared_writer_->writer(), MOJO_HANDLE_SIGNAL_WRITABLE,
                           base::Bind(&MojoAsyncResourceHandler::OnWritable,
@@ -263,6 +263,13 @@ bool MojoAsyncResourceHandler::OnReadCompleted(int bytes_read, bool* defer) {
       url_loader_client_->OnTransferSizeUpdated(transfer_size_diff);
   }
 
+  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 buffer on the data pipe in OnWillRead.
     DCHECK_EQ(0u, buffer_bytes_read_);