Reland "Fix crash in web request proxies connection error handler"

This is a reland of a475e1ab

Test needed to call Shutdown() so WebRequestAPI is cleaned up properly.

Original change's description:
> 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/1196031
> Reviewed-by: Ken Rockot <rockot@chromium.org>
> Commit-Queue: Clark DuVall <cduvall@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#587749}

TBR=rockot@chromium.org

Bug: 878366
Change-Id: I97ab12e12fe4955af94138ec5bb64c1391d27598
Reviewed-on: https://chromium-review.googlesource.com/1198567Reviewed-by: Clark DuVall <cduvall@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587927}

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,55 @@ 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.
+  api->Shutdown();
+  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(