[reland] Make ExternalFileURLLoaderFactory owned by its |receivers_|.

This CL introduces ExternalFileURLLoaderFactory::Create static method
that allows creating a ExternalFileURLLoaderFactory 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<ExternalFileURLLoaderFactory>, 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 ExternalFileURLLoaderFactory 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: Iadb47d1f73c73f08a8d0cd2a9c303ddef7158bad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2443398
Commit-Queue: Sam McNally <sammc@chromium.org>
Auto-Submit: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#813474}

chrome/browser/chrome_content_browser_client.cc

@@ -4464,10 +4464,9 @@ void ChromeContentBrowserClient::RegisterNonNetworkNavigationURLLoaderFactories(
 #if defined(OS_CHROMEOS)
   Profile* profile =
       Profile::FromBrowserContext(web_contents->GetBrowserContext());
-  uniquely_owned_factories->emplace(
-      content::kExternalFileScheme,
-      std::make_unique<chromeos::ExternalFileURLLoaderFactory>(
-          profile, content::ChildProcessHost::kInvalidUniqueID));
+  factories->emplace(content::kExternalFileScheme,
+                     chromeos::ExternalFileURLLoaderFactory::Create(
+                         profile, content::ChildProcessHost::kInvalidUniqueID));
 #endif  // defined(OS_CHROMEOS)
 #endif  // BUILDFLAG(ENABLE_EXTENSIONS) || defined(OS_CHROMEOS)
 }
@@ -4621,10 +4620,9 @@ void ChromeContentBrowserClient::
   if (web_contents) {
     Profile* profile =
         Profile::FromBrowserContext(web_contents->GetBrowserContext());
-    uniquely_owned_factories->emplace(
-        content::kExternalFileScheme,
-        std::make_unique<chromeos::ExternalFileURLLoaderFactory>(
-            profile, render_process_id));
+    factories->emplace(content::kExternalFileScheme,
+                       chromeos::ExternalFileURLLoaderFactory::Create(
+                           profile, render_process_id));
   }
 #endif  // defined(OS_CHROMEOS)
 

chrome/browser/chromeos/BUILD.gn

@@ -3816,6 +3816,7 @@ source_set("unit_tests") {
     "//google_apis/drive:test_support",
     "//mojo/public/cpp/bindings",
     "//mojo/public/cpp/system:system",
+    "//mojo/public/cpp/test_support:test_utils",
     "//services/data_decoder/public/cpp:test_support",
     "//services/device/public/cpp:test_support",
     "//services/service_manager/public/cpp/test:test_support",

chrome/browser/chromeos/fileapi/external_file_url_loader_factory.cc

@@ -341,8 +341,10 @@ class ExternalFileURLLoader : public network::mojom::URLLoader {
 
 ExternalFileURLLoaderFactory::ExternalFileURLLoaderFactory(
     void* profile_id,
-    int render_process_host_id)
-    : profile_id_(profile_id),
+    int render_process_host_id,
+    mojo::PendingReceiver<network::mojom::URLLoaderFactory> factory_receiver)
+    : content::NonNetworkURLLoaderFactoryBase(std::move(factory_receiver)),
+      profile_id_(profile_id),
       render_process_host_id_(render_process_host_id) {}
 
 ExternalFileURLLoaderFactory::~ExternalFileURLLoaderFactory() = default;
@@ -369,9 +371,19 @@ void ExternalFileURLLoaderFactory::CreateLoaderAndStart(
                      request, std::move(loader), std::move(client)));
 }
 
-void ExternalFileURLLoaderFactory::Clone(
-    mojo::PendingReceiver<network::mojom::URLLoaderFactory> loader) {
-  receivers_.Add(this, std::move(loader));
+// static
+mojo::PendingRemote<network::mojom::URLLoaderFactory>
+ExternalFileURLLoaderFactory::Create(void* profile_id,
+                                     int render_process_host_id) {
+  mojo::PendingRemote<network::mojom::URLLoaderFactory> pending_remote;
+
+  // The ExternalFileURLLoaderFactory will delete itself when there are no more
+  // receivers - see the NonNetworkURLLoaderFactoryBase::OnDisconnect method.
+  new ExternalFileURLLoaderFactory(
+      profile_id, render_process_host_id,
+      pending_remote.InitWithNewPipeAndPassReceiver());
+
+  return pending_remote;
 }
 
 }  // namespace chromeos

