[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 "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 137 matching lines @@
   mojom::DownloadedTempFilePtr downloaded_file_ptr;
   if (!response->head.download_file_path.empty()) {
     downloaded_file_ptr = DownloadedTempFileImpl::Create(info->GetChildID(),
                                                          info->GetRequestID());
     rdh_->RegisterDownloadedTempFile(info->GetChildID(), info->GetRequestID(),
                                      response->head.download_file_path);
   }
 
   url_loader_client_->OnReceiveResponse(response->head,
                                         std::move(downloaded_file_ptr));
+
+  if (data_pipe_consumer_handle_.is_valid()) {
+    // OnStartLoadingResponseBody waited for OnReceiveResponse. Now it can be
+    // sent.
+    url_loader_client_->OnStartLoadingResponseBody(
+        std::move(data_pipe_consumer_handle_));

[mmenke, 2017/01/17 16:13:57]

std::move leaves the original unique_ptr pointer in an indeterminate state,
doesn't it?  Is data_pipe_consumer_handle_.is_valid() guaranteed to work after
calling std::move on it?

[dcheng, 2017/01/17 23:20:18]

Note that std::unique_ptr's moved from state is actually well-specified in
[unique.ptr]/4:

Additionally, u can, upon request, transfer ownership to another unique pointer
u2. Upon completion of
such a transfer, the following postconditions hold:
— u2.p is equal to the pre-transfer u.p,
— u.p is equal to nullptr, and
— if the pre-transfer u.d maintained state, such state has been transferred to
u2.d.

That being said, this is a good question to ask of movable types in general
(though from brief inspection, it appears that the same holds true for the mojo
scoped helpers)

[yhirano, 2017/01/18 09:10:45]

Done (in OnReadCompleted), though I'm not sure if it's needed. According to the
ScopedHandleBase comments,

"
  It also provides (emulated) move semantics, again 
  along the lines of C++11's |unique_ptr| (and exactly
  like Chromium's |scoped_ptr|).
"

so I expect that it will be cleared when consumed by a move-constructor.

+  }
+
   return true;
 }
 
 bool MojoAsyncResourceHandler::OnWillStart(const GURL& url, bool* defer) {
   return true;
 }
 
 bool MojoAsyncResourceHandler::OnWillRead(scoped_refptr<net::IOBuffer>* buf,
                                           int* buf_size,
                                           int min_size) {
   DCHECK_EQ(-1, min_size);
 
   if (!CheckForSufficientResource())
     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 (sent_received_response_message_) {
+      // OnResponseStarted has already been sent. It's OK to send this message.
+      url_loader_client_->OnStartLoadingResponseBody(
+          std::move(data_pipe.consumer_handle));

[mmenke, 2017/01/17 16:13:57]

Is there any significant perf penalty with just always doing this in the first
MojoAsyncResourceHandler::OnResponseStarted call?  I prefer keeping the paths as
similar as we can, so it's less likely the paths will break independently of
each other.

(On a side note, last month, rdsmith was discussing fixing the ordering here by
having MimeSniffingResourceHandler not call OnWillRead early, and instead use
its own buffer in this case, which would allow us not to do this juggling.  Also
unclear what the perf cost of that would be.  That would also let us get rid of
the min_size parameter, I believe)

[Randy Smith (Not in Mondays), 2017/01/17 16:45:10]

If you wanted to switch course, I don't currently think that that option would
be much more complicated than this one, and it also would remove implicit
dependencies through the ResourceHandler stack on OnWillRead() returning a large
enough sized buffer for MimeSniffingResourceHandler.  If you do this, I'd
suggest putting in UMA just so we can confirm we're not killing ourselves with
too much copying.

(I started to rough out the approach on the plane yesterday, but it's a side
project I got into because the implicit dependencies annoyed me, and it's
unlikely I'll actually finish it before I leave on vacation in a week.)

[yhirano, 2017/01/18 09:10:45]

I found that moving this to OnReadCompleted simplified the code (Sending the
message from OnResponseStarted unconditionally may increase memory consumption
for responses without body). Done.

+    } else {
+      // To keep the message ordering, store the handle for a while.
+      data_pipe_consumer_handle_ = std::move(data_pipe.consumer_handle);
+    }
     if (!data_pipe.producer_handle.is_valid())
       return false;
 
     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;
@@ skipping 252 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