Make ExtensionURLLoaderFactory always owned by its |receivers_|.

This CL introduces ExtensionURLLoaderFactory::Create static method that allow creating an ExtensionURLLoaderFactory that is owned by its |receivers_| and will self-delete when the last receiver disconnects. This CL removes the ability to directly construct and own a std::unique_ptr<ExtensionURLLoaderFactory>, because this ability means that the factory can be destructed while receivers bound via the Clone method are still alive (see the associated bug). In particular, this CL stops exposing ExtensionURLLoaderFactory constructor as a public member, which forces construction to go via the new Create static method. This CL mostly just follows the pattern established earlier by https://crrev.com/c/2337411. Bug: 1106995 Change-Id: I16565e8200a6d7190d6ce06ea5c0250cef696f10 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2357523 Reviewed-by: Nasko Oskov <nasko@chromium.org> Reviewed-by: Sean Topping <seantopping@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Auto-Submit: Łukasz Anforowicz <lukasza@chromium.org> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Cr-Commit-Position: refs/heads/master@{#810911}
Denger-Network · Sep 26, 2020 · a782c5a · a782c5a
1 parent 3d27288
commit a782c5a
Show file tree

Hide file tree

Showing 13 changed files with 111 additions and 104 deletions.
diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
@@ -4470,7 +4470,7 @@ void ChromeContentBrowserClient::RegisterNonNetworkNavigationURLLoaderFactories(
   content::WebContents* web_contents =
       content::WebContents::FromFrameTreeNodeId(frame_tree_node_id);
 #if BUILDFLAG(ENABLE_EXTENSIONS)
-  uniquely_owned_factories->emplace(
+  factories->emplace(
       extensions::kExtensionScheme,
       extensions::CreateExtensionNavigationURLLoaderFactory(
           web_contents->GetBrowserContext(), ukm_source_id,
@@ -4490,10 +4490,11 @@ void ChromeContentBrowserClient::RegisterNonNetworkNavigationURLLoaderFactories(
 void ChromeContentBrowserClient::
     RegisterNonNetworkWorkerMainResourceURLLoaderFactories(
         content::BrowserContext* browser_context,
-        NonNetworkURLLoaderFactoryDeprecatedMap* factories) {
-#if BUILDFLAG(ENABLE_EXTENSIONS)
+        NonNetworkURLLoaderFactoryMap* factories) {
   DCHECK(browser_context);
   DCHECK(factories);
+
+#if BUILDFLAG(ENABLE_EXTENSIONS)
   factories->emplace(
       extensions::kExtensionScheme,
       extensions::CreateExtensionWorkerMainResourceURLLoaderFactory(
@@ -4504,10 +4505,11 @@ void ChromeContentBrowserClient::
 void ChromeContentBrowserClient::
     RegisterNonNetworkServiceWorkerUpdateURLLoaderFactories(
         content::BrowserContext* browser_context,
-        NonNetworkURLLoaderFactoryDeprecatedMap* factories) {
-#if BUILDFLAG(ENABLE_EXTENSIONS)
+        NonNetworkURLLoaderFactoryMap* factories) {
   DCHECK(browser_context);
   DCHECK(factories);
+
+#if BUILDFLAG(ENABLE_EXTENSIONS)
   factories->emplace(
       extensions::kExtensionScheme,
       extensions::CreateExtensionServiceWorkerScriptURLLoaderFactory(
@@ -4624,11 +4626,9 @@ void ChromeContentBrowserClient::
 #endif  // defined(OS_CHROMEOS)
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
-  auto factory = extensions::CreateExtensionURLLoaderFactory(render_process_id,
-                                                             render_frame_id);
-  if (factory)
-    uniquely_owned_factories->emplace(extensions::kExtensionScheme,
-                                      std::move(factory));
+  factories->emplace(extensions::kExtensionScheme,
+                     extensions::CreateExtensionURLLoaderFactory(
+                         render_process_id, render_frame_id));
 
   // This logic should match
   // ChromeExtensionWebContentsObserver::RenderFrameCreated.

diff --git a/chrome/browser/chrome_content_browser_client.h b/chrome/browser/chrome_content_browser_client.h
@@ -465,10 +465,10 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
       NonNetworkURLLoaderFactoryMap* factories) override;
   void RegisterNonNetworkWorkerMainResourceURLLoaderFactories(
       content::BrowserContext* browser_context,
-      NonNetworkURLLoaderFactoryDeprecatedMap* factories) override;
+      NonNetworkURLLoaderFactoryMap* factories) override;
   void RegisterNonNetworkServiceWorkerUpdateURLLoaderFactories(
       content::BrowserContext* browser_context,
-      NonNetworkURLLoaderFactoryDeprecatedMap* factories) override;
+      NonNetworkURLLoaderFactoryMap* factories) override;
   void RegisterNonNetworkSubresourceURLLoaderFactories(
       int render_process_id,
       int render_frame_id,

diff --git a/chrome/browser/extensions/extension_protocols_unittest.cc b/chrome/browser/extensions/extension_protocols_unittest.cc
@@ -236,8 +236,8 @@ class ExtensionProtocolsTestBase : public testing::Test {
   }
 
   void SetProtocolHandler(bool is_incognito) {
-    loader_factory_ = extensions::CreateExtensionNavigationURLLoaderFactory(
-        browser_context(), test_ukm_id_, false);
+    loader_factory_.Bind(extensions::CreateExtensionNavigationURLLoaderFactory(
+        browser_context(), test_ukm_id_, false));
   }
 
   GetResult RequestOrLoad(const GURL& url, ResourceType resource_type) {
@@ -373,7 +373,7 @@ class ExtensionProtocolsTestBase : public testing::Test {
 
   content::BrowserTaskEnvironment task_environment_;
   std::unique_ptr<content::RenderViewHostTestEnabler> rvh_test_enabler_;
-  std::unique_ptr<network::mojom::URLLoaderFactory> loader_factory_;
+  mojo::Remote<network::mojom::URLLoaderFactory> loader_factory_;
   std::unique_ptr<TestingProfile> testing_profile_;
   std::unique_ptr<content::WebContents> contents_;
   const bool force_incognito_;

diff --git a/chromecast/browser/cast_extension_url_loader_factory.cc b/chromecast/browser/cast_extension_url_loader_factory.cc
@@ -172,7 +172,7 @@ class CastExtensionURLLoader : public network::mojom::URLLoader,
 
 CastExtensionURLLoaderFactory::CastExtensionURLLoaderFactory(
     content::BrowserContext* browser_context,
-    std::unique_ptr<network::mojom::URLLoaderFactory> extension_factory)
+    mojo::PendingRemote<network::mojom::URLLoaderFactory> extension_factory)
     : extension_registry_(extensions::ExtensionRegistry::Get(browser_context)),
       extension_factory_(std::move(extension_factory)),
       network_factory_(

diff --git a/chromecast/browser/cast_extension_url_loader_factory.h b/chromecast/browser/cast_extension_url_loader_factory.h
@@ -5,12 +5,11 @@
 #ifndef CHROMECAST_BROWSER_CAST_EXTENSION_URL_LOADER_FACTORY_H_
 #define CHROMECAST_BROWSER_CAST_EXTENSION_URL_LOADER_FACTORY_H_
 
-#include <memory>
-
 #include "base/macros.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/receiver_set.h"
+#include "mojo/public/cpp/bindings/remote.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/network/public/mojom/url_loader_factory.mojom.h"
 
@@ -35,7 +34,7 @@ class CastExtensionURLLoaderFactory : public network::mojom::URLLoaderFactory {
   // the request isn't fetched from the web.
   CastExtensionURLLoaderFactory(
       content::BrowserContext* browser_context,
-      std::unique_ptr<network::mojom::URLLoaderFactory> extension_factory);
+      mojo::PendingRemote<network::mojom::URLLoaderFactory> extension_factory);
   ~CastExtensionURLLoaderFactory() override;
 
  private:
@@ -54,7 +53,7 @@ class CastExtensionURLLoaderFactory : public network::mojom::URLLoaderFactory {
 
   mojo::ReceiverSet<network::mojom::URLLoaderFactory> receivers_;
   extensions::ExtensionRegistry* extension_registry_;
-  std::unique_ptr<network::mojom::URLLoaderFactory> extension_factory_;
+  mojo::Remote<network::mojom::URLLoaderFactory> extension_factory_;
   scoped_refptr<network::SharedURLLoaderFactory> network_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CastExtensionURLLoaderFactory);

diff --git a/content/browser/service_worker/service_worker_context_wrapper.cc b/content/browser/service_worker/service_worker_context_wrapper.cc
@@ -1884,8 +1884,7 @@ std::unique_ptr<blink::PendingURLLoaderFactoryBundle>
 ServiceWorkerContextWrapper::
     CreateNonNetworkPendingURLLoaderFactoryBundleForUpdateCheck(
         BrowserContext* browser_context) {
-  ContentBrowserClient::NonNetworkURLLoaderFactoryDeprecatedMap
-      non_network_factories;
+  ContentBrowserClient::NonNetworkURLLoaderFactoryMap non_network_factories;
   GetContentClient()
       ->browser()
       ->RegisterNonNetworkServiceWorkerUpdateURLLoaderFactories(
@@ -1895,12 +1894,9 @@ ServiceWorkerContextWrapper::
       std::make_unique<blink::PendingURLLoaderFactoryBundle>();
   for (auto& pair : non_network_factories) {
     const std::string& scheme = pair.first;
-    std::unique_ptr<network::mojom::URLLoaderFactory> factory =
-        std::move(pair.second);
+    mojo::PendingRemote<network::mojom::URLLoaderFactory>& factory_remote =
+        pair.second;
 
-    mojo::PendingRemote<network::mojom::URLLoaderFactory> factory_remote;
-    mojo::MakeSelfOwnedReceiver(
-        std::move(factory), factory_remote.InitWithNewPipeAndPassReceiver());
     factory_bundle->pending_scheme_specific_factories().emplace(
         scheme, std::move(factory_remote));
   }

diff --git a/content/browser/worker_host/worker_script_fetch_initiator.cc b/content/browser/worker_host/worker_script_fetch_initiator.cc
@@ -225,8 +225,7 @@ WorkerScriptFetchInitiator::CreateFactoryBundle(
       GetContentClient()
           ->browser()
           ->RegisterNonNetworkWorkerMainResourceURLLoaderFactories(
-              storage_partition->browser_context(),
-              &non_network_uniquely_owned_factories);
+              storage_partition->browser_context(), &non_network_factories);
       break;
     case LoaderType::kSubResource:
       GetContentClient()

diff --git a/content/public/browser/content_browser_client.cc b/content/public/browser/content_browser_client.cc
@@ -760,12 +760,12 @@ void ContentBrowserClient::RegisterNonNetworkNavigationURLLoaderFactories(
 void ContentBrowserClient::
     RegisterNonNetworkWorkerMainResourceURLLoaderFactories(
         BrowserContext* browser_context,
-        NonNetworkURLLoaderFactoryDeprecatedMap* uniquely_owned_factories) {}
+        NonNetworkURLLoaderFactoryMap* factories) {}
 
 void ContentBrowserClient::
     RegisterNonNetworkServiceWorkerUpdateURLLoaderFactories(
         BrowserContext* browser_context,
-        NonNetworkURLLoaderFactoryDeprecatedMap* uniquely_owned_factories) {}
+        NonNetworkURLLoaderFactoryMap* factories) {}
 
 void ContentBrowserClient::RegisterNonNetworkSubresourceURLLoaderFactories(
     int render_process_id,

diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h
@@ -1265,13 +1265,9 @@ class CONTENT_EXPORT ContentBrowserClient {
   // initiated by the browser process for schemes not handled by the Network
   // Service. The resulting |factories| must be used only by the browser
   // process. The caller must not send any of |factories| to any other process.
-  //
-  // TODO(lukasza): https://crbug.com/1106995: Deprecate and remove the
-  // |uniquely_owned_factories| parameter - it results in incorrect factory
-  // lifetimes.
   virtual void RegisterNonNetworkWorkerMainResourceURLLoaderFactories(
       BrowserContext* browser_context,
-      NonNetworkURLLoaderFactoryDeprecatedMap* uniquely_owned_factories);
+      NonNetworkURLLoaderFactoryMap* factories);
 
   // Allows the embedder to register per-scheme URLLoaderFactory
   // implementations to handle service worker main/imported script requests
@@ -1280,13 +1276,9 @@ class CONTENT_EXPORT ContentBrowserClient {
   // ServiceWorkerImportedScriptUpdateCheck is enabled.
   // The resulting |factories| must be used only by the browser process. The
   // caller must not send any of |factories| to any other process.
-  //
-  // TODO(lukasza): https://crbug.com/1106995: Deprecate and remove the
-  // |uniquely_owned_factories| parameter - it results in incorrect factory
-  // lifetimes.
   virtual void RegisterNonNetworkServiceWorkerUpdateURLLoaderFactories(
       BrowserContext* browser_context,
-      NonNetworkURLLoaderFactoryDeprecatedMap* uniquely_owned_factories);
+      NonNetworkURLLoaderFactoryMap* factories);
 
   // Allows the embedder to register per-scheme URLLoaderFactory implementations
   // to handle subresource URL requests for schemes not handled by the Network

diff --git a/extensions/browser/extension_protocols.cc b/extensions/browser/extension_protocols.cc
@@ -43,8 +43,10 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/file_url_loader.h"
 #include "content/public/browser/navigation_ui_data.h"
+#include "content/public/browser/non_network_url_loader_factory_base.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
+#include "content/public/common/child_process_host.h"
 #include "crypto/secure_hash.h"
 #include "crypto/sha2.h"
 #include "extensions/browser/content_verifier.h"
@@ -292,7 +294,7 @@ bool AllowExtensionResourceLoad(const GURL& url,
   // Service Worker and the imported scripts can be loaded with extension URLs
   // in browser process during update check when
   // ServiceWorkerImportedScriptUpdateCheck is enabled.
-  if (child_id == -1 &&
+  if (child_id == content::ChildProcessHost::kInvalidUniqueID &&
       (blink::IsResourceTypeFrame(resource_type) ||
        (base::FeatureList::IsEnabled(blink::features::kPlzDedicatedWorker) &&
         resource_type == blink::mojom::ResourceType::kWorker) ||
@@ -449,39 +451,45 @@ class FileLoaderObserver : public content::FileURLLoaderObserver {
   DISALLOW_COPY_AND_ASSIGN(FileLoaderObserver);
 };
 
-class ExtensionURLLoaderFactory : public network::mojom::URLLoaderFactory {
+class ExtensionURLLoaderFactory
+    : public content::NonNetworkURLLoaderFactoryBase {
  public:
-  ExtensionURLLoaderFactory(int render_process_id, int render_frame_id)
-      : render_process_id_(render_process_id),
-        render_frame_id_(render_frame_id) {
-    content::RenderProcessHost* process_host =
-        content::RenderProcessHost::FromID(render_process_id);
-    browser_context_ = process_host->GetBrowserContext();
-    is_web_view_request_ = WebViewGuest::FromFrameID(
-                               render_process_id_, render_frame_id) != nullptr;
-    content::RenderFrameHost* rfh =
-        content::RenderFrameHost::FromID(render_process_id_, render_frame_id_);
-    if (rfh)
-      ukm_source_id_ = base::UkmSourceId::FromInt64(rfh->GetPageUkmSourceId());
-
-    Init();
-  }
-
-  ExtensionURLLoaderFactory(content::BrowserContext* browser_context,
-                            base::UkmSourceId ukm_source_id,
-                            bool is_web_view_request)
-      : browser_context_(browser_context),
-        is_web_view_request_(is_web_view_request),
-        ukm_source_id_(ukm_source_id),
-        render_process_id_(-1),
-        render_frame_id_(-1) {
-    Init();
+  static mojo::PendingRemote<network::mojom::URLLoaderFactory> Create(
+      content::BrowserContext* browser_context,
+      base::UkmSourceId ukm_source_id,
+      bool is_web_view_request,
+      int render_process_id) {
+    mojo::PendingRemote<network::mojom::URLLoaderFactory> pending_remote;
+
+    // Manages its own lifetime.
+    new ExtensionURLLoaderFactory(
+        browser_context, ukm_source_id, is_web_view_request, render_process_id,
+        pending_remote.InitWithNewPipeAndPassReceiver());
+
+    return pending_remote;
   }
 
-  void Init() {
+ private:
+  // Constructs ExtensionURLLoaderFactory bound to the |factory_receiver|.
+  //
+  // The factory is self-owned - it will delete itself once there are no more
+  // receivers (including the receiver associated with the returned
+  // mojo::PendingRemote and the receivers bound by the Clone method).  See also
+  // the NonNetworkURLLoaderFactoryBase::OnDisconnect method.
+  ExtensionURLLoaderFactory(
+      content::BrowserContext* browser_context,
+      base::UkmSourceId ukm_source_id,
+      bool is_web_view_request,
+      int render_process_id,
+      mojo::PendingReceiver<network::mojom::URLLoaderFactory> factory_receiver)
+      : content::NonNetworkURLLoaderFactoryBase(std::move(factory_receiver)),
+        browser_context_(browser_context),
+        is_web_view_request_(is_web_view_request),
+        ukm_source_id_(ukm_source_id),
+        render_process_id_(render_process_id) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     extension_info_map_ =
         extensions::ExtensionSystem::Get(browser_context_)->info_map();
-    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   }
 
   ~ExtensionURLLoaderFactory() override = default;
@@ -536,12 +544,6 @@ class ExtensionURLLoaderFactory : public network::mojom::URLLoaderFactory {
                   std::move(directory_path));
   }
 
-  void Clone(mojo::PendingReceiver<network::mojom::URLLoaderFactory> receiver)
-      override {
-    receivers_.Add(this, std::move(receiver));
-  }
-
- private:
   void LoadExtension(
       mojo::PendingReceiver<network::mojom::URLLoader> loader,
       const network::ResourceRequest& request,
@@ -729,9 +731,7 @@ class ExtensionURLLoaderFactory : public network::mojom::URLLoaderFactory {
   // avoid holding on to stale pointers if we get requests past the lifetime of
   // the objects.
   const int render_process_id_;
-  const int render_frame_id_;
   scoped_refptr<extensions::InfoMap> extension_info_map_;
-  mojo::ReceiverSet<network::mojom::URLLoaderFactory> receivers_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionURLLoaderFactory);
 };
@@ -782,35 +782,50 @@ void SetExtensionProtocolTestHandler(ExtensionProtocolTestHandler* handler) {
   g_test_handler = handler;
 }
 
-std::unique_ptr<network::mojom::URLLoaderFactory>
+mojo::PendingRemote<network::mojom::URLLoaderFactory>
 CreateExtensionNavigationURLLoaderFactory(
     content::BrowserContext* browser_context,
     base::UkmSourceId ukm_source_id,
     bool is_web_view_request) {
-  return std::make_unique<ExtensionURLLoaderFactory>(
-      browser_context, ukm_source_id, is_web_view_request);
+  return ExtensionURLLoaderFactory::Create(
+      browser_context, ukm_source_id, is_web_view_request,
+      content::ChildProcessHost::kInvalidUniqueID);
 }
 
-std::unique_ptr<network::mojom::URLLoaderFactory>
+mojo::PendingRemote<network::mojom::URLLoaderFactory>
 CreateExtensionWorkerMainResourceURLLoaderFactory(
     content::BrowserContext* browser_context) {
-  return std::make_unique<ExtensionURLLoaderFactory>(
+  return ExtensionURLLoaderFactory::Create(
       browser_context, base::kInvalidUkmSourceId,
-      /*is_web_view_request=*/false);
+      /*is_web_view_request=*/false,
+      content::ChildProcessHost::kInvalidUniqueID);
 }
 
-std::unique_ptr<network::mojom::URLLoaderFactory>
+mojo::PendingRemote<network::mojom::URLLoaderFactory>
 CreateExtensionServiceWorkerScriptURLLoaderFactory(
     content::BrowserContext* browser_context) {
-  return std::make_unique<ExtensionURLLoaderFactory>(
+  return ExtensionURLLoaderFactory::Create(
       browser_context, base::kInvalidUkmSourceId,
-      /*is_web_view_request=*/false);
+      /*is_web_view_request=*/false,
+      content::ChildProcessHost::kInvalidUniqueID);
 }
 
-std::unique_ptr<network::mojom::URLLoaderFactory>
+mojo::PendingRemote<network::mojom::URLLoaderFactory>
 CreateExtensionURLLoaderFactory(int render_process_id, int render_frame_id) {
-  return std::make_unique<ExtensionURLLoaderFactory>(render_process_id,
-                                                     render_frame_id);
+  content::RenderProcessHost* process_host =
+      content::RenderProcessHost::FromID(render_process_id);
+  content::BrowserContext* browser_context = process_host->GetBrowserContext();
+  bool is_web_view_request =
+      WebViewGuest::FromFrameID(render_process_id, render_frame_id) != nullptr;
+
+  content::RenderFrameHost* rfh =
+      content::RenderFrameHost::FromID(render_process_id, render_frame_id);
+  base::UkmSourceId ukm_source_id = base::kInvalidUkmSourceId;
+  if (rfh)
+    ukm_source_id = base::UkmSourceId::FromInt64(rfh->GetPageUkmSourceId());
+
+  return ExtensionURLLoaderFactory::Create(
+      browser_context, ukm_source_id, is_web_view_request, render_process_id);
 }
 
 }  // namespace extensions