chrome/browser/chromeos/fileapi/external_file_url_loader_factory.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_CHROMEOS_FILEAPI_EXTERNAL_FILE_URL_LOADER_FACTORY_H_
 
 #include "base/macros.h"
+#include "content/public/browser/non_network_url_loader_factory_base.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"
@@ -15,15 +16,30 @@ namespace chromeos {
 
 // URLLoaderFactory that creates URLLoader instances for URLs with the
 // externalfile scheme.
-class ExternalFileURLLoaderFactory : public network::mojom::URLLoaderFactory {
+class ExternalFileURLLoaderFactory
+    : public content::NonNetworkURLLoaderFactoryBase {
  public:
+  // Returns mojo::PendingRemote to a newly constructed
+  // ExternalFileURLLoaderFactory.  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).
+  //
   // |render_process_host_id| is used to verify that the child process has
   // permission to request the URL. It should be
   // ChildProcessHost::kInvalidUniqueID for navigations.
-  ExternalFileURLLoaderFactory(void* profile_id, int render_process_host_id);
-  ~ExternalFileURLLoaderFactory() override;
+  static mojo::PendingRemote<network::mojom::URLLoaderFactory> Create(
+      void* profile_id,
+      int render_process_host_id);
 
  private:
+  ExternalFileURLLoaderFactory(
+      void* profile_id,
+      int render_process_host_id,
+      mojo::PendingReceiver<network::mojom::URLLoaderFactory> factory_receiver);
+
+  ~ExternalFileURLLoaderFactory() override;
+
   friend class ExternalFileURLLoaderFactoryTest;
 
   // network::mojom::URLLoaderFactory:
@@ -36,10 +52,7 @@ class ExternalFileURLLoaderFactory : public network::mojom::URLLoaderFactory {
       mojo::PendingRemote<network::mojom::URLLoaderClient> client,
       const net::MutableNetworkTrafficAnnotationTag& traffic_annotation)
       override;
-  void Clone(
-      mojo::PendingReceiver<network::mojom::URLLoaderFactory> loader) override;
 
-  mojo::ReceiverSet<network::mojom::URLLoaderFactory> receivers_;
   void* profile_id_;
   const int render_process_host_id_;
 

chrome/browser/chromeos/fileapi/external_file_url_loader_factory_unittest.cc

@@ -21,6 +21,7 @@
 #include "content/public/test/browser_task_environment.h"
 #include "content/public/test/mock_render_process_host.h"
 #include "mojo/public/cpp/system/data_pipe_utils.h"
+#include "mojo/public/cpp/test_support/test_utils.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
 #include "net/url_request/redirect_info.h"
 #include "services/network/test/test_url_loader_client.h"
@@ -72,8 +73,8 @@ class ExternalFileURLLoaderFactoryTest : public testing::Test {
                                  kFileSystemId, "Test FileSystem"));
 
     // Create the URLLoaderFactory.
-    url_loader_factory_ = std::make_unique<ExternalFileURLLoaderFactory>(
-        profile, render_process_host_id());
+    url_loader_factory_.Bind(ExternalFileURLLoaderFactory::Create(
+        profile, render_process_host_id()));
   }
 
   virtual int render_process_host_id() {
@@ -106,7 +107,7 @@ class ExternalFileURLLoaderFactoryTest : public testing::Test {
  private:
   content::BrowserTaskEnvironment task_environment_;
 
-  std::unique_ptr<ExternalFileURLLoaderFactory> url_loader_factory_;
+  mojo::Remote<network::mojom::URLLoaderFactory> url_loader_factory_;
 
   std::unique_ptr<TestingProfileManager> profile_manager_;
   std::unique_ptr<chromeos::FakeChromeUserManager> user_manager_;
@@ -229,8 +230,11 @@ TEST_F(SubresourceExternalFileURLLoaderFactoryTest, SubresourceAllowed) {
 }
 
 TEST_F(SubresourceExternalFileURLLoaderFactoryTest, SubresourceNotAllowed) {
+  mojo::test::BadMessageObserver bad_message_observer;
   network::TestURLLoaderClient client;
-  ASSERT_DEATH(CreateURLLoaderAndStart(&client, CreateRequest(kTestUrl)), "");
+  CreateURLLoaderAndStart(&client, CreateRequest(kTestUrl));
+  EXPECT_EQ("Unauthorized externalfile request",
+            bad_message_observer.WaitForBadMessage());
 }
 
 }  // namespace chromeos