IndexedDB: Update DispatcherHost to use WeakPtr ref to BlobStorageCon…

…text IndexedDBDispatcherHost keeps a scoped_refptr<ChromeBlobStorageContext> so that it can pass a pointer to storage::BlobStorageContext around. The pointer is used during work on the IO thread to access the BlobStorage system. Technically, there's no requirement that the blob storage system be kept alive to complete operations. Really, BlobStorage should always be running, and any situation where it isn't indicates the backing system for storage is being / has been torn down. Since that means that IndexedDBDispatcherHost doesn't actually "own" the ChromeBlobStorageContext instance, it's more accurate for this reference to be held as a WeakPtr and tested when it's needed. This change will be even more important when, in a later change, IndexedDBDispatcherHost's residence will move to the IDB task runner where it won't be possible to destroy ChromeBlobStorageContext correctly during shutdown. Bug: 717812 Change-Id: Ided90f751dc03ab0e59d8be3e4666524929336ba Reviewed-on: https://chromium-review.googlesource.com/c/1457016 Reviewed-by: Daniel Murphy <[email protected]> Commit-Queue: Daniel Murphy <[email protected]> Commit-Queue: Chase Phillips <[email protected]> Cr-Commit-Position: refs/heads/master@{#631085}
feiyue1206 · Feb 12, 2019 · 6fd05b6 · 6fd05b6
1 parent d95fa07
commit 6fd05b6
Show file tree

Hide file tree

Showing 8 changed files with 41 additions and 22 deletions.
diff --git a/content/browser/blob_storage/chrome_blob_storage_context.cc b/content/browser/blob_storage/chrome_blob_storage_context.cc
@@ -88,6 +88,7 @@ class BlobHandleImpl : public BlobHandle {
 
 ChromeBlobStorageContext::ChromeBlobStorageContext() {}
 
+// static
 ChromeBlobStorageContext* ChromeBlobStorageContext::GetFor(
     BrowserContext* context) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);

diff --git a/content/browser/indexed_db/database_impl.cc b/content/browser/indexed_db/database_impl.cc
@@ -282,6 +282,12 @@ void DatabaseImpl::Put(
     blink::mojom::IDBPutMode mode,
     const std::vector<IndexedDBIndexKeys>& index_keys,
     blink::mojom::IDBCallbacksAssociatedPtrInfo callbacks_info) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  CHECK(dispatcher_host_);
+  if (!dispatcher_host_->blob_storage_context()) {
+    return;
+  }
+
   ChildProcessSecurityPolicyImpl* policy =
       ChildProcessSecurityPolicyImpl::GetInstance();
 

diff --git a/content/browser/indexed_db/indexed_db_callbacks.cc b/content/browser/indexed_db/indexed_db_callbacks.cc
@@ -94,7 +94,7 @@ class SafeIOThreadCursorWrapper {
 };
 
 std::unique_ptr<storage::BlobDataHandle> CreateBlobData(
-    storage::BlobStorageContext* blob_context,
+    base::WeakPtr<storage::BlobStorageContext> blob_context,
     IndexedDBContextImpl* indexed_db_context,
     const IndexedDBBlobInfo& blob_info) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
