[FileAPI] Move BlobUrlRegistry to the UI thread

Moving BlobUrlRegistry to the UI thread has several benefits, one
of which being that it makes it easier for the BlobUrlRegistry to
be exposed directly to the renderer (rather than being retrieved
via the BlobRegistry). As part of that effort we would like to
use a navigation-associated interface associated with the
corresponding RenderFrameHostImpl, but that appears to encounter
a DCHECK in the Mojo code when the interface implementation lives
on the IO thread.

Change-Id: I3bfd0385407c7ff261978b243ccdcf3dc4812352
Bug: 957249, 1261328
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2343608
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/main@{#984144}

content/browser/blob_storage/blob_registry_wrapper.cc

@@ -44,13 +44,13 @@ class BindingDelegate : public storage::BlobRegistryImpl::Delegate {
 scoped_refptr<BlobRegistryWrapper> BlobRegistryWrapper::Create(
     scoped_refptr<ChromeBlobStorageContext> blob_storage_context,
     scoped_refptr<storage::FileSystemContext> file_system_context,
-    scoped_refptr<BlobRegistryWrapper> registry_for_fallback_url_registry) {
+    base::WeakPtr<storage::BlobUrlRegistry> blob_url_registry) {
   scoped_refptr<BlobRegistryWrapper> result(new BlobRegistryWrapper());
   GetIOThreadTaskRunner({})->PostTask(
       FROM_HERE, base::BindOnce(&BlobRegistryWrapper::InitializeOnIOThread,
                                 result, std::move(blob_storage_context),
                                 std::move(file_system_context),
-                                std::move(registry_for_fallback_url_registry)));
+                                std::move(blob_url_registry)));
   return result;
 }
 
@@ -73,14 +73,11 @@ BlobRegistryWrapper::~BlobRegistryWrapper() {}
 void BlobRegistryWrapper::InitializeOnIOThread(
     scoped_refptr<ChromeBlobStorageContext> blob_storage_context,
     scoped_refptr<storage::FileSystemContext> file_system_context,
-    scoped_refptr<BlobRegistryWrapper> registry_for_fallback_url_registry) {
+    base::WeakPtr<storage::BlobUrlRegistry> blob_url_registry) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  url_registry_ = std::make_unique<storage::BlobUrlRegistry>(
-      registry_for_fallback_url_registry
-          ? registry_for_fallback_url_registry->url_registry()->AsWeakPtr()
-          : nullptr);
   blob_registry_ = std::make_unique<storage::BlobRegistryImpl>(
-      blob_storage_context->context()->AsWeakPtr(), url_registry_->AsWeakPtr(),
+      blob_storage_context->context()->AsWeakPtr(),
+      std::move(blob_url_registry), GetUIThreadTaskRunner({}),
       std::move(file_system_context));
 }
 

content/browser/blob_storage/blob_registry_wrapper.h

@@ -31,16 +31,11 @@ class BlobRegistryWrapper
   static scoped_refptr<BlobRegistryWrapper> Create(
       scoped_refptr<ChromeBlobStorageContext> blob_storage_context,
       scoped_refptr<storage::FileSystemContext> file_system_context,
-      scoped_refptr<BlobRegistryWrapper> registry_for_fallback_url_registry =
-          nullptr);
+      base::WeakPtr<storage::BlobUrlRegistry> blob_url_registry);
 
   void Bind(int process_id,
             mojo::PendingReceiver<blink::mojom::BlobRegistry> receiver);
 
-  // TODO(mek): Make this be owned by StoragePartition directly, and living
-  // on the UI thread.
-  storage::BlobUrlRegistry* url_registry() { return url_registry_.get(); }
-
  private:
   BlobRegistryWrapper();
   friend struct BrowserThread::DeleteOnThread<BrowserThread::IO>;
@@ -50,10 +45,9 @@ class BlobRegistryWrapper
   void InitializeOnIOThread(
       scoped_refptr<ChromeBlobStorageContext> blob_storage_context,
       scoped_refptr<storage::FileSystemContext> file_system_context,
-      scoped_refptr<BlobRegistryWrapper> registry_for_fallback_url_registry);
+      base::WeakPtr<storage::BlobUrlRegistry> blob_url_registry);
 
   std::unique_ptr<storage::BlobRegistryImpl> blob_registry_;
-  std::unique_ptr<storage::BlobUrlRegistry> url_registry_;
 };
 
 }  // namespace content

content/browser/blob_storage/chrome_blob_storage_context.cc

@@ -18,7 +18,6 @@
 #include "base/task/single_thread_task_runner.h"
 #include "base/task/task_runner.h"
 #include "base/task/thread_pool.h"
-#include "content/browser/blob_storage/blob_registry_wrapper.h"
 #include "content/browser/storage_partition_impl.h"
 #include "content/public/browser/blob_handle.h"
 #include "content/public/browser/browser_context.h"
