Ensures that there's no crash at NetworkContext shutdown if there are outstanding URLLoaders.

NetworkContext::domain_reliability_monitor_ was being destroyed before the URLLoaders that are owned by NetworkContext::url_loader_factories_ because of the declaration order. Reset domain_reliability_monitor_ in the destructor so that it's not used later (since we null-check it before use).

Bug: 906452
Change-Id: Ib4ec198953142672a3991a4c62436733bd8da171
Reviewed-on: https://chromium-review.googlesource.com/c/1343337
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Doug Turner <dougt@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609520}

chrome/browser/domain_reliability/browsertest.cc

@@ -21,6 +21,8 @@
 #include "net/test/embedded_test_server/http_request.h"
 #include "net/test/embedded_test_server/http_response.h"
 #include "net/test/url_request/url_request_failed_job.h"
+#include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
+#include "services/network/public/cpp/simple_url_loader.h"
 #include "url/gurl.h"
 
 namespace domain_reliability {
@@ -194,4 +196,31 @@ IN_PROC_BROWSER_TEST_F(DomainReliabilityBrowserTest, UploadAtShutdown) {
   // upload calls into already-destroyed parts of the component.
 }
 
+// Ensures that there's no crash at NetworkContext shutdown if there are
+// outstanding URLLoaders.
+IN_PROC_BROWSER_TEST_F(DomainReliabilityBrowserTest, RequestAtShutdown) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  GURL hung_url = embedded_test_server()->GetURL("/hung");
+  {
+    mojo::ScopedAllowSyncCallForTesting allow_sync_call;
+    GetNetworkContext()->AddDomainReliabilityContextForTesting(hung_url,
+                                                               hung_url);
+  }
+
+  // Use a SimpleURLLoader so we can leak the mojo pipe, ensuring that URLLoader
+  // doesn't see a connection error before NetworkContext does.
+  auto resource_request = std::make_unique<network::ResourceRequest>();
+  resource_request->url = hung_url;
+  auto simple_loader = network::SimpleURLLoader::Create(
+      std::move(resource_request), TRAFFIC_ANNOTATION_FOR_TESTS);
+  auto* storage_partition =
+      content::BrowserContext::GetDefaultStoragePartition(browser()->profile());
+  simple_loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+      storage_partition->GetURLLoaderFactoryForBrowserProcess().get(),
+      base::BindOnce([](std::unique_ptr<std::string> body) {}));
+
+  simple_loader.release();
+}
+
 }  // namespace domain_reliability

services/network/network_context.cc

@@ -594,6 +594,14 @@ NetworkContext::~NetworkContext() {
 
   if (domain_reliability_monitor_)
     domain_reliability_monitor_->Shutdown();
+  // Because of the order of declaration in the class,
+  // domain_reliability_monitor_ will be destroyed before
+  // |url_loader_factories_| which could own URLLoader's whose destructor call
+  // back into this class and might use domain_reliability_monitor_. So we reset
+  // |domain_reliability_monitor_| here expliclity, instead of changing the
+  // order, because any work calling into |domain_reliability_monitor_| at
+  // shutdown would be unnecessary as the reports would be thrown out.
+  domain_reliability_monitor_.reset();
 
   if (url_request_context_ &&
       url_request_context_->transport_security_state()) {