IndexedDB: Refactor AddCursorBinding into CreateCursorBinding

This relands https://crrev.com/1725318 with a fix for the crash
that can happen under certain circumstances.  The error in that
CL was that dispatch_context() shouldn't be used outside of the
context of a Mojo connection.  Here's the updated commit message:

As part of cleanup from CL https://crrev.com/c/1626660, this CL
moves creation of CursorImpl into IndexedDBDispatcherHost by
refactoring AddCursorBinding into CreateCursorBinding.

As a result, the OpenCursor calling path no longer needs to pass
the sequenced task runner into the OpenCursorOperation task and
ownership of CursorImpl and the mojo binding steps is made
simpler.

Bug: 988868
Change-Id: I8444f53b0c4f362d3152cb9bea232d65f99375a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1728540Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Chase Phillips <cmp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682851}

content/browser/indexed_db/database_impl.cc

@@ -321,7 +321,7 @@ void DatabaseImpl::OpenCursor(
       transaction, object_store_id, index_id,
       std::make_unique<IndexedDBKeyRange>(key_range), direction, key_only,
       task_type, std::move(aborting_callback), origin_,
-      dispatcher_host_->AsWeakPtr(), idb_runner_);
+      dispatcher_host_->AsWeakPtr());
 }
 
 void DatabaseImpl::Count(

content/browser/indexed_db/indexed_db_database.cc

@@ -1814,8 +1814,7 @@ void IndexedDBDatabase::OpenCursor(
     blink::mojom::IDBTaskType task_type,
     blink::mojom::IDBDatabase::OpenCursorCallback callback,
     const url::Origin& origin,
-    base::WeakPtr<IndexedDBDispatcherHost> dispatcher_host,
-    scoped_refptr<base::SequencedTaskRunner> idb_runner) {
+    base::WeakPtr<IndexedDBDispatcherHost> dispatcher_host) {
   DCHECK(transaction);
   IDB_TRACE1("IndexedDBDatabase::OpenCursor", "txn.id", transaction->id());
 
@@ -1832,20 +1831,30 @@ void IndexedDBDatabase::OpenCursor(
       key_only ? indexed_db::CURSOR_KEY_ONLY : indexed_db::CURSOR_KEY_AND_VALUE;
   params->task_type = task_type;
   params->callback = std::move(callback);
-  transaction->ScheduleTask(BindWeakOperation(
-      &IndexedDBDatabase::OpenCursorOperation, AsWeakPtr(), std::move(params),
-      origin, std::move(dispatcher_host), std::move(idb_runner)));
+  transaction->ScheduleTask(
+      BindWeakOperation(&IndexedDBDatabase::OpenCursorOperation, AsWeakPtr(),
+                        std::move(params), origin, std::move(dispatcher_host)));
 }
 
 Status IndexedDBDatabase::OpenCursorOperation(
     std::unique_ptr<OpenCursorOperationParams> params,
     const url::Origin& origin,
     base::WeakPtr<IndexedDBDispatcherHost> dispatcher_host,
-    scoped_refptr<base::SequencedTaskRunner> idb_runner,
     IndexedDBTransaction* transaction) {
   IDB_TRACE1("IndexedDBDatabase::OpenCursorOperation", "txn.id",
              transaction->id());
 
+  Status s = Status::OK();
+  if (!dispatcher_host) {
+    IndexedDBDatabaseError error =
+        CreateError(blink::kWebIDBDatabaseExceptionUnknownError,
+                    "Dispatcher not connected.", transaction);
+    std::move(params->callback)
+        .Run(blink::mojom::IDBDatabaseOpenCursorResult::NewErrorResult(
+            blink::mojom::IDBError::New(error.code(), error.message())));
+    return s;
+  }
+
   // The frontend has begun indexing, so this pauses the transaction
   // until the indexing is complete. This can't happen any earlier
   // because we don't want to switch to early mode in case multiple
@@ -1853,7 +1862,6 @@ Status IndexedDBDatabase::OpenCursorOperation(
   if (params->task_type == blink::mojom::IDBTaskType::Preemptive)
     transaction->AddPreemptiveEvent();
 
-  Status s = Status::OK();
   std::unique_ptr<IndexedDBBackingStore::Cursor> backing_store_cursor;
   if (params->index_id == IndexedDBIndexMetadata::kInvalidId) {
     if (params->cursor_type == indexed_db::CURSOR_KEY_ONLY) {
@@ -1897,16 +1905,6 @@ Status IndexedDBDatabase::OpenCursorOperation(
   IndexedDBCursor* cursor_ptr = cursor.get();
   transaction->RegisterOpenCursor(cursor_ptr);
 
-  if (!dispatcher_host) {
-    IndexedDBDatabaseError error =
-        CreateError(blink::kWebIDBDatabaseExceptionUnknownError,
-                    "Dispatcher not connected.", transaction);
-    std::move(params->callback)
-        .Run(blink::mojom::IDBDatabaseOpenCursorResult::NewErrorResult(
-            blink::mojom::IDBError::New(error.code(), error.message())));
-    return s;
-  }
-
   blink::mojom::IDBValuePtr mojo_value;
   std::vector<IndexedDBBlobInfo> blob_info;
   if (cursor_ptr->Value()) {
@@ -1914,8 +1912,6 @@ Status IndexedDBDatabase::OpenCursorOperation(
     blob_info.swap(cursor_ptr->Value()->blob_info);
   }
 
-  auto cursor_impl = std::make_unique<CursorImpl>(
-      std::move(cursor), origin, dispatcher_host.get(), idb_runner);
   if (mojo_value &&
       !IndexedDBCallbacks::CreateAllBlobs(
           dispatcher_host->blob_storage_context(),
@@ -1924,13 +1920,11 @@ Status IndexedDBDatabase::OpenCursorOperation(
     return s;
   }
 
-  blink::mojom::IDBCursorAssociatedPtrInfo ptr_info;
-  auto request = mojo::MakeRequest(&ptr_info);
-  dispatcher_host->AddCursorBinding(std::move(cursor_impl), std::move(request));
   std::move(params->callback)
       .Run(blink::mojom::IDBDatabaseOpenCursorResult::NewValue(
           blink::mojom::IDBDatabaseOpenCursorValue::New(
-              std::move(ptr_info), cursor_ptr->key(), cursor_ptr->primary_key(),
+              dispatcher_host->CreateCursorBinding(origin, std::move(cursor)),
+              cursor_ptr->key(), cursor_ptr->primary_key(),
               std::move(mojo_value))));
   return s;
 }

content/browser/indexed_db/indexed_db_database.h

@@ -196,8 +196,7 @@ class CONTENT_EXPORT IndexedDBDatabase {
                   blink::mojom::IDBTaskType task_type,
                   blink::mojom::IDBDatabase::OpenCursorCallback callback,
                   const url::Origin& origin,
-                  base::WeakPtr<IndexedDBDispatcherHost> dispatcher_host,
-                  scoped_refptr<base::SequencedTaskRunner> idb_runner);
+                  base::WeakPtr<IndexedDBDispatcherHost> dispatcher_host);
   void Count(IndexedDBTransaction* transaction,
              int64_t object_store_id,
              int64_t index_id,
@@ -275,7 +274,6 @@ class CONTENT_EXPORT IndexedDBDatabase {
       std::unique_ptr<OpenCursorOperationParams> params,
       const url::Origin& origin,
       base::WeakPtr<IndexedDBDispatcherHost> dispatcher_host,
-      scoped_refptr<base::SequencedTaskRunner> idb_runner,
       IndexedDBTransaction* transaction);
   leveldb::Status CountOperation(
       int64_t object_store_id,

content/browser/indexed_db/indexed_db_dispatcher_host.cc

@@ -17,6 +17,7 @@
 #include "content/browser/indexed_db/indexed_db_callbacks.h"
 #include "content/browser/indexed_db/indexed_db_connection.h"
 #include "content/browser/indexed_db/indexed_db_context_impl.h"
+#include "content/browser/indexed_db/indexed_db_cursor.h"
 #include "content/browser/indexed_db/indexed_db_database_callbacks.h"
 #include "content/browser/indexed_db/indexed_db_factory_impl.h"
 #include "content/browser/indexed_db/indexed_db_pending_connection.h"
@@ -93,16 +94,21 @@ void IndexedDBDispatcherHost::AddDatabaseBinding(
   database_bindings_.AddBinding(std::move(database), std::move(request));
 }
 
-void IndexedDBDispatcherHost::AddCursorBinding(
-    std::unique_ptr<CursorImpl> cursor,
-    blink::mojom::IDBCursorAssociatedRequest request) {
+blink::mojom::IDBCursorAssociatedPtrInfo
+IndexedDBDispatcherHost::CreateCursorBinding(
+    const url::Origin& origin,
+    std::unique_ptr<IndexedDBCursor> cursor) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  auto* cursor_ptr = cursor.get();
-  mojo::BindingId binding_id =
-      cursor_bindings_.AddBinding(std::move(cursor), std::move(request));
-  cursor_ptr->OnRemoveBinding(
+  auto cursor_impl = std::make_unique<CursorImpl>(std::move(cursor), origin,
+                                                  this, IDBTaskRunner());
+  auto* cursor_impl_ptr = cursor_impl.get();
+  blink::mojom::IDBCursorAssociatedPtrInfo ptr_info;
+  mojo::BindingId binding_id = cursor_bindings_.AddBinding(
+      std::move(cursor_impl), mojo::MakeRequest(&ptr_info));
+  cursor_impl_ptr->OnRemoveBinding(
       base::BindOnce(&IndexedDBDispatcherHost::RemoveCursorBinding,
                      weak_factory_.GetWeakPtr(), binding_id));
+  return ptr_info;
 }
 
 void IndexedDBDispatcherHost::RemoveCursorBinding(mojo::BindingId binding_id) {

content/browser/indexed_db/indexed_db_dispatcher_host.h

@@ -33,8 +33,8 @@ class Origin;
 }
 
 namespace content {
-class CursorImpl;
 class IndexedDBContextImpl;
+class IndexedDBCursor;
 class IndexedDBTransaction;
 
 // Constructed on UI thread.  All remaining calls (including destruction) should
@@ -55,8 +55,9 @@ class CONTENT_EXPORT IndexedDBDispatcherHost
   void AddDatabaseBinding(std::unique_ptr<blink::mojom::IDBDatabase> database,
                           blink::mojom::IDBDatabaseAssociatedRequest request);
 
-  void AddCursorBinding(std::unique_ptr<CursorImpl> cursor,
-                        blink::mojom::IDBCursorAssociatedRequest request);
+  blink::mojom::IDBCursorAssociatedPtrInfo CreateCursorBinding(
+      const url::Origin& origin,
+      std::unique_ptr<IndexedDBCursor> cursor);
   void RemoveCursorBinding(mojo::BindingId binding_id);
 
   void AddTransactionBinding(