Fix web request proxy crash by moving ownership to ResourceContext

The web request proxies will now be owned completely on the IO thread by
ResourceContext. This should prevent any handlers from being run after
the ResourceContext is destroyed.

Bug: 878366
Change-Id: I629d81597ee3ab3835a63ebe47cb97fa4497b36a
Reviewed-on: https://chromium-review.googlesource.com/1205470
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#588692}(cherry picked from commit 86bc694)
Reviewed-on: https://chromium-review.googlesource.com/1207354
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#50}
Cr-Branched-From: 79f7c91-refs/heads/master@{#587811}

Author: clarkduvall

extensions/browser/api/web_request/web_request_api.cc

@@ -30,6 +30,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
+#include "content/public/browser/resource_context.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/browser/web_contents.h"
@@ -141,6 +142,10 @@ const char* const kWebRequestEvents[] = {
     keys::kOnHeadersReceivedEvent,
 };
 
+// User data key for WebRequestAPI::ProxySet.
+const void* const kWebRequestProxySetUserDataKey =
+    &kWebRequestProxySetUserDataKey;
+
 const char* GetRequestStageAsString(
     ExtensionWebRequestEventRouter::EventTypes type) {
   switch (type) {
@@ -387,6 +392,19 @@ std::unique_ptr<WebRequestEventDetails> CreateEventDetails(
   return std::make_unique<WebRequestEventDetails>(request, extra_info_spec);
 }
 
+void MaybeProxyAuthRequestOnIO(
+    content::ResourceContext* resource_context,
+    net::AuthChallengeInfo* auth_info,
+    scoped_refptr<net::HttpResponseHeaders> response_headers,
+    const content::GlobalRequestID& request_id,
+    WebRequestAPI::AuthRequestCallback callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  auto* proxies =
+      WebRequestAPI::ProxySet::GetFromResourceContext(resource_context);
+  proxies->MaybeProxyAuthRequest(auth_info, std::move(response_headers),
+                                 request_id, std::move(callback));
+}
+
 }  // namespace
 
 void WebRequestAPI::Proxy::HandleAuthRequest(
@@ -401,16 +419,25 @@ void WebRequestAPI::Proxy::HandleAuthRequest(
 }
 
 WebRequestAPI::ProxySet::ProxySet() {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 }
 
 WebRequestAPI::ProxySet::~ProxySet() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 }
 
+WebRequestAPI::ProxySet* WebRequestAPI::ProxySet::GetFromResourceContext(
+    content::ResourceContext* resource_context) {
+  if (!resource_context->GetUserData(kWebRequestProxySetUserDataKey)) {
+    resource_context->SetUserData(kWebRequestProxySetUserDataKey,
+                                  std::make_unique<WebRequestAPI::ProxySet>());
+  }
+  return static_cast<WebRequestAPI::ProxySet*>(
+      resource_context->GetUserData(kWebRequestProxySetUserDataKey));
+}
+
 void WebRequestAPI::ProxySet::AddProxy(std::unique_ptr<Proxy> proxy) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  DCHECK(!is_shutdown_);
 
   proxies_.insert(std::move(proxy));
 }
@@ -430,13 +457,6 @@ void WebRequestAPI::ProxySet::RemoveProxy(Proxy* proxy) {
   proxies_.erase(proxy_it);
 }
 
-void WebRequestAPI::ProxySet::Shutdown() {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-  is_shutdown_ = true;
-
-  proxies_.clear();
-}
-
 void WebRequestAPI::ProxySet::AssociateProxyWithRequestId(
     Proxy* proxy,
     const content::GlobalRequestID& id) {
@@ -490,7 +510,6 @@ void WebRequestAPI::ProxySet::MaybeProxyAuthRequest(
 WebRequestAPI::WebRequestAPI(content::BrowserContext* context)
     : browser_context_(context),
       info_map_(ExtensionSystem::Get(browser_context_)->info_map()),
-      proxies_(base::MakeRefCounted<ProxySet>()),
       request_id_generator_(base::MakeRefCounted<RequestIDGenerator>()),
       may_have_proxies_(MayHaveProxies()),
       rules_monitor_observer_(this),
@@ -516,10 +535,6 @@ WebRequestAPI::WebRequestAPI(content::BrowserContext* context)
 }
 
 WebRequestAPI::~WebRequestAPI() {
-  proxies_->SetAPIDestroyed();
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::BindOnce(&ProxySet::Shutdown, std::move(proxies_)));
 }
 
 void WebRequestAPI::Shutdown() {
@@ -614,7 +629,7 @@ bool WebRequestAPI::MaybeProxyURLLoaderFactory(
           is_for_browser_initiated_requests ? -1 : frame->GetProcess()->GetID(),
           request_id_generator_, std::move(navigation_ui_data),
           base::Unretained(info_map_), std::move(proxied_request),
-          std::move(target_factory_info), proxies_));
+          std::move(target_factory_info)));
   return true;
 }
 
