IndexedDB: Correct transaction start scheduling to match spec

Per spec, exclusive transactions should execute in creation order, not
first-use order. This lets the state machine for transactions to be
simplified.

BUG=322238

Review URL: https://codereview.chromium.org/82573006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@237258 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 84 insertions, 110 deletions

diff --git a/content/browser/indexed_db/indexed_db_context_impl.cc b/content/browser/indexed_db/indexed_db_context_impl.cc
index dbf7aca..ed7db6e 100644
--- a/content/browser/indexed_db/indexed_db_context_impl.cc
+++ b/content/browser/indexed_db/indexed_db_context_impl.cc
@@ -219,19 +219,19 @@ ListValue* IndexedDBContextImpl::GetAllOriginsDetails() {
 
           const char* kModes[] = { "readonly", "readwrite", "versionchange" };
           transaction_info->SetString("mode", kModes[transaction->mode()]);
-          switch (transaction->queue_status()) {
+          switch (transaction->state()) {
             case IndexedDBTransaction::CREATED:
-              transaction_info->SetString("status", "created");
-              break;
-            case IndexedDBTransaction::BLOCKED:
               transaction_info->SetString("status", "blocked");
               break;
-            case IndexedDBTransaction::UNBLOCKED:
-              if (transaction->IsRunning())
+            case IndexedDBTransaction::STARTED:
+              if (transaction->tasks_scheduled() > 0)
                 transaction_info->SetString("status", "running");
               else
                 transaction_info->SetString("status", "started");
               break;
+            case IndexedDBTransaction::FINISHED:
+              transaction_info->SetString("status", "finished");
+              break;
           }
 
           transaction_info->SetDouble(
diff --git a/content/browser/indexed_db/indexed_db_transaction.cc b/content/browser/indexed_db/indexed_db_transaction.cc
index 8566a38..4ac8795 100644
--- a/content/browser/indexed_db/indexed_db_transaction.cc
+++ b/content/browser/indexed_db/indexed_db_transaction.cc
@@ -57,14 +57,15 @@ IndexedDBTransaction::IndexedDBTransaction(
     : id_(id),
       object_store_ids_(object_store_ids),
       mode_(mode),
-      state_(UNUSED),
+      used_(false),
+      state_(CREATED),
       commit_pending_(false),
       callbacks_(callbacks),
       database_(database),
       transaction_(database->backing_store()),
+      backing_store_transaction_begun_(false),
       should_process_queue_(false),
       pending_preemptive_events_(0),
-      queue_status_(CREATED),
       creation_time_(base::Time::Now()),
       tasks_scheduled_(0),
       tasks_completed_(0) {
@@ -83,10 +84,12 @@ IndexedDBTransaction::~IndexedDBTransaction() {
 void IndexedDBTransaction::ScheduleTask(Operation task, Operation abort_task) {
   if (state_ == FINISHED)
     return;
+
+  used_ = true;
   task_queue_.push(task);
   ++tasks_scheduled_;
   abort_task_stack_.push(abort_task);
-  EnsureTasksRunning();
+  RunTasksIfStarted();
 }
 
 void IndexedDBTransaction::ScheduleTask(IndexedDBDatabase::TaskType type,
@@ -94,23 +97,30 @@ void IndexedDBTransaction::ScheduleTask(IndexedDBDatabase::TaskType type,
   if (state_ == FINISHED)
     return;
 
+  used_ = true;
   if (type == IndexedDBDatabase::NORMAL_TASK) {
     task_queue_.push(task);
     ++tasks_scheduled_;
   } else {
     preemptive_task_queue_.push(task);
   }
-  EnsureTasksRunning();
+  RunTasksIfStarted();
 }
 
-void IndexedDBTransaction::EnsureTasksRunning() {
-  if (state_ == UNUSED) {
-    Start();
-  } else if (state_ == RUNNING && !should_process_queue_) {
-    should_process_queue_ = true;
-    base::MessageLoop::current()->PostTask(
-        FROM_HERE, base::Bind(&IndexedDBTransaction::ProcessTaskQueue, this));
-  }
+void IndexedDBTransaction::RunTasksIfStarted() {
+  DCHECK(used_);
+
+  // Not started by the coordinator yet.
+  if (state_ != STARTED)
+    return;
+
+  // A task is already posted.
+  if (should_process_queue_)
+    return;
+
+  should_process_queue_ = true;
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE, base::Bind(&IndexedDBTransaction::ProcessTaskQueue, this));
 }
 
 void IndexedDBTransaction::Abort() {
@@ -123,8 +133,6 @@ void IndexedDBTransaction::Abort(const IndexedDBDatabaseError& error) {
   if (state_ == FINISHED)
     return;
 
-  bool was_running = state_ == RUNNING;
-
   // The last reference to this object may be released while performing the
   // abort steps below. We therefore take a self reference to keep ourselves
   // alive while executing this method.
@@ -133,7 +141,7 @@ void IndexedDBTransaction::Abort(const IndexedDBDatabaseError& error) {
   state_ = FINISHED;
   should_process_queue_ = false;
 
-  if (was_running)
+  if (backing_store_transaction_begun_)
     transaction_.Rollback();
 
   // Run the abort tasks, if any.
@@ -184,23 +192,17 @@ void IndexedDBTransaction::UnregisterOpenCursor(IndexedDBCursor* cursor) {
   open_cursors_.erase(cursor);
 }
 
-void IndexedDBTransaction::Run() {
+void IndexedDBTransaction::Start() {
   // TransactionCoordinator has started this transaction.
-  DCHECK(state_ == START_PENDING || state_ == RUNNING);
-  DCHECK(!should_process_queue_);
-
+  DCHECK_EQ(CREATED, state_);
+  state_ = STARTED;
+  database_->TransactionStarted(this);
   start_time_ = base::Time::Now();
-  should_process_queue_ = true;
-  base::MessageLoop::current()->PostTask(
-      FROM_HERE, base::Bind(&IndexedDBTransaction::ProcessTaskQueue, this));
-}
 
-void IndexedDBTransaction::Start() {
-  DCHECK_EQ(state_, UNUSED);
+  if (!used_)
+    return;
 
-  state_ = START_PENDING;
-  database_->transaction_coordinator().DidStartTransaction(this);
-  database_->TransactionStarted(this);
+  RunTasksIfStarted();
 }
 
 void IndexedDBTransaction::Commit() {
@@ -212,7 +214,7 @@ void IndexedDBTransaction::Commit() {
   if (state_ == FINISHED)
     return;
 
-  DCHECK(state_ == UNUSED || state_ == RUNNING);
+  DCHECK(!used_ || state_ == STARTED);
   commit_pending_ = true;
 
   // Front-end has requested a commit, but there may be tasks like
@@ -229,10 +231,9 @@ void IndexedDBTransaction::Commit() {
   // TODO(jsbell): Run abort tasks if commit fails? http://crbug.com/241843
   abort_task_stack_.clear();
 
-  bool unused = state_ == UNUSED;
   state_ = FINISHED;
 
-  bool committed = unused || transaction_.Commit();
+  bool committed = !used_ || transaction_.Commit();
 
   // Backing store resources (held via cursors) must be released
   // before script callbacks are fired, as the script callbacks may
@@ -272,9 +273,9 @@ void IndexedDBTransaction::ProcessTaskQueue() {
   DCHECK(!IsTaskQueueEmpty());
   should_process_queue_ = false;
 
-  if (state_ == START_PENDING) {
+  if (!backing_store_transaction_begun_) {
     transaction_.Begin();
-    state_ = RUNNING;
+    backing_store_transaction_begun_ = true;
   }
 
   // The last reference to this object may be released while performing the
@@ -285,7 +286,7 @@ void IndexedDBTransaction::ProcessTaskQueue() {
   TaskQueue* task_queue =
       pending_preemptive_events_ ? &preemptive_task_queue_ : &task_queue_;
   while (!task_queue->empty() && state_ != FINISHED) {
-    DCHECK_EQ(state_, RUNNING);
+    DCHECK_EQ(STARTED, state_);
     Operation task(task_queue->pop());
     task.Run(this);
     if (!pending_preemptive_events_) {
diff --git a/content/browser/indexed_db/indexed_db_transaction.h b/content/browser/indexed_db/indexed_db_transaction.h
index 24f4449..5e4bf0f 100644
--- a/content/browser/indexed_db/indexed_db_transaction.h
+++ b/content/browser/indexed_db/indexed_db_transaction.h
@@ -34,15 +34,17 @@ class IndexedDBTransaction : public base::RefCounted<IndexedDBTransaction> {
 
   virtual void Abort();
   void Commit();
-
   void Abort(const IndexedDBDatabaseError& error);
-  void Run();
+
+  // Called by the transaction coordinator when this transaction is unblocked.
+  void Start();
+
   indexed_db::TransactionMode mode() const { return mode_; }
   const std::set<int64>& scope() const { return object_store_ids_; }
+
   void ScheduleTask(Operation task) {
     ScheduleTask(IndexedDBDatabase::NORMAL_TASK, task);
   }
-
   void ScheduleTask(Operation task, Operation abort_task);
   void ScheduleTask(IndexedDBDatabase::TaskType, Operation task);
   void RegisterOpenCursor(IndexedDBCursor* cursor);
@@ -59,17 +61,15 @@ class IndexedDBTransaction : public base::RefCounted<IndexedDBTransaction> {
 
   IndexedDBDatabase* database() const { return database_; }
   IndexedDBDatabaseCallbacks* connection() const { return callbacks_; }
-  bool IsRunning() const { return state_ == RUNNING; }
 
-  // The following types/accessors are for diagnostics only.
-  enum QueueStatus {
-    CREATED,
-    BLOCKED,
-    UNBLOCKED,
+  enum State {
+    CREATED,   // Created, but not yet started by coordinator.
+    STARTED,   // Started by the coordinator.
+    FINISHED,  // Either aborted or committed.
   };
 
-  QueueStatus queue_status() const { return queue_status_; }
-  void set_queue_status(QueueStatus status) { queue_status_ = status; }
+  // These are for diagnostic purposes only.
+  State state() const { return state_; }
   base::Time creation_time() const { return creation_time_; }
   base::Time start_time() const { return start_time_; }
   int tasks_scheduled() const { return tasks_scheduled_; }
@@ -80,16 +80,7 @@ class IndexedDBTransaction : public base::RefCounted<IndexedDBTransaction> {
   friend class base::RefCounted<IndexedDBTransaction>;
 
  private:
-  enum State {
-    UNUSED,         // Created, but no tasks yet.
-    START_PENDING,  // Enqueued tasks, but backing store transaction not yet
-                    // started.
-    RUNNING,        // Backing store transaction started but not yet finished.
-    FINISHED,       // Either aborted or committed.
-  };
-
-  void EnsureTasksRunning();
-  void Start();
+  void RunTasksIfStarted();
 
   bool IsTaskQueueEmpty() const;
   bool HasPendingTasks() const;
@@ -101,6 +92,7 @@ class IndexedDBTransaction : public base::RefCounted<IndexedDBTransaction> {
   const std::set<int64> object_store_ids_;
   const indexed_db::TransactionMode mode_;
 
+  bool used_;
   State state_;
   bool commit_pending_;
   scoped_refptr<IndexedDBDatabaseCallbacks> callbacks_;
@@ -137,6 +129,7 @@ class IndexedDBTransaction : public base::RefCounted<IndexedDBTransaction> {
   TaskStack abort_task_stack_;
 
   IndexedDBBackingStore::Transaction transaction_;
+  bool backing_store_transaction_begun_;
 
   bool should_process_queue_;
   int pending_preemptive_events_;
@@ -144,7 +137,6 @@ class IndexedDBTransaction : public base::RefCounted<IndexedDBTransaction> {
   std::set<IndexedDBCursor*> open_cursors_;
 
   // The following members are for diagnostics only.
-  QueueStatus queue_status_;
   base::Time creation_time_;
   base::Time start_time_;
   int tasks_scheduled_;
diff --git a/content/browser/indexed_db/indexed_db_transaction_coordinator.cc b/content/browser/indexed_db/indexed_db_transaction_coordinator.cc
index 56a7c99..be47624 100644
--- a/content/browser/indexed_db/indexed_db_transaction_coordinator.cc
+++ b/content/browser/indexed_db/indexed_db_transaction_coordinator.cc
@@ -13,40 +13,31 @@ namespace content {
 IndexedDBTransactionCoordinator::IndexedDBTransactionCoordinator() {}
 
 IndexedDBTransactionCoordinator::~IndexedDBTransactionCoordinator() {
-  DCHECK(!transactions_.size());
   DCHECK(!queued_transactions_.size());
   DCHECK(!started_transactions_.size());
 }
 
 void IndexedDBTransactionCoordinator::DidCreateTransaction(
-    IndexedDBTransaction* transaction) {
-  DCHECK(transactions_.find(transaction) == transactions_.end());
-  DCHECK(transaction->queue_status() == IndexedDBTransaction::CREATED);
-  transactions_[transaction] = transaction;
-}
-
-void IndexedDBTransactionCoordinator::DidStartTransaction(
-    IndexedDBTransaction* transaction) {
-  DCHECK(transactions_.find(transaction) != transactions_.end());
+    scoped_refptr<IndexedDBTransaction> transaction) {
+  DCHECK(!queued_transactions_.count(transaction));
+  DCHECK(!started_transactions_.count(transaction));
+  DCHECK_EQ(IndexedDBTransaction::CREATED, transaction->state());
 
   queued_transactions_.insert(transaction);
-  ProcessStartedTransactions();
+  ProcessQueuedTransactions();
 }
 
 void IndexedDBTransactionCoordinator::DidFinishTransaction(
-    IndexedDBTransaction* transaction) {
-  DCHECK(transactions_.find(transaction) != transactions_.end());
-
+    scoped_refptr<IndexedDBTransaction> transaction) {
   if (queued_transactions_.count(transaction)) {
     DCHECK(!started_transactions_.count(transaction));
     queued_transactions_.erase(transaction);
   } else {
-    if (started_transactions_.count(transaction))
-      started_transactions_.erase(transaction);
+    DCHECK(started_transactions_.count(transaction));
+    started_transactions_.erase(transaction);
   }
-  transactions_.erase(transaction);
 
-  ProcessStartedTransactions();
+  ProcessQueuedTransactions();
 }
 
 #ifndef NDEBUG
@@ -60,7 +51,6 @@ bool IndexedDBTransactionCoordinator::IsActive(
     DCHECK(!found);
     found = true;
   }
-  DCHECK_EQ(found, (transactions_.find(transaction) != transactions_.end()));
   return found;
 }
 #endif
@@ -69,14 +59,12 @@ std::vector<const IndexedDBTransaction*>
 IndexedDBTransactionCoordinator::GetTransactions() const {
   std::vector<const IndexedDBTransaction*> result;
 
-  for (list_set<IndexedDBTransaction*>::const_iterator it =
-           started_transactions_.begin();
+  for (TransactionSet::const_iterator it = started_transactions_.begin();
        it != started_transactions_.end();
        ++it) {
     result.push_back(*it);
   }
-  for (list_set<IndexedDBTransaction*>::const_iterator it =
-           queued_transactions_.begin();
+  for (TransactionSet::const_iterator it = queued_transactions_.begin();
        it != queued_transactions_.end();
        ++it) {
     result.push_back(*it);
@@ -85,7 +73,7 @@ IndexedDBTransactionCoordinator::GetTransactions() const {
   return result;
 }
 
-void IndexedDBTransactionCoordinator::ProcessStartedTransactions() {
+void IndexedDBTransactionCoordinator::ProcessQueuedTransactions() {
   if (queued_transactions_.empty())
     return;
 
@@ -100,31 +88,28 @@ void IndexedDBTransactionCoordinator::ProcessStartedTransactions() {
   // data. ("Version change" transactions are exclusive, but handled by the
   // connection sequencing in IndexedDBDatabase.)
   std::set<int64> locked_scope;
-  for (list_set<IndexedDBTransaction*>::const_iterator it =
-           started_transactions_.begin();
+  for (TransactionSet::const_iterator it = started_transactions_.begin();
        it != started_transactions_.end();
        ++it) {
     IndexedDBTransaction* transaction = *it;
     if (transaction->mode() == indexed_db::TRANSACTION_READ_WRITE) {
-      // Running read/write transactions have exclusive access to the object
+      // Started read/write transactions have exclusive access to the object
       // stores within their scopes.
       locked_scope.insert(transaction->scope().begin(),
                           transaction->scope().end());
     }
   }
 
-  list_set<IndexedDBTransaction*>::const_iterator it =
-      queued_transactions_.begin();
+  TransactionSet::const_iterator it = queued_transactions_.begin();
   while (it != queued_transactions_.end()) {
-    IndexedDBTransaction* transaction = *it;
+    scoped_refptr<IndexedDBTransaction> transaction = *it;
     ++it;
-    if (CanRunTransaction(transaction, locked_scope)) {
-      transaction->set_queue_status(IndexedDBTransaction::UNBLOCKED);
+    if (CanStartTransaction(transaction, locked_scope)) {
+      DCHECK_EQ(IndexedDBTransaction::CREATED, transaction->state());
       queued_transactions_.erase(transaction);
       started_transactions_.insert(transaction);
-      transaction->Run();
-    } else {
-      transaction->set_queue_status(IndexedDBTransaction::BLOCKED);
+      transaction->Start();
+      DCHECK_EQ(IndexedDBTransaction::STARTED, transaction->state());
     }
     if (transaction->mode() == indexed_db::TRANSACTION_READ_WRITE) {
       // Either the transaction started, so it has exclusive access to the
@@ -152,7 +137,7 @@ static bool DoSetsIntersect(const std::set<T>& set1,
   return false;
 }
 
-bool IndexedDBTransactionCoordinator::CanRunTransaction(
+bool IndexedDBTransactionCoordinator::CanStartTransaction(
     IndexedDBTransaction* const transaction,
     const std::set<int64>& locked_scope) const {
   DCHECK(queued_transactions_.count(transaction));
diff --git a/content/browser/indexed_db/indexed_db_transaction_coordinator.h b/content/browser/indexed_db/indexed_db_transaction_coordinator.h
index e123a4e..a5d543c 100644
--- a/content/browser/indexed_db/indexed_db_transaction_coordinator.h
+++ b/content/browser/indexed_db/indexed_db_transaction_coordinator.h
@@ -24,9 +24,8 @@ class IndexedDBTransactionCoordinator {
   ~IndexedDBTransactionCoordinator();
 
   // Called by transactions as they start and finish.
-  void DidCreateTransaction(IndexedDBTransaction* transaction);
-  void DidStartTransaction(IndexedDBTransaction* transaction);
-  void DidFinishTransaction(IndexedDBTransaction* transaction);
+  void DidCreateTransaction(scoped_refptr<IndexedDBTransaction> transaction);
+  void DidFinishTransaction(scoped_refptr<IndexedDBTransaction> transaction);
 
 #ifndef NDEBUG
   bool IsActive(IndexedDBTransaction* transaction);
@@ -36,19 +35,16 @@ class IndexedDBTransactionCoordinator {
   std::vector<const IndexedDBTransaction*> GetTransactions() const;
 
  private:
-  void ProcessStartedTransactions();
-  bool CanRunTransaction(IndexedDBTransaction* const transaction,
-                         const std::set<int64>& locked_scope) const;
-
-  // This is just an efficient way to keep references to all transactions.
-  std::map<IndexedDBTransaction*, scoped_refptr<IndexedDBTransaction> >
-      transactions_;
+  void ProcessQueuedTransactions();
+  bool CanStartTransaction(IndexedDBTransaction* const transaction,
+                           const std::set<int64>& locked_scope) const;
 
   // Transactions in different states are grouped below.
   // list_set is used to provide stable ordering; required by spec
   // for the queue, convenience for diagnostics for the rest.
-  list_set<IndexedDBTransaction*> queued_transactions_;
-  list_set<IndexedDBTransaction*> started_transactions_;
+  typedef list_set<scoped_refptr<IndexedDBTransaction> > TransactionSet;
+  TransactionSet queued_transactions_;
+  TransactionSet started_transactions_;
 };
 
 }  // namespace content