@@ -252,21 +251,14 @@ ChromeBlobStorageContext::URLLoaderFactoryForToken(
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   mojo::PendingRemote<network::mojom::URLLoaderFactory>
       blob_url_loader_factory_remote;
-  GetIOThreadTaskRunner({})->PostTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](scoped_refptr<BlobRegistryWrapper> registry,
-             mojo::PendingReceiver<network::mojom::URLLoaderFactory> receiver,
-             mojo::PendingRemote<blink::mojom::BlobURLToken> token) {
-            storage::BlobURLLoaderFactory::Create(
-                std::move(token), registry->url_registry()->AsWeakPtr(),
-                std::move(receiver));
-          },
-          base::WrapRefCounted(
-              static_cast<StoragePartitionImpl*>(storage_partition)
-                  ->GetBlobRegistry()),
-          blob_url_loader_factory_remote.InitWithNewPipeAndPassReceiver(),
-          std::move(token)));
+
+  storage::BlobURLLoaderFactory::Create(
+      std::move(token),
+      static_cast<StoragePartitionImpl*>(storage_partition)
+          ->GetBlobUrlRegistry()
+          ->AsWeakPtr(),
+      blob_url_loader_factory_remote.InitWithNewPipeAndPassReceiver());
+
   return base::MakeRefCounted<network::WrapperSharedURLLoaderFactory>(
       std::move(blob_url_loader_factory_remote));
 }
@@ -279,21 +271,13 @@ ChromeBlobStorageContext::URLLoaderFactoryForUrl(
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   mojo::PendingRemote<network::mojom::URLLoaderFactory>
       blob_url_loader_factory_remote;
-  GetIOThreadTaskRunner({})->PostTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](scoped_refptr<BlobRegistryWrapper> registry,
-             mojo::PendingReceiver<network::mojom::URLLoaderFactory> receiver,
-             const GURL& url) {
-            auto blob_remote = registry->url_registry()->GetBlobFromUrl(url);
-            storage::BlobURLLoaderFactory::Create(std::move(blob_remote), url,
-                                                  std::move(receiver));
-          },
-          base::WrapRefCounted(
-              static_cast<StoragePartitionImpl*>(storage_partition)
-                  ->GetBlobRegistry()),
-          blob_url_loader_factory_remote.InitWithNewPipeAndPassReceiver(),
-          url));
+
+  storage::BlobURLLoaderFactory::Create(
+      static_cast<StoragePartitionImpl*>(storage_partition)
+          ->GetBlobUrlRegistry()
+          ->GetBlobFromUrl(url),
+      url, blob_url_loader_factory_remote.InitWithNewPipeAndPassReceiver());
+
   return base::MakeRefCounted<network::WrapperSharedURLLoaderFactory>(
       std::move(blob_url_loader_factory_remote));
 }

content/browser/storage_partition_impl.cc

@@ -1290,11 +1290,13 @@ void StoragePartitionImpl::Initialize(
                                 browser_context_->GetSpecialStoragePolicy(),
                                 blob_context.get());
 
-  BlobRegistryWrapper* fallback_blob_registry =
-      fallback_for_blob_urls ? fallback_for_blob_urls->GetBlobRegistry()
-                             : nullptr;
+  blob_url_registry_ = std::make_unique<storage::BlobUrlRegistry>(
+      fallback_for_blob_urls
+          ? fallback_for_blob_urls->GetBlobUrlRegistry()->AsWeakPtr()
+          : nullptr);
+
   blob_registry_ = BlobRegistryWrapper::Create(
-      blob_context, filesystem_context_, fallback_blob_registry);
+      blob_context, filesystem_context_, blob_url_registry_->AsWeakPtr());
 
   prefetch_url_loader_service_ =
       std::make_unique<PrefetchURLLoaderService>(browser_context_);
@@ -1596,6 +1598,11 @@ BlobRegistryWrapper* StoragePartitionImpl::GetBlobRegistry() {
   return blob_registry_.get();
 }
 
