Deal with canceled requests when flushing deferred messages.

Flushing deferred messages might lead to a request being canceled
(e.g. when an ImageResource loads a corrupt image). The code didn't
fully take this into account which would cause crashes (and resource
leaks if it would have survived).

BUG=

Review-Url: https://codereview.chromium.org/2425173003
Cr-Commit-Position: refs/heads/master@{#427298}

content/child/resource_dispatcher.cc

@@ -173,16 +173,13 @@ bool ResourceDispatcher::OnMessageReceived(const IPC::Message& message) {
     request_info->deferred_message_queue.push_back(new IPC::Message(message));
     return true;
   }
+
   // Make sure any deferred messages are dispatched before we dispatch more.
   if (!request_info->deferred_message_queue.empty()) {
-    FlushDeferredMessages(request_id);
-    request_info = GetPendingRequestInfo(request_id);
-    DCHECK(request_info);
-    if (request_info->is_deferred) {
     request_info->deferred_message_queue.push_back(new IPC::Message(message));
+    FlushDeferredMessages(request_id);
     return true;
   }
-  }
 
   DispatchMessage(message);
   return true;
@@ -580,11 +577,8 @@ void ResourceDispatcher::DispatchMessage(const IPC::Message& message) {
 }
 
 void ResourceDispatcher::FlushDeferredMessages(int request_id) {
-  PendingRequestMap::iterator it = pending_requests_.find(request_id);
-  if (it == pending_requests_.end())  // The request could have become invalid.
-    return;
-  PendingRequestInfo* request_info = it->second.get();
-  if (request_info->is_deferred)
+  PendingRequestInfo* request_info = GetPendingRequestInfo(request_id);
+  if (!request_info || request_info->is_deferred)
     return;
   // Because message handlers could result in request_info being destroyed,
   // we need to work with a stack reference to the deferred queue.
@@ -595,17 +589,21 @@ void ResourceDispatcher::FlushDeferredMessages(int request_id) {
     q.pop_front();
     DispatchMessage(*m);
     delete m;
-    // If this request is deferred in the context of the above message, then
-    // we should honor the same and stop dispatching further messages.
     // We need to find the request again in the list as it may have completed
     // by now and the request_info instance above may be invalid.
-    PendingRequestMap::iterator index = pending_requests_.find(request_id);
-    if (index != pending_requests_.end()) {
-      PendingRequestInfo* pending_request = index->second.get();
-      if (pending_request->is_deferred) {
-        pending_request->deferred_message_queue.swap(q);
+    request_info = GetPendingRequestInfo(request_id);
+    if (!request_info) {
+      // The recipient is gone, the messages won't be handled and
+      // resources they might hold won't be released. Explicitly release
+      // them from here so that they won't leak.
+      ReleaseResourcesInMessageQueue(&q);
       return;
     }
+    // If this request is deferred in the context of the above message, then
+    // we should honor the same and stop dispatching further messages.
+    if (request_info->is_deferred) {
+      request_info->deferred_message_queue.swap(q);
+      return;
     }
   }
 }

content/child/resource_dispatcher_unittest.cc

@@ -93,6 +93,11 @@ class TestRequestPeer : public RequestPeer {
     EXPECT_FALSE(context_->complete);
     context_->data.append(data->payload(), data->length());
     context_->total_encoded_data_length += data->encoded_data_length();
+
+    if (context_->cancel_on_receive_data) {
+      dispatcher_->Cancel(context_->request_id);
+      context_->cancelled = true;
+    }
   }
 
   void OnCompletedRequest(int error_code,
@@ -117,6 +122,7 @@ class TestRequestPeer : public RequestPeer {
     int seen_redirects = 0;
 
     bool cancel_on_receive_response = false;
+    bool cancel_on_receive_data = false;
     bool received_response = false;
 
     // Data received. If downloading to file, remains empty.
@@ -810,6 +816,113 @@ TEST_F(ResourceDispatcherTest, CancelDeferredRequest) {
   EXPECT_EQ(0, peer_context.seen_redirects);
 }
 
+// Checks cancelling a request while flushing deferred requests from
+// the FlushDeferredMessages() task.
+TEST_F(ResourceDispatcherTest, CancelWhileFlushingDeferredRequests) {
+  std::unique_ptr<ResourceRequest> request(CreateResourceRequest(false));
+  TestRequestPeer::Context peer_context;
+  int request_id = StartAsync(std::move(request), NULL, &peer_context);
+
+  // Cancel the request when the data message is handled.
+  peer_context.cancel_on_receive_data = true;
+
+  int id = ConsumeRequestResource();
+  EXPECT_EQ(0u, queued_messages());
+
+  dispatcher()->SetDefersLoading(request_id, true);
+  NotifyReceivedResponse(id);
+  NotifySetDataBuffer(id, strlen(kTestPageContents));
+  NotifyDataReceived(id, kTestPageContents);
+
+  // None of the messages should have been processed yet.
+  EXPECT_EQ("", peer_context.data);
+  EXPECT_FALSE(peer_context.complete);
+  EXPECT_EQ(0u, queued_messages());
+
+  dispatcher()->SetDefersLoading(request_id, false);
+
+  // Make sure that the FlushDeferredMessages() task posted from
+  // SetDefersLoading() is run. It should dispatch all the deferred
+  // messages.
+  base::RunLoop().RunUntilIdle();
+
+  // When the deferred DataReceived is dispatched, the handler will
+  // cancel the request, but the ACK is sent after the handler
+  // returns, so the cancel request ends up before the ACK in the
+  // message queue.
+  ConsumeCancelRequest(id);
+  ConsumeDataReceived_ACK(id);
+
+  // The data was consumed before the handler canceled
+  // the request, so the data should have been received.
+  EXPECT_EQ(kTestPageContents, peer_context.data);
+  EXPECT_FALSE(peer_context.complete);
+  EXPECT_EQ(0u, queued_messages());
+}
+
+// Checks cancelling a request while flushing deferred requests from
+// OnMessageReceived().
+TEST_F(ResourceDispatcherTest,
+       CancelWhileFlushingDeferredRequestsFromOnMessageReceived) {
+  std::unique_ptr<ResourceRequest> request(CreateResourceRequest(false));
+  TestRequestPeer::Context peer_context;
+  int request_id = StartAsync(std::move(request), NULL, &peer_context);
+
+  // Cancel the request when the data message is handled.
+  peer_context.cancel_on_receive_data = true;
+
+  int id = ConsumeRequestResource();
+  EXPECT_EQ(0u, queued_messages());
+
+  dispatcher()->SetDefersLoading(request_id, true);
+  NotifyReceivedResponse(id);
+  NotifySetDataBuffer(id, strlen(kTestPageContents));
+  NotifyDataReceived(id, kTestPageContents);
+
+  // None of the messages should have been processed yet.
+  EXPECT_EQ("", peer_context.data);
+  EXPECT_FALSE(peer_context.complete);
+  EXPECT_EQ(0u, queued_messages());
+
+  dispatcher()->SetDefersLoading(request_id, false);
+
+  // SetDefersLoading() posts a task to run FlushDeferredMessages() to dispatch
+  // the deferred messages. Since the message loop hasn't been run yet the
+  // task hasn't been run either and no IPC-messages should have been
+  // dispatched.
+  EXPECT_EQ("", peer_context.data);
+  EXPECT_FALSE(peer_context.complete);
+  EXPECT_EQ(0u, queued_messages());
+
+  // Calling NotifyRequestComplete() here, before the task from
+  // SetDefersLoading() has been run, triggers the flush in
+  // OnMessageReceived().
+  NotifyRequestComplete(id, strlen(kTestPageContents));
+
+  // When the deferred DataReceived is dispatched, the handler will
+  // cancel the request, but the ACK is sent after the handler
+  // returns, so the cancel request ends up before the ACK in the
+  // message queue.
+  ConsumeCancelRequest(id);
+  ConsumeDataReceived_ACK(id);
+
+  // The data was consumed before the handler canceled
+  // the request, so the data should have been received.
+  EXPECT_EQ(kTestPageContents, peer_context.data);
+  EXPECT_FALSE(peer_context.complete);
+  EXPECT_EQ(0u, queued_messages());
+
+  // Make sure that the FlushDeferredMessages() task posted from
+  // SetDefersLoading() is run. The messages should already have been
+  // flushed above, so it should be a NOOP.
+  base::RunLoop().RunUntilIdle();
+
+  // Check that the task didn't change anything.
+  EXPECT_EQ(kTestPageContents, peer_context.data);
+  EXPECT_FALSE(peer_context.complete);
+  EXPECT_EQ(0u, queued_messages());
+}
+
 TEST_F(ResourceDispatcherTest, DownloadToFile) {
   std::unique_ptr<ResourceRequest> request(CreateResourceRequest(true));
   TestRequestPeer::Context peer_context;