@@ -180,7 +180,7 @@ class IndexedDBCallbacks::IOThreadHelper {
 
 // static
 bool IndexedDBCallbacks::CreateAllBlobs(
-    storage::BlobStorageContext* blob_context,
+    base::WeakPtr<storage::BlobStorageContext> blob_context,
     IndexedDBContextImpl* indexed_db_context,
     const std::vector<IndexedDBBlobInfo>& blob_info,
     std::vector<blink::mojom::IDBBlobInfoPtr>* blob_or_file_info) {
@@ -504,7 +504,7 @@ void IndexedDBCallbacks::IOThreadHelper::SendError(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (!callbacks_)
     return;
-  if (!dispatcher_host_) {
+  if (!dispatcher_host_ || !dispatcher_host_->blob_storage_context()) {
     OnConnectionError();
     return;
   }
@@ -516,7 +516,7 @@ void IndexedDBCallbacks::IOThreadHelper::SendSuccessNamesAndVersionsList(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (!callbacks_)
     return;
-  if (!dispatcher_host_) {
+  if (!dispatcher_host_ || !dispatcher_host_->blob_storage_context()) {
     OnConnectionError();
     return;
   }
@@ -528,7 +528,7 @@ void IndexedDBCallbacks::IOThreadHelper::SendSuccessStringList(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (!callbacks_)
     return;
-  if (!dispatcher_host_) {
+  if (!dispatcher_host_ || !dispatcher_host_->blob_storage_context()) {
     OnConnectionError();
     return;
   }
@@ -537,7 +537,7 @@ void IndexedDBCallbacks::IOThreadHelper::SendSuccessStringList(
 
 void IndexedDBCallbacks::IOThreadHelper::SendBlocked(int64_t existing_version) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  if (!dispatcher_host_) {
+  if (!dispatcher_host_ || !dispatcher_host_->blob_storage_context()) {
     OnConnectionError();
     return;
   }
@@ -554,7 +554,7 @@ void IndexedDBCallbacks::IOThreadHelper::SendUpgradeNeeded(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (!callbacks_)
     return;
-  if (!dispatcher_host_) {
+  if (!dispatcher_host_ || !dispatcher_host_->blob_storage_context()) {
     OnConnectionError();
     return;
   }
@@ -577,7 +577,7 @@ void IndexedDBCallbacks::IOThreadHelper::SendSuccessDatabase(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (!callbacks_)
     return;
-  if (!dispatcher_host_) {
+  if (!dispatcher_host_ || !dispatcher_host_->blob_storage_context()) {
     OnConnectionError();
     return;
   }
@@ -603,7 +603,7 @@ void IndexedDBCallbacks::IOThreadHelper::SendSuccessCursor(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (!callbacks_)
     return;
-  if (!dispatcher_host_) {
+  if (!dispatcher_host_ || !dispatcher_host_->blob_storage_context()) {
     OnConnectionError();
     return;
   }
@@ -629,7 +629,7 @@ void IndexedDBCallbacks::IOThreadHelper::SendSuccessValue(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (!callbacks_)
     return;
-  if (!dispatcher_host_) {
+  if (!dispatcher_host_ || !dispatcher_host_->blob_storage_context()) {
     OnConnectionError();
     return;
   }
@@ -649,7 +649,7 @@ void IndexedDBCallbacks::IOThreadHelper::SendSuccessArray(
 
   if (!callbacks_)
     return;
-  if (!dispatcher_host_) {
+  if (!dispatcher_host_ || !dispatcher_host_->blob_storage_context()) {
     OnConnectionError();
     return;
   }
@@ -672,7 +672,7 @@ void IndexedDBCallbacks::IOThreadHelper::SendSuccessCursorContinue(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (!callbacks_)
     return;
-  if (!dispatcher_host_) {
+  if (!dispatcher_host_ || !dispatcher_host_->blob_storage_context()) {
     OnConnectionError();
     return;
   }
@@ -694,7 +694,7 @@ void IndexedDBCallbacks::IOThreadHelper::SendSuccessCursorPrefetch(
 
   if (!callbacks_)
     return;
-  if (!dispatcher_host_) {
+  if (!dispatcher_host_ || !dispatcher_host_->blob_storage_context()) {
     OnConnectionError();
     return;
   }
@@ -716,7 +716,7 @@ void IndexedDBCallbacks::IOThreadHelper::SendSuccessKey(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (!callbacks_)
     return;
-  if (!dispatcher_host_) {
+  if (!dispatcher_host_ || !dispatcher_host_->blob_storage_context()) {
     OnConnectionError();
     return;
   }
@@ -727,7 +727,7 @@ void IndexedDBCallbacks::IOThreadHelper::SendSuccessInteger(int64_t value) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (!callbacks_)
     return;
-  if (!dispatcher_host_) {
+  if (!dispatcher_host_ || !dispatcher_host_->blob_storage_context()) {
     OnConnectionError();
     return;
   }
@@ -738,7 +738,7 @@ void IndexedDBCallbacks::IOThreadHelper::SendSuccess() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (!callbacks_)
     return;
-  if (!dispatcher_host_) {
+  if (!dispatcher_host_ || !dispatcher_host_->blob_storage_context()) {
     OnConnectionError();
     return;
   }

diff --git a/content/browser/indexed_db/indexed_db_callbacks.h b/content/browser/indexed_db/indexed_db_callbacks.h
@@ -47,7 +47,7 @@ class CONTENT_EXPORT IndexedDBCallbacks
     : public base::RefCounted<IndexedDBCallbacks> {
  public:
   static bool CreateAllBlobs(
-      storage::BlobStorageContext* blob_context,
+      base::WeakPtr<storage::BlobStorageContext> blob_context,
       IndexedDBContextImpl* indexed_db_context,
       const std::vector<IndexedDBBlobInfo>& blob_info,
       std::vector<blink::mojom::IDBBlobInfoPtr>* blob_or_file_info);

diff --git a/content/browser/indexed_db/indexed_db_cursor.cc b/content/browser/indexed_db/indexed_db_cursor.cc
@@ -174,7 +174,7 @@ leveldb::Status IndexedDBCursor::CursorAdvanceOperation(
              blink::mojom::IDBCursor::AdvanceCallback callback,
              IndexedDBKey key, IndexedDBKey primary_key,
              IndexedDBValue* value) {
-            if (!dispatcher_host)
+            if (!dispatcher_host || !dispatcher_host->blob_storage_context())
               return;
 
             blink::mojom::IDBValuePtr mojo_value;

diff --git a/content/browser/indexed_db/indexed_db_dispatcher_host.cc b/content/browser/indexed_db/indexed_db_dispatcher_host.cc
@@ -125,8 +125,7 @@ IndexedDBDispatcherHost::IndexedDBDispatcherHost(
     : indexed_db_context_(std::move(indexed_db_context)),
       blob_storage_context_(std::move(blob_storage_context)),
       ipc_process_id_(ipc_process_id),
-      idb_helper_(new IDBSequenceHelper(ipc_process_id_,
-                                        indexed_db_context_)),
+      idb_helper_(new IDBSequenceHelper(ipc_process_id_, indexed_db_context_)),
       weak_factory_(this) {
   DCHECK(indexed_db_context_.get());
 }
@@ -138,6 +137,12 @@ IndexedDBDispatcherHost::~IndexedDBDispatcherHost() {
 void IndexedDBDispatcherHost::AddBinding(
     blink::mojom::IDBFactoryRequest request,
     const url::Origin& origin) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  if (blob_storage_context_) {
+    io_weak_blob_storage_context_ =
+        blob_storage_context_->context()->AsWeakPtr();
+    blob_storage_context_.reset();
+  }
   bindings_.AddBinding(this, std::move(request), {origin});
 }
 

diff --git a/content/browser/indexed_db/indexed_db_dispatcher_host.h b/content/browser/indexed_db/indexed_db_dispatcher_host.h
@@ -58,8 +58,8 @@ class CONTENT_EXPORT IndexedDBDispatcherHost
 
   // A shortcut for accessing our context.
   IndexedDBContextImpl* context() const { return indexed_db_context_.get(); }
-  storage::BlobStorageContext* blob_storage_context() const {
-    return blob_storage_context_->context();
+  base::WeakPtr<storage::BlobStorageContext> blob_storage_context() const {
+    return io_weak_blob_storage_context_;
   }
   int ipc_process_id() const { return ipc_process_id_; }
 
@@ -107,7 +107,9 @@ class CONTENT_EXPORT IndexedDBDispatcherHost
   base::SequencedTaskRunner* IDBTaskRunner() const;
 
   scoped_refptr<IndexedDBContextImpl> indexed_db_context_;
+
   scoped_refptr<ChromeBlobStorageContext> blob_storage_context_;
+  base::WeakPtr<storage::BlobStorageContext> io_weak_blob_storage_context_;
 
   // Used to set file permissions for blob storage.
   const int ipc_process_id_;

diff --git a/content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc b/content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc
@@ -193,6 +193,11 @@ class IndexedDBDispatcherHostTest : public testing::Test {
   }
 
   void SetUp() override {
+    // ChromeBlobStorageContext::GetFor() issues PostTask() calls to the IO
+    // thread.  Let those run before calling AddBinding().
+    base::RunLoop loop;
+    loop.RunUntilIdle();
+
     host_->AddBinding(::mojo::MakeRequest(&idb_mojo_factory_),
                       {url::Origin::Create(GURL(kOrigin))});
   }