Fix network socket starvation.

This fixes https://crbug.com/872284 and adds a regression test.

The bug can be triggered when the PDFIframeNavigationThrottle cancels the
PDF download. There was some code in ResourceLoader made to ignore the
cancellation. It causes the net socket to never been released. Only 6
network socket can be used on a website, so there was some kind of
resource starvation.

Bug: 872284
Change-Id: I9fcaf8da409da38e0cb4b2e9c6ad57bb2fab911c
Reviewed-on: https://chromium-review.googlesource.com/c/1348036
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612172}

content/browser/loader/mojo_async_resource_handler.cc

@@ -414,6 +414,7 @@ void MojoAsyncResourceHandler::FollowRedirect(
 }
 
 void MojoAsyncResourceHandler::ProceedWithResponse() {
+  was_proceed_with_response_called_ = true;
   DCHECK(did_defer_on_response_started_);
 
   request()->LogUnblocked();
@@ -644,8 +645,20 @@ void MojoAsyncResourceHandler::Cancel(uint32_t custom_reason,
   if (custom_reason == network::mojom::URLLoader::kClientDisconnectReason)
     info->set_custom_cancel_reason(description);
 
-  ResourceDispatcherHostImpl::Get()->CancelRequestFromRenderer(
-      GlobalRequestID(info->GetChildID(), info->GetRequestID()));
+  // Navigation requests are handled by the browser process until
+  // ProceedWithResponse() is called.
+  bool canceled_from_browser =
+      !was_proceed_with_response_called_ &&
+      IsResourceTypeFrame(
+          ResourceRequestInfoImpl::ForRequest(request())->GetResourceType());
+
+  if (canceled_from_browser) {
+    ResourceDispatcherHostImpl::Get()->CancelRequest(info->GetChildID(),
+                                                     info->GetRequestID());
+  } else {
+    ResourceDispatcherHostImpl::Get()->CancelRequestFromRenderer(
+        GlobalRequestID(info->GetChildID(), info->GetRequestID()));
+  }
 }
 
 int64_t MojoAsyncResourceHandler::CalculateRecentlyReceivedBytes() {

content/browser/loader/mojo_async_resource_handler.h

@@ -149,6 +149,7 @@ class CONTENT_EXPORT MojoAsyncResourceHandler
   bool has_checked_for_sufficient_resources_ = false;
   bool sent_received_response_message_ = false;
   bool is_using_io_buffer_not_from_writer_ = false;
+  bool was_proceed_with_response_called_ = false;
   // True if OnWillRead was deferred, in order to wait to be able to allocate a
   // buffer.
   bool did_defer_on_will_read_ = false;

content/browser/navigation_browsertest.cc

@@ -9,6 +9,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/post_task.h"
+#include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/threading/thread_restrictions.h"
 #include "build/build_config.h"
@@ -40,6 +41,8 @@
 #include "content/public/test/download_test_observer.h"
 #include "content/public/test/navigation_handle_observer.h"
 #include "content/public/test/test_navigation_observer.h"
+#include "content/public/test/test_navigation_throttle.h"
+#include "content/public/test/test_navigation_throttle_inserter.h"
 #include "content/shell/browser/shell.h"
 #include "content/shell/browser/shell_content_browser_client.h"
 #include "content/shell/browser/shell_download_manager_delegate.h"
@@ -1446,4 +1449,43 @@ IN_PROC_BROWSER_TEST_F(NavigationBrowserTest, OpenerNavigation_DownloadPolicy) {
                                 1);
 }
 
+// Regression test for https://crbug.com/872284.
+// A NavigationThrottle cancels a download in WillProcessResponse.
+// The navigation request must be canceled and it must also cancel the network
+// request. Failing to do so resulted in the network socket being leaked.
+IN_PROC_BROWSER_TEST_F(NavigationDownloadBrowserTest,
+                       CancelDownloadOnResponseStarted) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  GURL url(embedded_test_server()->GetURL("/title1.html"));
+  NavigateToURL(shell(), url);
+
+  // Block every iframe in WillProcessResponse.
+  content::TestNavigationThrottleInserter throttle_inserter(
+      shell()->web_contents(),
+      base::BindLambdaForTesting(
+          [&](NavigationHandle* handle) -> std::unique_ptr<NavigationThrottle> {
+            auto throttle = std::make_unique<TestNavigationThrottle>(handle);
+            throttle->SetResponse(TestNavigationThrottle::WILL_PROCESS_RESPONSE,
+                                  TestNavigationThrottle::SYNCHRONOUS,
+                                  NavigationThrottle::CANCEL_AND_IGNORE);
+
+            return throttle;
+          }));
+
+  // Insert enough iframes so that if sockets are not properly released: there
+  // will not be enough of them to complete all navigations. As of today, only 6
+  // sockets can be used simultaneously. So using 7 iframes is enough. This test
+  // uses 33 as a margin.
+  EXPECT_TRUE(ExecJs(shell(), R"(
+    for(let i = 0; i<33; ++i) {
+      let iframe = document.createElement('iframe');
+      iframe.src = './download-test1.lib'
+      document.body.appendChild(iframe);
+    }
+  )"));
+
+  EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+}
+
 }  // namespace content