IndexedDB: Make IDBCursor.Advance() use native Mojo callback

IDBCursor.Advance() previously took a separate IDBCallbacks interface
that had its own lifecycle and could have a number of methods called
on it.

This change updates Advance() to use Mojo's native callback mechanism
so we can start moving away from the complicated IDBCallbacks
interface and instead use a simpler and easier to reason about async
response return approach.

Bug: 717812
Change-Id: Ia08bebfcafe5f63d60366e81b7f973b1e9ad2378
Reviewed-on: https://chromium-review.googlesource.com/c/1336662
Commit-Queue: Chase Phillips <cmp@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622176}

content/browser/indexed_db/cursor_impl.cc

@@ -19,7 +19,9 @@ class CursorImpl::IDBSequenceHelper {
   explicit IDBSequenceHelper(std::unique_ptr<IndexedDBCursor> cursor);
   ~IDBSequenceHelper();
 
-  void Advance(uint32_t count, scoped_refptr<IndexedDBCallbacks> callbacks);
+  void Advance(uint32_t count,
+               base::WeakPtr<IndexedDBDispatcherHost> dispatcher_host,
+               blink::mojom::IDBCursor::AdvanceCallback callback);
   void Continue(const IndexedDBKey& key,
                 const IndexedDBKey& primary_key,
                 scoped_refptr<IndexedDBCallbacks> callbacks);
@@ -45,15 +47,13 @@ CursorImpl::~CursorImpl() {
   idb_runner_->DeleteSoon(FROM_HERE, helper_);
 }
 
-void CursorImpl::Advance(
-    uint32_t count,
-    blink::mojom::IDBCallbacksAssociatedPtrInfo callbacks_info) {
-  scoped_refptr<IndexedDBCallbacks> callbacks(
-      new IndexedDBCallbacks(dispatcher_host_->AsWeakPtr(), origin_,
-                             std::move(callbacks_info), idb_runner_));
-  idb_runner_->PostTask(FROM_HERE, base::BindOnce(&IDBSequenceHelper::Advance,
-                                                  base::Unretained(helper_),
-                                                  count, std::move(callbacks)));
+void CursorImpl::Advance(uint32_t count,
+                         blink::mojom::IDBCursor::AdvanceCallback callback) {
+  idb_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&IDBSequenceHelper::Advance, base::Unretained(helper_),
+                     count, dispatcher_host_->AsWeakPtr(),
+                     std::move(callback)));
 }
 
 void CursorImpl::CursorContinue(
@@ -96,8 +96,9 @@ CursorImpl::IDBSequenceHelper::~IDBSequenceHelper() {}
 
 void CursorImpl::IDBSequenceHelper::Advance(
     uint32_t count,
-    scoped_refptr<IndexedDBCallbacks> callbacks) {
-  cursor_->Advance(count, std::move(callbacks));
+    base::WeakPtr<content::IndexedDBDispatcherHost> dispatcher_host,
+    blink::mojom::IDBCursor::AdvanceCallback callback) {
+  cursor_->Advance(count, std::move(dispatcher_host), std::move(callback));
 }
 
 void CursorImpl::IDBSequenceHelper::Continue(

content/browser/indexed_db/cursor_impl.h

@@ -31,7 +31,7 @@ class CursorImpl : public blink::mojom::IDBCursor {
 
   // blink::mojom::IDBCursor implementation
   void Advance(uint32_t count,
-               blink::mojom::IDBCallbacksAssociatedPtrInfo callbacks) override;
+               blink::mojom::IDBCursor::AdvanceCallback callback) override;
   void CursorContinue(
       const blink::IndexedDBKey& key,
       const blink::IndexedDBKey& primary_key,

content/browser/indexed_db/indexed_db_cursor.cc

@@ -10,11 +10,14 @@
 
 #include "base/bind.h"
 #include "base/logging.h"
+#include "base/task/post_task.h"
 #include "content/browser/indexed_db/indexed_db_callbacks.h"
 #include "content/browser/indexed_db/indexed_db_database_error.h"
 #include "content/browser/indexed_db/indexed_db_tracing.h"
 #include "content/browser/indexed_db/indexed_db_transaction.h"
 #include "content/browser/indexed_db/indexed_db_value.h"
+#include "content/public/browser/browser_task_traits.h"
+#include "content/public/browser/browser_thread.h"
 #include "third_party/blink/public/platform/modules/indexeddb/web_idb_database_exception.h"
 
 using blink::IndexedDBKey;
@@ -95,24 +98,40 @@ void IndexedDBCursor::Continue(std::unique_ptr<IndexedDBKey> key,
                         base::Passed(&primary_key), callbacks));
 }
 
-void IndexedDBCursor::Advance(uint32_t count,
-                              scoped_refptr<IndexedDBCallbacks> callbacks) {
+void IndexedDBCursor::Advance(
+    uint32_t count,
+    base::WeakPtr<content::IndexedDBDispatcherHost> dispatcher_host,
+    blink::mojom::IDBCursor::AdvanceCallback callback) {
   IDB_TRACE("IndexedDBCursor::Advance");
 
   if (closed_) {
-    callbacks->OnError(CreateCursorClosedError());
+    base::PostTaskWithTraits(
+        FROM_HERE, {BrowserThread::IO},
+        base::BindOnce(
+            [](blink::mojom::IDBCursor::AdvanceCallback callback) {
+              const IndexedDBDatabaseError closed_error(
+                  CreateCursorClosedError());
+              DCHECK_NE(closed_error.code(), 0);
+              std::move(callback).Run(
+                  blink::mojom::IDBError::New(closed_error.code(),
+                                              closed_error.message()),
+                  blink::mojom::IDBCursorValuePtr());
+            },
+            std::move(callback)));
     return;
   }
 
   transaction_->ScheduleTask(
       task_type_,
       BindWeakOperation(&IndexedDBCursor::CursorAdvanceOperation,
-                        ptr_factory_.GetWeakPtr(), count, callbacks));
+                        ptr_factory_.GetWeakPtr(), count,
+                        std::move(dispatcher_host), std::move(callback)));
 }
 
 leveldb::Status IndexedDBCursor::CursorAdvanceOperation(
     uint32_t count,
-    scoped_refptr<IndexedDBCallbacks> callbacks,
+    base::WeakPtr<IndexedDBDispatcherHost> dispatcher_host,
+    blink::mojom::IDBCursor::AdvanceCallback callback,
     IndexedDBTransaction* /*transaction*/) {
   IDB_TRACE("IndexedDBCursor::CursorAdvanceOperation");
   leveldb::Status s = leveldb::Status::OK();
@@ -120,16 +139,66 @@ leveldb::Status IndexedDBCursor::CursorAdvanceOperation(
   if (!cursor_ || !cursor_->Advance(count, &s)) {
     cursor_.reset();
     if (s.ok()) {
-      callbacks->OnSuccess(nullptr);
+      base::PostTaskWithTraits(
+          FROM_HERE, {BrowserThread::IO},
+          base::BindOnce(
+              [](blink::mojom::IDBCursor::AdvanceCallback callback) {
+                std::move(callback).Run(blink::mojom::IDBErrorPtr(),
+                                        blink::mojom::IDBCursorValuePtr());
+              },
+              std::move(callback)));
       return s;
     }
     Close();
-    callbacks->OnError(IndexedDBDatabaseError(
-        blink::kWebIDBDatabaseExceptionUnknownError, "Error advancing cursor"));
+
+    base::PostTaskWithTraits(
+        FROM_HERE, {BrowserThread::IO},
+        base::BindOnce(
+            [](blink::mojom::IDBCursor::AdvanceCallback callback) {
+              std::move(callback).Run(
+                  blink::mojom::IDBError::New(
+                      blink::kWebIDBDatabaseExceptionUnknownError,
+                      base::ASCIIToUTF16("Error advancing cursor")),
+                  blink::mojom::IDBCursorValuePtr());
+            },
+            std::move(callback)));
     return s;
   }
 
-  callbacks->OnSuccess(key(), primary_key(), Value());
+  base::PostTaskWithTraits(
+      FROM_HERE, {BrowserThread::IO},
+      base::BindOnce(
+          [](base::WeakPtr<content::IndexedDBDispatcherHost> dispatcher_host,
+             blink::mojom::IDBCursor::AdvanceCallback callback,
+             IndexedDBKey key, IndexedDBKey primary_key,
+             IndexedDBValue* value) {
+            if (!dispatcher_host)
+              return;
+
+            blink::mojom::IDBValuePtr mojo_value;
+            std::vector<IndexedDBBlobInfo> blob_info;
+            if (value) {
+              mojo_value = IndexedDBValue::ConvertAndEraseValue(value);
+              blob_info.swap(value->blob_info);
+
+              if (!IndexedDBCallbacks::CreateAllBlobs(
+                      dispatcher_host->blob_storage_context(),
+                      dispatcher_host->context(), blob_info,
+                      &mojo_value->blob_or_file_info))
+                return;
+            } else {
+              mojo_value = blink::mojom::IDBValue::New();
+            }
+
+            blink::mojom::IDBCursorValuePtr cursor_value =
+                blink::mojom::IDBCursorValue::New(std::move(key),
+                                                  std::move(primary_key),
+                                                  std::move(mojo_value));
+            std::move(callback).Run(blink::mojom::IDBErrorPtr(),
+                                    std::move(cursor_value));
+          },
+          std::move(dispatcher_host), std::move(callback), key(), primary_key(),
+          Value()));
   return s;
 }
 

content/browser/indexed_db/indexed_db_cursor.h

@@ -29,7 +29,9 @@ class CONTENT_EXPORT IndexedDBCursor {
                   IndexedDBTransaction* transaction);
   ~IndexedDBCursor();
 
-  void Advance(uint32_t count, scoped_refptr<IndexedDBCallbacks> callbacks);
+  void Advance(uint32_t count,
+               base::WeakPtr<IndexedDBDispatcherHost> dispatcher_host,
+               blink::mojom::IDBCursor::AdvanceCallback callback);
   void Continue(std::unique_ptr<blink::IndexedDBKey> key,
                 std::unique_ptr<blink::IndexedDBKey> primary_key,
                 scoped_refptr<IndexedDBCallbacks> callbacks);
@@ -55,7 +57,8 @@ class CONTENT_EXPORT IndexedDBCursor {
       IndexedDBTransaction* transaction);
   leveldb::Status CursorAdvanceOperation(
       uint32_t count,
-      scoped_refptr<IndexedDBCallbacks> callbacks,
+      base::WeakPtr<IndexedDBDispatcherHost> dispatcher_host,
+      blink::mojom::IDBCursor::AdvanceCallback callback,
       IndexedDBTransaction* transaction);
   leveldb::Status CursorPrefetchIterationOperation(
       int number_to_fetch,

third_party/blink/public/mojom/indexeddb/indexeddb.mojom

@@ -269,11 +269,28 @@ interface IDBDatabaseCallbacks {
   Changes(IDBObserverChanges changes);
 };
 
+struct IDBError {
+  int32 error_code;
+  mojo_base.mojom.String16 error_message;
+};
+
+struct IDBCursorValue {
+  IDBKey key;
+  IDBKey primary_key;
+  IDBValue value;
+};
+
 interface IDBCursor {
-  Advance(uint32 count, associated IDBCallbacks callbacks);
+  // Advance() can call its return callback in one of 3 ways:
+  // * with |error| set and |value| unset, if an error occurs
+  // * with |error| unset and |value| set, under normal operation
+  // * with |error| unset and |value| unset, under normal operation when the end
+  //   of the source being iterated is reached
+  Advance(uint32 count) => (IDBError? error, IDBCursorValue? value);
+
   // Avoid calling the following function "Continue" since the Mojo
   // Java bindings generate incorrect Java as a result.  We've named
-  // the following funciton "CursorContinue" as a workaround.
+  // the following function "CursorContinue" as a workaround.
   CursorContinue(IDBKey key,
                  IDBKey primary_key,
                  associated IDBCallbacks callbacks);

third_party/blink/renderer/modules/indexeddb/web_idb_callbacks_impl.h

@@ -93,12 +93,12 @@ class WebIDBCallbacksImpl final : public WebIDBCallbacks {
                      const IDBDatabaseMetadata&) override;
   void DetachRequestFromCallback() override;
 
-  void Detach();
-  void DetachCallbackFromRequest();
-
  private:
   explicit WebIDBCallbacksImpl(IDBRequest*);
 
+  void Detach();
+  void DetachCallbackFromRequest();
+
   Persistent<IDBRequest> request_;
   base::WeakPtr<WebIDBCursorImpl> cursor_;
   int64_t transaction_id_;

third_party/blink/renderer/modules/indexeddb/web_idb_cursor_impl.cc

@@ -13,6 +13,7 @@
 #include "mojo/public/cpp/bindings/strong_associated_binding.h"
 #include "third_party/blink/renderer/modules/indexeddb/idb_key_range.h"
 #include "third_party/blink/renderer/modules/indexeddb/indexed_db_dispatcher.h"
+#include "third_party/blink/renderer/platform/wtf/functional.h"
 
 using blink::mojom::blink::IDBCallbacksAssociatedPtrInfo;
 using blink::mojom::blink::IDBCursorAssociatedPtrInfo;
@@ -49,7 +50,31 @@ void WebIDBCursorImpl::Advance(uint32_t count, WebIDBCallbacks* callbacks_ptr) {
   ResetPrefetchCache();
 
   callbacks->SetState(weak_factory_.GetWeakPtr(), transaction_id_);
-  cursor_->Advance(count, GetCallbacksProxy(std::move(callbacks)));
+  cursor_->Advance(count,
+                   WTF::Bind(&WebIDBCursorImpl::AdvanceCallback,
+                             WTF::Unretained(this), std::move(callbacks)));
+}
+
+void WebIDBCursorImpl::AdvanceCallback(
+    std::unique_ptr<WebIDBCallbacks> callbacks,
+    mojom::blink::IDBErrorPtr error,
+    mojom::blink::IDBCursorValuePtr cursor_value) {
+  if (error) {
+    callbacks->Error(error->error_code, error->error_message);
+    callbacks.reset();
+    return;
+  }
+
+  if (!cursor_value) {
+    callbacks->SuccessValue(nullptr);
+    callbacks.reset();
+    return;
+  }
+
+  callbacks->SuccessCursorContinue(std::move(cursor_value->key),
+                                   std::move(cursor_value->primary_key),
+                                   std::move(cursor_value->value));
+  callbacks.reset();
 }
 
 void WebIDBCursorImpl::CursorContinue(const IDBKey* key,

third_party/blink/renderer/modules/indexeddb/web_idb_cursor_impl.h

@@ -25,6 +25,7 @@ class MODULES_EXPORT WebIDBCursorImpl : public WebIDBCursor {
   ~WebIDBCursorImpl() override;
 
   void Advance(uint32_t count, WebIDBCallbacks* callback) override;
+
   void CursorContinue(const IDBKey* key,
                       const IDBKey* primary_key,
                       WebIDBCallbacks* callback) override;
@@ -43,6 +44,9 @@ class MODULES_EXPORT WebIDBCursorImpl : public WebIDBCursor {
   int64_t transaction_id() const { return transaction_id_; }
 
  private:
+  void AdvanceCallback(std::unique_ptr<WebIDBCallbacks> callbacks,
+                       mojom::blink::IDBErrorPtr error,
+                       mojom::blink::IDBCursorValuePtr value);
   mojom::blink::IDBCallbacksAssociatedPtrInfo GetCallbacksProxy(
       std::unique_ptr<WebIDBCallbacks> callbacks);
 

third_party/blink/renderer/modules/indexeddb/web_idb_cursor_impl_unittest.cc

@@ -44,8 +44,10 @@ class MockCursorImpl : public mojom::blink::IDBCursor {
   }
 
   void Advance(uint32_t count,
-               mojom::blink::IDBCallbacksAssociatedPtrInfo callbacks) override {
+               mojom::blink::IDBCursor::AdvanceCallback callback) override {
     ++advance_calls_;
+    std::move(callback).Run(mojom::blink::IDBErrorPtr(),
+                            mojom::blink::IDBCursorValuePtr());
   }
 
   void CursorContinue(
@@ -83,6 +85,10 @@ class MockContinueCallbacks : public testing::StrictMock<MockWebIDBCallbacks> {
                         Vector<WebBlobInfo>* blobs = nullptr)
       : key_(key), blobs_(blobs) {}
 
+  void SetState(base::WeakPtr<WebIDBCursorImpl> cursor,
+                int64_t transaction_id) override {}
+  void SuccessValue(mojom::blink::IDBReturnValuePtr return_value) override {}
+
   void SuccessCursorContinue(
       std::unique_ptr<IDBKey> key,
       std::unique_ptr<IDBKey> primaryKey,