@@ -632,7 +647,8 @@ bool WebRequestAPI::MaybeProxyAuthRequest(
     proxied_request_id.child_id = -1;
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
-      base::BindOnce(&ProxySet::MaybeProxyAuthRequest, proxies_,
+      base::BindOnce(&MaybeProxyAuthRequestOnIO,
+                     browser_context_->GetResourceContext(),
                      base::RetainedRef(auth_info), std::move(response_headers),
                      proxied_request_id, std::move(callback)));
   return true;
@@ -660,8 +676,7 @@ void WebRequestAPI::MaybeProxyWebSocket(
           frame->GetProcess()->GetBrowserContext(),
           frame->GetProcess()->GetBrowserContext()->GetResourceContext(),
           base::Unretained(info_map_), std::move(proxied_socket_ptr_info),
-          std::move(proxied_request), std::move(authentication_request),
-          proxies_));
+          std::move(proxied_request), std::move(authentication_request)));
 }
 
 bool WebRequestAPI::MayHaveProxies() const {

extensions/browser/api/web_request/web_request_api.h

@@ -105,31 +105,21 @@ class WebRequestAPI
   };
 
   // A ProxySet is a set of proxies used by WebRequestAPI: It holds Proxy
-  // instances, and removes all proxies when the WebRequestAPI instance is
-  // gone, on the IO thread.
-  // This proxy set is created on the UI thread but anything else other than
-  // AddRef() and Release() including destruction will be done in the IO thread.
-  class ProxySet : public base::RefCountedThreadSafe<
-                       ProxySet,
-                       content::BrowserThread::DeleteOnIOThread> {
+  // instances, and removes all proxies when the ResourceContext it is bound to
+  // is destroyed.
+  class ProxySet : public base::SupportsUserData::Data {
    public:
     ProxySet();
+    ~ProxySet() override;
 
-    // Add a Proxy. This can be called only when |is_shutdown()| is false.
+    // Gets or creates a ProxySet from the given ResourceContext.
+    static ProxySet* GetFromResourceContext(
+        content::ResourceContext* resource_context);
+
+    // Add a Proxy.
     void AddProxy(std::unique_ptr<Proxy> proxy);
     // Remove a Proxy. The removed proxy is deleted upon this call.
     void RemoveProxy(Proxy* proxy);
-    // Set is_shutdown_ and deletes app proxies.
-    void Shutdown();
-    bool is_shutdown() const {
-      DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-      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.
@@ -153,31 +143,16 @@ class WebRequestAPI
         AuthRequestCallback callback);
 
    private:
-    friend struct content::BrowserThread::DeleteOnThread<
-        content::BrowserThread::IO>;
-    friend class base::DeleteHelper<ProxySet>;
-
-    ~ProxySet();
-
     // Although these members are initialized on the UI thread, we expect at
     // least one memory barrier before actually calling Generate in the IO
     // thread, so we don't protect them with a lock.
     std::set<std::unique_ptr<Proxy>, base::UniquePtrComparator> proxies_;
-    bool is_shutdown_ = false;
 
     // Bi-directional mapping between request ID and Proxy for faster lookup.
     std::map<content::GlobalRequestID, Proxy*> request_id_to_proxy_map_;
     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);
   };
 
@@ -276,9 +251,6 @@ class WebRequestAPI
   content::BrowserContext* const browser_context_;
   InfoMap* const info_map_;
 
-  // Active proxies. Only used when the Network Service is enabled.
-  scoped_refptr<ProxySet> proxies_;
-
   scoped_refptr<RequestIDGenerator> request_id_generator_;
 
   // Stores the last result of |MayHaveProxies()|, so it can be used in

extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc

@@ -522,9 +522,6 @@ 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(
@@ -587,16 +584,15 @@ void WebRequestProxyingURLLoaderFactory::StartProxying(
     std::unique_ptr<ExtensionNavigationUIData> navigation_ui_data,
     InfoMap* info_map,
     network::mojom::URLLoaderFactoryRequest loader_request,
-    network::mojom::URLLoaderFactoryPtrInfo target_factory_info,
-    scoped_refptr<WebRequestAPI::ProxySet> proxies) {
+    network::mojom::URLLoaderFactoryPtrInfo target_factory_info) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-  if (proxies->is_shutdown())
-    return;
+  auto* proxies =
+      WebRequestAPI::ProxySet::GetFromResourceContext(resource_context);
 
   auto proxy = std::make_unique<WebRequestProxyingURLLoaderFactory>(
       browser_context, resource_context, render_process_id,
       std::move(request_id_generator), std::move(navigation_ui_data), info_map,
-      std::move(loader_request), std::move(target_factory_info), proxies.get());
+      std::move(loader_request), std::move(target_factory_info), proxies);
 
   proxies->AddProxy(std::move(proxy));
 }

extensions/browser/api/web_request/web_request_proxying_url_loader_factory.h

@@ -169,8 +169,7 @@ class WebRequestProxyingURLLoaderFactory
       std::unique_ptr<ExtensionNavigationUIData> navigation_ui_data,
       InfoMap* info_map,
       network::mojom::URLLoaderFactoryRequest loader_request,
-      network::mojom::URLLoaderFactoryPtrInfo target_factory_info,
-      scoped_refptr<WebRequestAPI::ProxySet> proxies);
+      network::mojom::URLLoaderFactoryPtrInfo target_factory_info);
 
   // network::mojom::URLLoaderFactory:
   void CreateLoaderAndStart(network::mojom::URLLoaderRequest loader_request,

extensions/browser/api/web_request/web_request_proxying_websocket.cc

@@ -265,17 +265,16 @@ void WebRequestProxyingWebSocket::StartProxying(
     InfoMap* info_map,
     network::mojom::WebSocketPtrInfo proxied_socket_ptr_info,
     network::mojom::WebSocketRequest proxied_request,
-    network::mojom::AuthenticationHandlerRequest auth_request,
-    scoped_refptr<WebRequestAPI::ProxySet> proxies) {
+    network::mojom::AuthenticationHandlerRequest auth_request) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-  if (proxies->is_shutdown())
-    return;
+  auto* proxies =
+      WebRequestAPI::ProxySet::GetFromResourceContext(resource_context);
 
   auto proxy = std::make_unique<WebRequestProxyingWebSocket>(
       process_id, render_frame_id, origin, browser_context, resource_context,
       info_map, std::move(request_id_generator),
       network::mojom::WebSocketPtr(std::move(proxied_socket_ptr_info)),
-      std::move(proxied_request), std::move(auth_request), proxies.get());
+      std::move(proxied_request), std::move(auth_request), proxies);
 
   proxies->AddProxy(std::move(proxy));
 }
@@ -400,9 +399,6 @@ void WebRequestProxyingWebSocket::ResumeIncomingMethodCallProcessing() {
 }
 
 void WebRequestProxyingWebSocket::OnError(int error_code) {
-  if (proxies_->IsAPIDestroyed())
-    return;
-
   if (!is_done_) {
     is_done_ = true;
     ExtensionWebRequestEventRouter::GetInstance()->OnErrorOccurred(

extensions/browser/api/web_request/web_request_proxying_websocket.h

@@ -94,8 +94,7 @@ class WebRequestProxyingWebSocket
       InfoMap* info_map,
       network::mojom::WebSocketPtrInfo proxied_socket_ptr_info,
       network::mojom::WebSocketRequest proxied_request,
-      network::mojom::AuthenticationHandlerRequest auth_request,
-      scoped_refptr<WebRequestAPI::ProxySet> proxies);
+      network::mojom::AuthenticationHandlerRequest auth_request);
 
  private:
   void OnBeforeRequestComplete(int error_code);