Implement SetDefersLoading on content::URLLoaderClientImpl

content/child/url_loader_client_impl.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/child/url_loader_client_impl.h"
 
 #include "base/callback.h"
 #include "base/single_thread_task_runner.h"
 #include "content/child/resource_dispatcher.h"
 #include "content/child/url_response_body_consumer.h"
 #include "content/common/resource_messages.h"
 #include "mojo/public/cpp/bindings/associated_group.h"
 #include "net/url_request/redirect_info.h"
 
 namespace content {
 
 URLLoaderClientImpl::URLLoaderClientImpl(
     int request_id,
     ResourceDispatcher* resource_dispatcher,
     scoped_refptr<base::SingleThreadTaskRunner> task_runner)
     : binding_(this),
       request_id_(request_id),
       resource_dispatcher_(resource_dispatcher),
+      is_destroyed_(new base::RefCountedData<bool>(false)),
       task_runner_(std::move(task_runner)) {}
 
 URLLoaderClientImpl::~URLLoaderClientImpl() {
   if (body_consumer_)
     body_consumer_->Cancel();
+  is_destroyed_->data = true;
 }
 
 void URLLoaderClientImpl::Bind(
     mojom::URLLoaderClientAssociatedPtrInfo* client_ptr_info,
     mojo::AssociatedGroup* associated_group) {
   binding_.Bind(client_ptr_info, associated_group);
 }
 
+void URLLoaderClientImpl::SetDefersLoading() {
+  is_deferred_ = true;
+  if (body_consumer_)
+    body_consumer_->SetDefersLoading();
+}
+
+void URLLoaderClientImpl::UnsetDefersLoading() {
+  is_deferred_ = false;
+}
+
+void URLLoaderClientImpl::FlushDeferredMessages() {
+  DCHECK(!is_deferred_);
+  std::vector<IPC::Message> messages;
+  messages.swap(deferred_messages_);
+  bool has_completion_message = false;
+  scoped_refptr<base::RefCountedData<bool>> is_destroyed = is_destroyed_;
+  size_t index = 0;
+  // First, dispatch all messages excluding the followings:
+  //  - response body (dispatched by |body_consumer_|)
+  //  - transfer size change (dispatched later)
+  //  - completion (dispatched by |body_consumer_| or dispatched later)
+  for (index = 0; index < messages.size(); ++index) {
+    if (messages[index].type() == ResourceMsg_RequestComplete::ID) {
+      // The completion message arrives at the end of the message queue.
+      DCHECK(!has_completion_message);
+      DCHECK_EQ(index, messages.size() - 1);
+      has_completion_message = true;
+    } else {
+      Dispatch(messages[index]);
+    }
+    if (is_destroyed->data)

[kinuko, 2016/12/23 13:11:23]

Looking into the usage, I think using WeakPtr is probably more common?  (Have a
WeakPtrFactory, get a WeakPtr of this at the beginning and check if it's null
later)

[yhirano, 2016/12/26 05:08:35]

Done.

+      return;
+    if (is_deferred_) {
+      deferred_messages_.insert(deferred_messages_.begin(),
+                                messages.begin() + index + 1, messages.end());
+      return;
+    }
+  }
+
+  // Dispatch the transfer size update.
+  if (accumulated_transfer_size_diff_during_deferred_ > 0) {
+    auto transfer_size_diff = accumulated_transfer_size_diff_during_deferred_;
+    accumulated_transfer_size_diff_during_deferred_ = 0;
+    resource_dispatcher_->OnTransferSizeUpdated(request_id_,
+                                                transfer_size_diff);
+    if (is_destroyed->data)
+      return;
+    if (is_deferred_) {
+      deferred_messages_.insert(deferred_messages_.begin(),
+                                messages.begin() + index, messages.end());

[kinuko, 2016/12/23 13:11:23]

I'm a little confused, we don't break the for loop (except for returning in the
middle), isn't the index always at the end of the messages after the for loop?

[yhirano, 2016/12/26 05:08:35]

Oops, sorry. Fixed.

[kinuko, 2016/12/26 07:21:17]

Thanks. Should we have a test that must have caught a bug like this?

Let me make sure... so basically we're inserting ResourceMsg_RequestComplete
which must be messages.back(), am I following?  I feel just pushing back the
last one might be clearer, but what do you think?

[yhirano, 2016/12/26 08:22:00]

Done.

+      return;
+    }
+  }
+
+  if (body_consumer_) {
+    // Dispatch the response body.
+    body_consumer_->UnsetDefersLoading();
+    if (is_destroyed->data)
+      return;
+    if (is_deferred_) {
+      deferred_messages_.insert(deferred_messages_.begin(),
+                                messages.begin() + index, messages.end());
+      return;
+    }
+  }
+
+  // Dispatch the completion message.
+  if (has_completion_message) {
+    DCHECK_GT(messages.size(), 0u);
+    DCHECK_EQ(messages.back().type(),
+              static_cast<uint32_t>(ResourceMsg_RequestComplete::ID));
+    Dispatch(messages.back());
+  }
+}
+
 void URLLoaderClientImpl::OnReceiveResponse(
     const ResourceResponseHead& response_head,
     mojom::DownloadedTempFilePtr downloaded_file) {
   has_received_response_ = true;
   if (body_consumer_)
     body_consumer_->Start();
   downloaded_file_ = std::move(downloaded_file);
   Dispatch(ResourceMsg_ReceivedResponse(request_id_, response_head));
 }
 
 void URLLoaderClientImpl::OnReceiveRedirect(
     const net::RedirectInfo& redirect_info,
     const ResourceResponseHead& response_head) {
   DCHECK(!has_received_response_);
   DCHECK(!body_consumer_);
   Dispatch(
       ResourceMsg_ReceivedRedirect(request_id_, redirect_info, response_head));
 }
 
 void URLLoaderClientImpl::OnDataDownloaded(int64_t data_len,
                                            int64_t encoded_data_len) {
   Dispatch(ResourceMsg_DataDownloaded(request_id_, data_len, encoded_data_len));
 }
 
 void URLLoaderClientImpl::OnTransferSizeUpdated(int32_t transfer_size_diff) {
-  resource_dispatcher_->OnTransferSizeUpdated(request_id_,
-                                              transfer_size_diff);
+  if (is_deferred_) {
+    accumulated_transfer_size_diff_during_deferred_ += transfer_size_diff;
+  } else {
+    resource_dispatcher_->OnTransferSizeUpdated(request_id_,
+                                                transfer_size_diff);
+  }
 }
 
 void URLLoaderClientImpl::OnStartLoadingResponseBody(
     mojo::ScopedDataPipeConsumerHandle body) {
   DCHECK(!body_consumer_);
   body_consumer_ = new URLResponseBodyConsumer(
       request_id_, resource_dispatcher_, std::move(body), task_runner_);
   if (has_received_response_)
     body_consumer_->Start();
+  if (is_deferred_)
+    body_consumer_->SetDefersLoading();
 }
 
 void URLLoaderClientImpl::OnComplete(
     const ResourceRequestCompletionStatus& status) {
   if (!body_consumer_) {
     Dispatch(ResourceMsg_RequestComplete(request_id_, status));
     return;
   }
   body_consumer_->OnComplete(status);
 }
 
 void URLLoaderClientImpl::Dispatch(const IPC::Message& message) {
-  resource_dispatcher_->OnMessageReceived(message);
+  if (is_deferred_) {
+    deferred_messages_.push_back(message);
+  } else if (deferred_messages_.size() > 0) {
+    deferred_messages_.push_back(message);
+    FlushDeferredMessages();
+  } else {
+    resource_dispatcher_->OnMessageReceived(message);

[kinuko, 2016/12/23 13:11:23]

At this point couldn't we directly call ResourceDispatcher::DispatchMessage()? 
I feel it's less confusing as OnMessageReceived itself has its own deferred
message handling (& sanity checks that don't seem necessary in our case).

[yhirano, 2016/12/26 05:08:35]

ResourceDispatcher::DispatchMessage is a private method and I think exposing it
also has a risk that someone accidentally calls it.

[kinuko, 2016/12/26 07:21:17]

But eventually we plan to get rid of OnMessageReceived and the non-mojo code,
right?  I don't want to leave the possibility that the new code weirdly depends
on the old code that's supposed to be go away.  If you're uncomfortable could we
add another public method with Mojo suffix, or making URLLoaderClientImpl friend
of RD?

[yhirano, 2016/12/26 08:22:00]

Done.

+  }
 }
 
 }  // namespace content