+storage::BlobUrlRegistry* StoragePartitionImpl::GetBlobUrlRegistry() {
+  DCHECK(initialized_);
+  return blob_url_registry_.get();
+}
+
 PrefetchURLLoaderService* StoragePartitionImpl::GetPrefetchURLLoaderService() {
   DCHECK(initialized_);
   return prefetch_url_loader_service_.get();

content/browser/storage_partition_impl.h

@@ -42,6 +42,7 @@
 #include "mojo/public/cpp/bindings/unique_receiver_set.h"
 #include "services/network/public/mojom/cookie_manager.mojom.h"
 #include "services/network/public/mojom/network_context.mojom.h"
+#include "storage/browser/blob/blob_url_registry.h"
 #include "storage/browser/quota/quota_client_type.h"
 #include "storage/browser/quota/quota_settings.h"
 #include "third_party/blink/public/common/tokens/tokens.h"
@@ -234,6 +235,7 @@ class CONTENT_EXPORT StoragePartitionImpl
   BroadcastChannelService* GetBroadcastChannelService();
   BluetoothAllowedDevicesMap* GetBluetoothAllowedDevicesMap();
   BlobRegistryWrapper* GetBlobRegistry();
+  storage::BlobUrlRegistry* GetBlobUrlRegistry();
   PrefetchURLLoaderService* GetPrefetchURLLoaderService();
   CookieStoreManager* GetCookieStoreManager();
   FileSystemAccessManagerImpl* GetFileSystemAccessManager();
@@ -620,6 +622,7 @@ class CONTENT_EXPORT StoragePartitionImpl
   std::unique_ptr<BroadcastChannelService> broadcast_channel_service_;
   std::unique_ptr<BluetoothAllowedDevicesMap> bluetooth_allowed_devices_map_;
   scoped_refptr<BlobRegistryWrapper> blob_registry_;
+  std::unique_ptr<storage::BlobUrlRegistry> blob_url_registry_;
   std::unique_ptr<PrefetchURLLoaderService> prefetch_url_loader_service_;
   std::unique_ptr<CookieStoreManager> cookie_store_manager_;
   scoped_refptr<BucketContext> bucket_context_;

storage/browser/blob/blob_registry_impl.cc

@@ -501,10 +501,12 @@ bool BlobRegistryImpl::BlobUnderConstruction::ContainsCycles(
 BlobRegistryImpl::BlobRegistryImpl(
     base::WeakPtr<BlobStorageContext> context,
     base::WeakPtr<BlobUrlRegistry> url_registry,
+    scoped_refptr<base::TaskRunner> url_registry_runner,
     scoped_refptr<FileSystemContext> file_system_context)
     : context_(std::move(context)),
+      file_system_context_(std::move(file_system_context)),
       url_registry_(std::move(url_registry)),
-      file_system_context_(std::move(file_system_context)) {}
+      url_registry_runner_(std::move(url_registry_runner)) {}
 
 BlobRegistryImpl::~BlobRegistryImpl() {
   // BlobBuilderFromStream needs to be aborted before it can be destroyed, but
@@ -652,11 +654,22 @@ void BlobRegistryImpl::URLStoreForOrigin(
         "BlobRegistryImpl::URLStoreForOrigin");
     return;
   }
-  auto self_owned_associated_receiver = mojo::MakeSelfOwnedAssociatedReceiver(
-      std::make_unique<BlobURLStoreImpl>(origin, url_registry_),
-      std::move(receiver));
-  if (g_url_store_creation_hook)
-    g_url_store_creation_hook->Run(self_owned_associated_receiver);
+  url_registry_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          [](const url::Origin& origin,
+             mojo::PendingAssociatedReceiver<blink::mojom::BlobURLStore>
+                 receiver,
+             base::WeakPtr<BlobUrlRegistry> url_registry) {
+            auto self_owned_associated_receiver =
+                mojo::MakeSelfOwnedAssociatedReceiver(
+                    std::make_unique<BlobURLStoreImpl>(origin,
+                                                       std::move(url_registry)),
+                    std::move(receiver));
+            if (g_url_store_creation_hook)
+              g_url_store_creation_hook->Run(self_owned_associated_receiver);
+          },
+          origin, std::move(receiver), url_registry_));
 }
 
 // static

storage/browser/blob/blob_registry_impl.h

@@ -38,6 +38,7 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) BlobRegistryImpl
 
   BlobRegistryImpl(base::WeakPtr<BlobStorageContext> context,
                    base::WeakPtr<BlobUrlRegistry> url_registry,
+                   scoped_refptr<base::TaskRunner> url_registry_runner,
                    scoped_refptr<FileSystemContext> file_system_context);
 
   BlobRegistryImpl(const BlobRegistryImpl&) = delete;
@@ -95,9 +96,12 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) BlobRegistryImpl
                          std::unique_ptr<BlobDataHandle> result);
 
   base::WeakPtr<BlobStorageContext> context_;
-  base::WeakPtr<BlobUrlRegistry> url_registry_;
   scoped_refptr<FileSystemContext> file_system_context_;
 
+  // `url_registry_` should only be accessed on `url_registry_runner_`.
+  base::WeakPtr<BlobUrlRegistry> url_registry_;
+  scoped_refptr<base::TaskRunner> url_registry_runner_;
+
   mojo::ReceiverSet<blink::mojom::BlobRegistry, std::unique_ptr<Delegate>>
       receivers_;
 

storage/browser/blob/blob_registry_impl_unittest.cc

@@ -87,7 +87,8 @@ class BlobRegistryImplTest : public testing::Test {
                           /*force_in_memory=*/false,
                           std::vector<std::string>()));
     registry_impl_ = std::make_unique<BlobRegistryImpl>(
-        context_->AsWeakPtr(), url_registry_.AsWeakPtr(), file_system_context_);
+        context_->AsWeakPtr(), url_registry_.AsWeakPtr(),
+        base::SequencedTaskRunnerHandle::Get(), file_system_context_);
     auto delegate = std::make_unique<MockBlobRegistryDelegate>();
     delegate_ptr_ = delegate.get();
     registry_impl_->Bind(registry_.BindNewPipeAndPassReceiver(),