storage: Remove IO thread hops for BlobStorageContentWrapper

This is a followup to remove unnecessary thread hops by
binding the mojo interface on the correct sequence in the
first place.

Bug: 1022104
Change-Id: Id69811673d6a14e11a1da4caa452c40868454df1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1907095
Commit-Queue: enne <enne@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Rayan Kanso <rayankans@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/master@{#718391}

content/browser/background_fetch/background_fetch_test_data_manager.cc

@@ -76,8 +76,11 @@ void BackgroundFetchTestDataManager::InitializeOnCoreThread() {
       base::MakeRefCounted<CacheStorageContextImpl::ObserverList>());
   DCHECK(cache_manager_);
 
-  auto context = base::MakeRefCounted<BlobStorageContextWrapper>(
-      blob_storage_context_->MojoContext());
+  mojo::PendingRemote<storage::mojom::BlobStorageContext> remote;
+  blob_storage_context_->BindMojoContext(
+      remote.InitWithNewPipeAndPassReceiver());
+  auto context =
+      base::MakeRefCounted<BlobStorageContextWrapper>(std::move(remote));
   cache_manager_->SetBlobParametersForCache(std::move(context));
 }
 

content/browser/blob_storage/blob_storage_context_wrapper.cc

@@ -11,18 +11,18 @@ namespace content {
 
 BlobStorageContextWrapper::BlobStorageContextWrapper(
     mojo::PendingRemote<storage::mojom::BlobStorageContext> context) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   context_.Bind(std::move(context));
 }
 
 mojo::Remote<storage::mojom::BlobStorageContext>&
 BlobStorageContextWrapper::context() {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return context_;
 }
 
 BlobStorageContextWrapper::~BlobStorageContextWrapper() {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 }
 
 }  // namespace content

content/browser/blob_storage/blob_storage_context_wrapper.h

