[Mojo-Loading] OnStartLoadingResponseBody should be called after OnReceiveResponse

content/browser/loader/mojo_async_resource_handler.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 <utility>
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/containers/hash_tables.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/time/time.h"
 #include "content/browser/loader/downloaded_temp_file_impl.h"
 #include "content/browser/loader/netlog_observer.h"
 #include "content/browser/loader/resource_controller.h"
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/browser/loader/resource_request_info_impl.h"
 #include "content/common/resource_request_completion_status.h"
 #include "content/public/browser/global_request_id.h"
 #include "content/public/browser/resource_dispatcher_host_delegate.h"
 #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"
 #include "net/url_request/redirect_info.h"
 
 namespace content {
 namespace {
 
 int g_allocation_size = MojoAsyncResourceHandler::kDefaultAllocationSize;
 
@@ skipping 168 matching lines @@
     return false;
 
   if (!shared_writer_) {
     MojoCreateDataPipeOptions options;
     options.struct_size = sizeof(MojoCreateDataPipeOptions);
     options.flags = MOJO_CREATE_DATA_PIPE_OPTIONS_FLAG_NONE;
     options.element_num_bytes = 1;
     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,
                                      base::Unretained(this)));
 
     bool defer = false;
     scoped_refptr<net::IOBufferWithSize> buffer;
     if (!AllocateWriterIOBuffer(&buffer, &defer))
       return false;
     if (!defer) {
@@ skipping 25 matching lines @@
   if (!bytes_read)
     return true;
 
   const ResourceRequestInfoImpl* info = GetRequestInfo();
   if (info->ShouldReportRawHeaders()) {
     auto transfer_size_diff = CalculateRecentlyReceivedBytes();
     if (transfer_size_diff > 0)
       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_);
     buffer_bytes_read_ = bytes_read;
     if (!CopyReadDataToDataPipe(defer))
       return false;
     if (*defer) {
       request()->LogBlockedBy("MojoAsyncResourceHandler");
       did_defer_on_writing_ = true;
     }
@@ skipping 209 matching lines @@
     mojom::URLLoaderAssociatedRequest mojo_request,
     mojom::URLLoaderClientAssociatedPtr url_loader_client) {
   binding_.Unbind();
   binding_.Bind(std::move(mojo_request));
   binding_.set_connection_error_handler(
       base::Bind(&MojoAsyncResourceHandler::Cancel, base::Unretained(this)));
   url_loader_client_ = std::move(url_loader_client);
 }
 
 }  // namespace content