Fix crash in web request proxies connection error handler

When WebRequestAPI is destroyed, it posts a task to the IO thread to
shutdown the proxies. This crash can happen when the WebRequestAPI is
destroyed, and a connection error happens on one of the proxies (which
accesses data destroyed by the profile) before the ProxySet can be fully
shutdown on the IO thread. This change adds an atomic flag to check to
make sure the WebRequestAPI has not been destroyed before running error
handlers.

Bug: 878366
Change-Id: I2f7b98c1d6df7c53a5917444ac996e9b5ef4e794
Reviewed-on: https://chromium-review.googlesource.com/1196031Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587749}

chrome/browser/extensions/api/web_request/web_request_apitest.cc

@@ -33,6 +33,8 @@
 #include "chrome/browser/net/profile_network_context_service_factory.h"
 #include "chrome/browser/net/system_network_context_manager.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_destroyer.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/search/one_google_bar/one_google_bar_loader.h"
 #include "chrome/browser/search/one_google_bar/one_google_bar_service.h"
 #include "chrome/browser/search/one_google_bar/one_google_bar_service_factory.h"
@@ -92,6 +94,7 @@
 #include "services/network/public/cpp/features.h"
 #include "services/network/public/cpp/resource_request.h"
 #include "services/network/public/cpp/simple_url_loader.h"
+#include "services/network/test/test_url_loader_client.h"
 #include "third_party/blink/public/platform/web_input_event.h"
 
 #if defined(OS_CHROMEOS)
@@ -1532,6 +1535,54 @@ IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest,
   EXPECT_FALSE(has_connection_error);
 }
 
+// Regression test for http://crbug.com/878366.
+IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest,
+                       WebRequestApiDoesNotCrashOnErrorAfterProfileDestroyed) {
+  if (!base::FeatureList::IsEnabled(network::features::kNetworkService))
+    return;
+
+  ASSERT_TRUE(StartEmbeddedTestServer());
+
+  // Create a profile that will be destroyed later.
+  base::ScopedAllowBlockingForTesting allow_blocking;
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  Profile* tmp_profile = Profile::CreateProfile(
+      profile_manager->user_data_dir().AppendASCII("profile"), nullptr,
+      Profile::CreateMode::CREATE_MODE_SYNCHRONOUS);
+
+  // Create a WebRequestAPI instance that we can control the lifetime of.
+  auto api = std::make_unique<WebRequestAPI>(tmp_profile);
+  // Add a listener to make sure we proxy.
+  api->OnListenerAdded(EventListenerInfo("name", "id1", GURL(), tmp_profile));
+  content::BrowserContext::GetDefaultStoragePartition(tmp_profile)
+      ->FlushNetworkInterfaceForTesting();
+
+  network::mojom::URLLoaderFactoryPtr factory;
+  auto request = mojo::MakeRequest(&factory);
+  api->MaybeProxyURLLoaderFactory(nullptr, false, &request);
+  auto params = network::mojom::URLLoaderFactoryParams::New();
+  params->process_id = 0;
+  content::BrowserContext::GetDefaultStoragePartition(tmp_profile)
+      ->GetNetworkContext()
+      ->CreateURLLoaderFactory(std::move(request), std::move(params));
+
+  network::TestURLLoaderClient client;
+  network::mojom::URLLoaderPtr loader;
+  network::ResourceRequest resource_request;
+  resource_request.url = embedded_test_server()->GetURL("/hung");
+  factory->CreateLoaderAndStart(
+      mojo::MakeRequest(&loader), 0, 0, network::mojom::kURLLoadOptionNone,
+      resource_request, client.CreateInterfacePtr(),
+      net::MutableNetworkTrafficAnnotationTag(TRAFFIC_ANNOTATION_FOR_TESTS));
+
+  // Destroy profile, unbind client to cause a connection error, and delete the
+  // WebRequestAPI. This will cause the connection error that will reach the
+  // proxy before the ProxySet shutdown code runs on the IO thread.
+  ProfileDestroyer::DestroyProfileWhenAppropriate(tmp_profile);
+  client.Unbind();
+  api.reset();
+}
+
 // Test fixture which sets a custom NTP Page.
 class NTPInterceptionWebRequestAPITest : public ExtensionApiTest {
  public:

extensions/browser/api/web_request/web_request_api.cc

@@ -516,6 +516,7 @@ WebRequestAPI::WebRequestAPI(content::BrowserContext* context)
 }
 
 WebRequestAPI::~WebRequestAPI() {
+  proxies_->SetAPIDestroyed();
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::BindOnce(&ProxySet::Shutdown, std::move(proxies_)));

extensions/browser/api/web_request/web_request_api.h

@@ -126,6 +126,11 @@ class WebRequestAPI
       return is_shutdown_;
     }
 
+    // Threadsafe methods to check and set if the WebRequestAPI has been
+    // destroyed.
+    void SetAPIDestroyed() { api_destroyed_.Set(); }
+    bool IsAPIDestroyed() { return api_destroyed_.IsSet(); }
+
     // Associates |proxy| with |id|. |proxy| must already be registered within
     // this ProxySet.
     //
@@ -165,6 +170,14 @@ class WebRequestAPI
     std::map<Proxy*, std::set<content::GlobalRequestID>>
         proxy_to_request_id_map_;
 
+    // Tracks whether the WebRequestAPI has been destroyed. Since WebRequestAPI
+    // is destroyed on the UI thread, and ProxySet is destroyed on the IO
+    // thread, there may be race conditions where a Proxy mojo pipe receives a
+    // connection error after WebRequestAPI is destroyed but before the ProxySet
+    // has been destroyed. Before running any error handlers, make sure to check
+    // this flag.
+    base::AtomicFlag api_destroyed_;
+
     DISALLOW_COPY_AND_ASSIGN(ProxySet);
   };
 

extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc

@@ -522,6 +522,9 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest::
 }
 void WebRequestProxyingURLLoaderFactory::InProgressRequest::OnRequestError(
     const network::URLLoaderCompletionStatus& status) {
+  if (factory_->proxies_->IsAPIDestroyed())
+    return;
+
   if (!request_completed_) {
     target_client_->OnComplete(status);
     ExtensionWebRequestEventRouter::GetInstance()->OnErrorOccurred(

extensions/browser/api/web_request/web_request_proxying_websocket.cc

@@ -400,6 +400,9 @@ void WebRequestProxyingWebSocket::ResumeIncomingMethodCallProcessing() {
 }
 
 void WebRequestProxyingWebSocket::OnError(int error_code) {
+  if (proxies_->IsAPIDestroyed())
+    return;
+
   if (!is_done_) {
     is_done_ = true;
     ExtensionWebRequestEventRouter::GetInstance()->OnErrorOccurred(