@@ -16,35 +16,24 @@
 
 namespace content {
 
-// An IO-thread bound wrapper for the BlobStorageContext remote.
+// A refcounted wrapper for the BlobStorageContext remote.
+// Everything in this wrapper must be called from the same sequence.
 // This gets passed through many layers of classes on different sequences
 // and so it's easier to have the remote bound on the correct sequence
 // and share that remote than it is to bind and clone repeatedly.
-// This is also more efficient than SharedRemote as we know that everything
-// will use this remote from the IO thread.
-//
-// TODO(enne): once cache storage and idb have been converted to talk to the
-// blob system over mojo, there should be no more need for the IO thread hops
-// and everything could be run on the same sequence, eliminating the need
-// for this class.
 class CONTENT_EXPORT BlobStorageContextWrapper
-    : public base::RefCountedThreadSafe<BlobStorageContextWrapper,
-                                        BrowserThread::DeleteOnIOThread> {
+    : public base::RefCounted<BlobStorageContextWrapper> {
  public:
-  // Must be called from the IO thread.
   BlobStorageContextWrapper(
       mojo::PendingRemote<storage::mojom::BlobStorageContext> context);
 
-  // Must be called from the IO thread.
   mojo::Remote<storage::mojom::BlobStorageContext>& context();
 
  private:
-  friend class base::RefCountedThreadSafe<BlobStorageContextWrapper>;
-  friend struct BrowserThread::DeleteOnThread<BrowserThread::IO>;
-  friend class base::DeleteHelper<BlobStorageContextWrapper>;
+  friend class base::RefCounted<BlobStorageContextWrapper>;
+
+  SEQUENCE_CHECKER(sequence_checker_);
 
-  // Must be destroyed on the IO thread, but RefCountedThreadSafe takes
-  // care of that automatically.
   ~BlobStorageContextWrapper();
 
   mojo::Remote<storage::mojom::BlobStorageContext> context_;

content/browser/blob_storage/chrome_blob_storage_context.cc

@@ -160,12 +160,10 @@ storage::BlobStorageContext* ChromeBlobStorageContext::context() const {
   return context_.get();
 }
 
-mojo::PendingRemote<storage::mojom::BlobStorageContext>
-ChromeBlobStorageContext::MojoContext() const {
+void ChromeBlobStorageContext::BindMojoContext(
+    mojo::PendingReceiver<storage::mojom::BlobStorageContext> receiver) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  mojo::PendingRemote<storage::mojom::BlobStorageContext> remote;
-  context_->Bind(remote.InitWithNewPipeAndPassReceiver());
-  return remote;
+  context_->Bind(std::move(receiver));
 }
 
 std::unique_ptr<BlobHandle> ChromeBlobStorageContext::CreateMemoryBackedBlob(

content/browser/blob_storage/chrome_blob_storage_context.h

@@ -58,9 +58,10 @@ class CONTENT_EXPORT ChromeBlobStorageContext
 
   storage::BlobStorageContext* context() const;
 
-  // Return a BlobStorageContext mojo interface to be used by storage apis.
+  // Bind a BlobStorageContext mojo interface to be used by storage apis.
   // This interface should not be exposed to renderers.
-  mojo::PendingRemote<storage::mojom::BlobStorageContext> MojoContext() const;
+  void BindMojoContext(
+      mojo::PendingReceiver<storage::mojom::BlobStorageContext> receiver);
 
   // Returns a NULL scoped_ptr on failure.
   std::unique_ptr<BlobHandle> CreateMemoryBackedBlob(

content/browser/cache_storage/cache_storage_cache_entry_handler.cc

@@ -6,7 +6,6 @@
 
 #include "base/guid.h"
 #include "base/optional.h"
-#include "base/task/post_task.h"
 #include "content/browser/background_fetch/storage/cache_entry_handler_impl.h"
 #include "content/browser/cache_storage/cache_storage_manager.h"
 #include "content/browser/cache_storage/legacy/legacy_cache_storage.h"
@@ -171,16 +170,6 @@ class EntryReaderImpl : public storage::mojom::BlobDataItemReader {
   DISALLOW_COPY_AND_ASSIGN(EntryReaderImpl);
 };
 
-void FinalizeBlobOnIOThread(
-    scoped_refptr<BlobStorageContextWrapper> blob_storage_context,
-    storage::mojom::BlobDataItemPtr element,
-    const std::string& uuid,
-    mojo::PendingReceiver<blink::mojom::Blob> receiver) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  blob_storage_context->context()->RegisterFromDataItem(
-      std::move(receiver), uuid, std::move(element));
-}
-
 }  // namespace
 
 CacheStorageCacheEntryHandler::DiskCacheBlobEntry::DiskCacheBlobEntry(
@@ -403,29 +392,14 @@ CacheStorageCacheEntryHandler::CreateBlobWithSideData(
           : blob_entry->GetSize(side_data_disk_cache_index);
   element->type = storage::mojom::BlobDataItemType::kCacheStorage;
 
-  // Bind the blob data item on the current sequence.  This ensures
-  // that the mojo messages are delivered directly to the correct
-  // cache_storage sequence.  This works even if the cache_storage is
-  // running off the IO thread.
   auto handle = std::make_unique<EntryReaderImpl>(
       std::move(blob_entry), disk_cache_index, side_data_disk_cache_index);
   mojo::MakeSelfOwnedReceiver(std::move(handle),
                               element->reader.InitWithNewPipeAndPassReceiver());
 
-  // We can only register the blob in the storage context on the IO thread.
-  // TODO(crbug/1022104): Once the blob context is not locked to the IO thread
-  //                      we can finalize the blob directly on the sequence.
-  if (BrowserThread::CurrentlyOn(BrowserThread::IO)) {
-    FinalizeBlobOnIOThread(blob_storage_context_, std::move(element),
-                           blob->uuid,
-                           blob->blob.InitWithNewPipeAndPassReceiver());
-  } else {
-    base::PostTask(
-        FROM_HERE, {BrowserThread::IO},
-        base::BindOnce(&FinalizeBlobOnIOThread, blob_storage_context_,
-                       std::move(element), blob->uuid,
-                       blob->blob.InitWithNewPipeAndPassReceiver()));
-  }
+  blob_storage_context_->context()->RegisterFromDataItem(
+      blob->blob.InitWithNewPipeAndPassReceiver(), blob->uuid,
+      std::move(element));
 
   return blob;
 }

content/browser/cache_storage/cache_storage_cache_entry_handler.h

@@ -141,7 +141,7 @@ class CONTENT_EXPORT CacheStorageCacheEntryHandler {
       CacheStorageCache::EntryIndex disk_cache_index,
       CacheStorageCache::EntryIndex side_data_disk_cache_index);
 
-  // IO thread wrapper for storage::mojom::BlobStorageContext.
+  // Wrapper for storage::mojom::BlobStorageContext bound to this sequence.
   scoped_refptr<BlobStorageContextWrapper> blob_storage_context_;
 
   // Every subclass should provide its own implementation to avoid partial

content/browser/cache_storage/cache_storage_cache_unittest.cc

@@ -398,8 +398,12 @@ class CacheStorageCacheTest : public testing::Test {
         ChromeBlobStorageContext::GetFor(&browser_context_);
     // Wait for chrome_blob_storage_context to finish initializing.
     base::RunLoop().RunUntilIdle();
-    blob_storage_context_ = base::MakeRefCounted<BlobStorageContextWrapper>(
-        blob_storage_context->MojoContext());
+
+    mojo::PendingRemote<storage::mojom::BlobStorageContext> remote;
+    blob_storage_context->BindMojoContext(
+        remote.InitWithNewPipeAndPassReceiver());
+    blob_storage_context_ =
+        base::MakeRefCounted<BlobStorageContextWrapper>(std::move(remote));
 
     const bool is_incognito = MemoryOnly();
     if (!is_incognito) {

content/browser/cache_storage/cache_storage_context_impl.cc

@@ -255,21 +255,26 @@ void CacheStorageContextImpl::GetBlobStorageMojoContextOnIOThread(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(blob_storage_context);
 
-  // TODO(enne): this receiver will need to be sent to the storage service when
+  mojo::PendingRemote<storage::mojom::BlobStorageContext> remote;
+  blob_storage_context->BindMojoContext(
+      remote.InitWithNewPipeAndPassReceiver());
+
+  // TODO(enne): this remote will need to be sent to the storage service when
   // cache storage is moved.
-  auto context = blob_storage_context->MojoContext();
   task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(
           &CacheStorageContextImpl::SetBlobParametersForCacheOnTaskRunner, this,
-          base::MakeRefCounted<BlobStorageContextWrapper>(std::move(context))));
+          std::move(remote)));
 }
 
 void CacheStorageContextImpl::SetBlobParametersForCacheOnTaskRunner(
-    scoped_refptr<BlobStorageContextWrapper> blob_storage_context) {
+    mojo::PendingRemote<storage::mojom::BlobStorageContext> remote) {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
-  if (cache_manager_)
-    cache_manager_->SetBlobParametersForCache(std::move(blob_storage_context));
+  if (!cache_manager_)
+    return;
+  cache_manager_->SetBlobParametersForCache(
+      base::MakeRefCounted<BlobStorageContextWrapper>(std::move(remote)));
 }
 
 void CacheStorageContextImpl::CreateQuotaClientsOnIOThread(

content/browser/cache_storage/cache_storage_context_impl.h

@@ -125,7 +125,7 @@ class CONTENT_EXPORT CacheStorageContextImpl
       ChromeBlobStorageContext* blob_storage_context);
 
   void SetBlobParametersForCacheOnTaskRunner(
-      scoped_refptr<BlobStorageContextWrapper> blob_storage_context);
+      mojo::PendingRemote<storage::mojom::BlobStorageContext> remote);
 
   void CreateQuotaClientsOnIOThread(
       scoped_refptr<storage::QuotaManagerProxy> quota_manager_proxy);

content/browser/cache_storage/cache_storage_manager_unittest.cc

@@ -361,8 +361,12 @@ class CacheStorageManagerTest : public testing::Test {
     // Wait for ChromeBlobStorageContext to finish initializing.
     base::RunLoop().RunUntilIdle();
 
-    blob_storage_context_ = base::MakeRefCounted<BlobStorageContextWrapper>(
-        blob_storage_context->MojoContext());
+    mojo::PendingRemote<storage::mojom::BlobStorageContext> remote;
+    blob_storage_context->BindMojoContext(
+        remote.InitWithNewPipeAndPassReceiver());
+    blob_storage_context_ =
+        base::MakeRefCounted<BlobStorageContextWrapper>(std::move(remote));
+
     base::FilePath temp_dir_path;
     if (!MemoryOnly())
       temp_dir_path = temp_dir_.GetPath();