[IndexedDB]: Add commit check to aborting txns.

A check is added to ensure that transactions that have already been sent
a commit signal from the front end (for whatever reason) are committed
instead of aborted in the event of an abort.

Spec Change: https://github.com/w3c/IndexedDB/pull/242

Explainer: https://andreas-butler.github.io/idb-transaction-commit/EXPLAINER
Change-Id: I942136c1bbfb8a5347bf7cf1f9702f5cbff66521
Reviewed-on: https://chromium-review.googlesource.com/c/1330692
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619859}

content/browser/indexed_db/indexed_db_browsertest.cc

@@ -336,6 +336,20 @@ class IndexedDBBrowserTestWithGCExposed : public IndexedDBBrowserTest {
   DISALLOW_COPY_AND_ASSIGN(IndexedDBBrowserTestWithGCExposed);
 };
 
+class IndexedDBBrowserTestWithExperimentalWebFeatures
+    : public IndexedDBBrowserTest {
+ public:
+  IndexedDBBrowserTestWithExperimentalWebFeatures() = default;
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitch(
+        switches::kEnableExperimentalWebPlatformFeatures);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(IndexedDBBrowserTestWithExperimentalWebFeatures);
+};
+
 IN_PROC_BROWSER_TEST_F(IndexedDBBrowserTestWithGCExposed,
                        DatabaseCallbacksTest) {
   SimpleTest(GetTestUrl("indexeddb", "database_callbacks_first.html"));
@@ -925,6 +939,34 @@ IN_PROC_BROWSER_TEST_F(
   EXPECT_EQ(expected_title16, title_watcher.WaitAndGetTitle());
 }
 
+// Testing abort ordering for commit. Verifies that an explicitly committed
+// transaction blocked by another pending transaction will be committed rather
+// than aborted in the event that the page crashes before its committed.
+IN_PROC_BROWSER_TEST_F(IndexedDBBrowserTestWithExperimentalWebFeatures,
+                       CommitContinuesOnAbort) {
+  // Sets up a database, opens four transactions against it such that two end
+  // up indefinitely blocked, one of which is explicitly committed.
+  NavigateAndWaitForTitle(shell(), "blocked_explicit_commit.html", "#tab1",
+                          "transactions registered");
+
+  // Crashes the tab to cause the database set up above to force close with the
+  // blocked transactions still open.
+  shell()->web_contents()->GetMainFrame()->GetProcess()->Shutdown(0);
+
+  // Reopens the same page that was just crashed and inspects the database to
+  // see the results of the transactions that were open at time of crash.
+  Shell* new_shell = CreateBrowser();
+  GURL url = GetTestUrl("indexeddb", "blocked_explicit_commit.html");
+  url = GURL(url.spec() + "#tab2");
+  base::string16 expected_title16(
+      ASCIIToUTF16("transactions aborted and committed as expected"));
+  TitleWatcher title_watcher(new_shell->web_contents(), expected_title16);
+  title_watcher.AlsoWaitForTitle(
+      ASCIIToUTF16("fail - transactions did not abort and commit as expected"));
+  NavigateToURL(new_shell, url);
+  EXPECT_EQ(expected_title16, title_watcher.WaitAndGetTitle());
+}
+
 // Verify that a "close" event is fired at database connections when
 // the backing store is deleted.
 IN_PROC_BROWSER_TEST_F(IndexedDBBrowserTest, ForceCloseEventTest) {

content/browser/indexed_db/indexed_db_connection.cc

@@ -123,13 +123,21 @@ void IndexedDBConnection::AbortTransaction(
   transaction->Abort(error);
 }
 
-void IndexedDBConnection::AbortAllTransactions(
+void IndexedDBConnection::FinishAllTransactions(
     const IndexedDBDatabaseError& error) {
   std::unordered_map<int64_t, std::unique_ptr<IndexedDBTransaction>> temp_map;
   std::swap(temp_map, transactions_);
   for (const auto& pair : temp_map) {
-    IDB_TRACE1("IndexedDBDatabase::Abort(error)", "txn.id", pair.second->id());
-    pair.second->Abort(error);
+    auto& transaction = pair.second;
+    if (transaction->is_commit_pending()) {
+      IDB_TRACE1("IndexedDBDatabase::Commit", "transaction.id",
+                 transaction->id());
+      transaction->ForcePendingCommit();
+    } else {
+      IDB_TRACE1("IndexedDBDatabase::Abort(error)", "transaction.id",
+                 transaction->id());
+      transaction->Abort(error);
+    }
   }
 }
 

content/browser/indexed_db/indexed_db_connection.h

@@ -66,7 +66,11 @@ class CONTENT_EXPORT IndexedDBConnection {
   void AbortTransaction(IndexedDBTransaction* transaction,
                         const IndexedDBDatabaseError& error);
 
-  void AbortAllTransactions(const IndexedDBDatabaseError& error);
+  // Aborts or commits each transaction owned by this connection depending on
+  // the transaction's current state. Any transaction with is_commit_pending_
+  // false is aborted, and any transaction with is_commit_pending_ true is
+  // committed.
+  void FinishAllTransactions(const IndexedDBDatabaseError& error);
 
   IndexedDBTransaction* GetTransaction(int64_t id) const;
 

content/browser/indexed_db/indexed_db_database.cc

@@ -1892,7 +1892,7 @@ void IndexedDBDatabase::Close(IndexedDBConnection* connection, bool forced) {
   // happen if the close is requested by the connection itself as the
   // front-end defers the close until all transactions are complete, but can
   // occur on process termination or forced close.
-  connection->AbortAllTransactions(IndexedDBDatabaseError(
+  connection->FinishAllTransactions(IndexedDBDatabaseError(
       blink::kWebIDBDatabaseExceptionUnknownError, "Connection is closing."));
 
   // Abort transactions before removing the connection; aborting may complete
@@ -1947,7 +1947,7 @@ void IndexedDBDatabase::AbortAllTransactionsForConnections() {
   IDB_TRACE("IndexedDBDatabase::AbortAllTransactionsForConnections");
 
   for (IndexedDBConnection* connection : connections_) {
-    connection->AbortAllTransactions(
+    connection->FinishAllTransactions(
         IndexedDBDatabaseError(blink::kWebIDBDatabaseExceptionUnknownError,
                                "Database is compacting."));
   }

content/browser/indexed_db/indexed_db_transaction.cc

@@ -178,9 +178,27 @@ void IndexedDBTransaction::RunTasksIfStarted() {
                                 ptr_factory_.GetWeakPtr()));
 }
 
+void IndexedDBTransaction::ForcePendingCommit() {
+  IDB_TRACE1("IndexedDBTransaction::ForceCommit", "txn.id", id());
+  DCHECK(is_commit_pending_);
+  if (state_ == FINISHED)
+    return;
+
+  should_process_queue_ = true;
+  state_ = STARTED;
+  if (!task_queue_.empty()) {
+    // Commits when completed.
+    ProcessTaskQueue();
+  } else {
+    leveldb::Status result = Commit();
+    if (!result.ok())
+      database_->ReportError(result);
+  }
+}
+
 void IndexedDBTransaction::Abort(const IndexedDBDatabaseError& error) {
-  IDB_TRACE1("IndexedDBTransaction::Abort", "txn.id", id());
   DCHECK(!processing_event_queue_);
+  DCHECK(!is_commit_pending_);
   if (state_ == FINISHED)
     return;
 
@@ -254,7 +272,7 @@ void IndexedDBTransaction::Start() {
   diagnostics_.start_time = base::Time::Now();
 
   if (!used_) {
-    if (commit_pending_) {
+    if (is_commit_pending_) {
       // The transaction has never had requests issued against it, but the
       // front-end previously requested a commit; do the commit now, but not
       // re-entrantly as that may renter the coordinator.
@@ -328,7 +346,7 @@ leveldb::Status IndexedDBTransaction::Commit() {
     return leveldb::Status::OK();
   DCHECK_NE(state_, COMMITTING);
 
-  commit_pending_ = true;
+  is_commit_pending_ = true;
 
   // Front-end has requested a commit, but this transaction is blocked by
   // other transactions. The commit will be initiated when the transaction
@@ -500,7 +518,7 @@ void IndexedDBTransaction::ProcessTaskQueue() {
 
   // If there are no pending tasks, we haven't already committed/aborted,
   // and the front-end requested a commit, it is now safe to do so.
-  if (!HasPendingTasks() && state_ != FINISHED && commit_pending_) {
+  if (!HasPendingTasks() && state_ != FINISHED && is_commit_pending_) {
     processing_event_queue_ = false;
     // This can delete |this|.
     leveldb::Status result = Commit();

content/browser/indexed_db/indexed_db_transaction.h

@@ -62,6 +62,11 @@ class CONTENT_EXPORT IndexedDBTransaction {
 
   leveldb::Status Commit();
 
+  // If is_commit_pending_ is true this method does the necessary state
+  // manipulation to prepare the transaction to be committed, processes its
+  // task_queue_, and commits the transaction.
+  void ForcePendingCommit();
+
   // This object is destroyed by this method.
   void Abort(const IndexedDBDatabaseError& error);
 
@@ -104,6 +109,7 @@ class CONTENT_EXPORT IndexedDBTransaction {
   IndexedDBDatabase* database() const { return database_.get(); }
   IndexedDBDatabaseCallbacks* callbacks() const { return callbacks_.get(); }
   IndexedDBConnection* connection() const { return connection_.get(); }
+  bool is_commit_pending() const { return is_commit_pending_; }
 
   State state() const { return state_; }
   bool IsTimeoutTimerRunning() const { return timeout_timer_.IsRunning(); }
@@ -182,7 +188,7 @@ class CONTENT_EXPORT IndexedDBTransaction {
 
   bool used_ = false;
   State state_ = CREATED;
-  bool commit_pending_ = false;
+  bool is_commit_pending_ = false;
   // We are owned by the connection object, but during force closes sometimes
   // there are issues if there is a pending OpenRequest. So use a WeakPtr.
   base::WeakPtr<IndexedDBConnection> connection_;

content/browser/indexed_db/indexed_db_transaction_unittest.cc

@@ -435,7 +435,7 @@ TEST_P(IndexedDBTransactionTestMode, AbortPreemptive) {
   EXPECT_FALSE(transaction->should_process_queue_);
   EXPECT_TRUE(transaction->backing_store_transaction_begun_);
   EXPECT_TRUE(transaction->used_);
-  EXPECT_FALSE(transaction->commit_pending_);
+  EXPECT_FALSE(transaction->is_commit_pending_);
 
   // This task will be ignored.
   transaction->ScheduleTask(

content/test/data/indexeddb/blocked_explicit_commit.html

+<html>
+  <head>
+    <title>IndexedDB regression test for bug 98562</title>
+    <script type="text/javascript" src="shared.js"></script>
+    <script type="text/javascript" src="common.js"></script>
+    <script type="text/javascript" src="blocked_explicit_commit.js"></script>
+  </head>
+  <body onLoad="test()">
+    <div id="status">Starting...</div>
+  </body>
+</html>

content/test/data/indexeddb/blocked_explicit_commit.js

+// Copyright (c) 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/**
+ * @fileoverview Javascript test code for verifying proper committing
+ * behaviour for transactions that are blocked when a tab is force
+ * closed.
+ */
+
+function test() {
+  if (document.location.hash === '#tab1') {
+    setUpBlockedTransactions();
+  } else if (document.location.hash === '#tab2') {
+    checkForCommit();
+  } else {
+    result('fail - unexpected hash');
+  }
+}
+
+function upgradeCallback() {
+  db = event.target.result;
+  deleteAllObjectStores(db);
+  db.createObjectStore('store');
+}
+
+// We register four transactions, all on the same object store and all readwrite
+// so as to incur blocking, and verify the expected results after a crash.
+async function setUpBlockedTransactions() {
+  const db = await
+      promiseDeleteThenOpenDb('blocked-explicit-commit', upgradeCallback);
+
+  // An auto-committed put that we expect to be committed.
+  db.transaction('store', 'readwrite').objectStore('store')
+      .put('auto', 'auto-key');
+
+  // A transaction with a put request on it that we keep alive with a request
+  // loop and which we expect to never commit.
+  const blockingTransaction = db.transaction('store', 'readwrite');
+  const blockingRequest = blockingTransaction.objectStore('store')
+      .put('blocking', 'blocking-key');
+  blockingRequest.onsuccess = () => { result('transactions registered'); };
+  keepAlive(blockingTransaction, 'store');
+
+  // A transaction with a put request on it that is blocked by the previous
+  // transaction. We call an explicit commit on this transaction and so expect
+  // its data to be committed after tab1 crashes and the blocking transaction is
+  // aborted.
+  const commitTransaction = db.transaction('store', 'readwrite');
+  commitTransaction.objectStore('store').put('explicit', 'explicit-key');
+  commitTransaction.commit();
+
+  // A transaction with a put request on it that is blocked by the explicit
+  // commit transaction. It is expected to be aborted and thus never commit
+  // because it was not explicitly committed.
+  db.transaction('store', 'readwrite').objectStore('store')
+      .put('blocked', 'blocked-key');
+}
+
+async function checkForCommit() {
+  const db = await promiseOpenDb('blocked-explicit-commit');
+  const transaction = db.transaction('store', 'readonly');
+  const objectStore = transaction.objectStore('store');
+
+  const autoRequest = objectStore.get('auto-key');
+  const blockingRequest = objectStore.get('blocking-key');
+  const explicitRequest = objectStore.get('explicit-key');
+  const blockedRequest = objectStore.get('blocked-key');
+
+  for (const request of [autoRequest, blockingRequest, explicitRequest,
+      blockedRequest]) {
+    request.onerror = unexpectedErrorCallback;
+    request.onblocked = unexpectedBlockedCallback;
+  }
+
+  transaction.oncomplete = () => {
+    if (autoRequest.result == 'auto' && blockingRequest.result == undefined
+        && explicitRequest.result == 'explicit'
+        && blockedRequest.result == undefined) {
+      result('transactions aborted and committed as expected');
+    } else {
+      result('fail - transactions did not abort and commit as expected');
+    }
+  };
+}

content/test/data/indexeddb/common.js

@@ -147,6 +147,75 @@ function indexedDBTest(upgradeCallback, optionalOpenCallback) {
   };
 }
 
+function promiseDeleteThenOpenDb(dbName, upgradeCallback) {
+  return new Promise((resolve, reject) => {
+    const deleteRequest = indexedDB.deleteDatabase(dbName);
+    deleteRequest.onerror = () => {
+      reject(new Error('An error occurred on deleting database ${dbName}'));
+    };
+    deleteRequest.onsuccess = () => {
+      const openRequest = indexedDB.open(dbName);
+      openRequest.onerror = () => {
+        reject(new Error('An error occurred on opening database ${dbName}'));
+      };
+      openRequest.onblocked = () => {
+        reject(new Error('Opening database ${dbName} was blocked'));
+      };
+      openRequest.onupgradeneeded = () => {
+        upgradeCallback();
+      };
+      openRequest.onsuccess = () => {
+        resolve(event.target.result);
+      };
+    }
+  });
+}
+
+function promiseOpenDb(dbName, optionalUpgradeCallback) {
+  return new Promise((resolve, reject) => {
+    const openRequest = indexedDB.open(dbName);
+    openRequest.onerror = () => {
+      const e = new Error('Error opening database ${dbName}');
+      unexepectedErrorCallback(e);
+      reject(e);
+    };
+    openRequest.onblocked = () => {
+      const e = new Error('Opening database ${dbName}');
+      unexpectedBlockedCallback(e);
+      reject(e);
+    };
+    if (optionalUpgradeCallback) {
+      openRequest.onupgradeneeded = () => {
+        const db = event.target.result;
+        optionalUpgradeCallback(db);
+      };
+    }
+    openRequest.onsuccess = () => {
+      db = event.target.result;
+      resolve(db);
+    };
+  });
+}
+
+function keepAlive(transaction, storeName) {
+  let completed = false;
+  transaction.addEventListener('complete', () => { completed = true; });
+
+  let pin = true;
+
+  function spin() {
+    if (!pin)
+      return;
+    transaction.objectStore(storeName).get(0).onsuccess = spin;
+  }
+  spin();
+
+  return () => {
+    shouldBeFalse(completed);
+    pin = false;
+  };
+}
+
 if (typeof String.prototype.startsWith !== 'function') {
   String.prototype.startsWith = function (str) {
     return this.indexOf(str) === 0;

third_party/blink/web_tests/external/wpt/IndexedDB/keypath-special-identifiers.htm

@@ -54,6 +54,8 @@
         const result = request.result;
         assert_key_equals(result[testcase.property], key,
                           'Property should be used as key');
+      });
+      tx.oncomplete = t.step_func(function() {
         t.done();
       });
     },