[IndexedDB] Refactoring to remove ref ptrs and host transaction ids.

content/browser/indexed_db/indexed_db_transaction.cc

Index: content/browser/indexed_db/indexed_db_transaction.cc
diff --git a/content/browser/indexed_db/indexed_db_transaction.cc b/content/browser/indexed_db/indexed_db_transaction.cc
index 40de131f4f3045af60cc4d11f519ba1286d0fb93..1eb92cf9bdf4434bde3c810bd0fe9c31d7e92f47 100644
--- a/content/browser/indexed_db/indexed_db_transaction.cc
+++ b/content/browser/indexed_db/indexed_db_transaction.cc
@@ -30,7 +30,9 @@ const int64_t kInactivityTimeoutPeriodSeconds = 60;
 
 // Helper for posting a task to call IndexedDBTransaction::Commit when we know
 // the transaction had no requests and therefore the commit must succeed.
-void CommitUnused(scoped_refptr<IndexedDBTransaction> transaction) {
+void CommitUnused(base::WeakPtr<IndexedDBTransaction> transaction) {
+  if (!transaction)
+    return;
   leveldb::Status status = transaction->Commit();
   DCHECK(status.ok());
 }
@@ -85,7 +87,7 @@ IndexedDBTransaction::AbortOperation IndexedDBTransaction::TaskStack::pop() {
 
 IndexedDBTransaction::IndexedDBTransaction(
     int64_t id,
-    base::WeakPtr<IndexedDBConnection> connection,
+    IndexedDBConnection* connection,
     const std::set<int64_t>& object_store_ids,
     blink::WebIDBTransactionMode mode,
     IndexedDBBackingStore::Transaction* backing_store_transaction)
@@ -93,12 +95,11 @@ IndexedDBTransaction::IndexedDBTransaction(
       object_store_ids_(object_store_ids),
       mode_(mode),
       connection_(std::move(connection)),
-      transaction_(backing_store_transaction) {
+      transaction_(backing_store_transaction),
+      ptr_factory_(this) {
   callbacks_ = connection_->callbacks();
   database_ = connection_->database();
 
-  database_->transaction_coordinator().DidCreateTransaction(this);
-
   diagnostics_.tasks_scheduled = 0;
   diagnostics_.tasks_completed = 0;
   diagnostics_.creation_time = base::Time::Now();
@@ -112,7 +113,6 @@ IndexedDBTransaction::~IndexedDBTransaction() {
   DCHECK_EQ(pending_preemptive_events_, 0);
   DCHECK(task_queue_.empty());
   DCHECK(abort_task_stack_.empty());
-  DCHECK(pending_observers_.empty());
   DCHECK(!processing_event_queue_);
 }
 
@@ -152,7 +152,8 @@ void IndexedDBTransaction::RunTasksIfStarted() {
 
   should_process_queue_ = true;
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::Bind(&IndexedDBTransaction::ProcessTaskQueue, this));
+      FROM_HERE, base::Bind(&IndexedDBTransaction::ProcessTaskQueue,
+                            ptr_factory_.GetWeakPtr()));
 }
 
 void IndexedDBTransaction::Abort() {
@@ -162,14 +163,10 @@ void IndexedDBTransaction::Abort() {
 
 void IndexedDBTransaction::Abort(const IndexedDBDatabaseError& error) {
   IDB_TRACE1("IndexedDBTransaction::Abort", "txn.id", id());
+  DCHECK(!processing_event_queue_);
   if (state_ == FINISHED)
     return;
 
-  // 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.
-  scoped_refptr<IndexedDBTransaction> protect(this);
-
   timeout_timer_.Stop();
 
   state_ = FINISHED;
@@ -202,13 +199,12 @@ void IndexedDBTransaction::Abort(const IndexedDBDatabaseError& error) {
 #endif
 
   if (callbacks_.get())
-    callbacks_->OnAbort(id_, error);
+    callbacks_->OnAbort(*this, error);
 
   database_->TransactionFinished(this, false);
 
-  database_ = NULL;
-  connection_ = nullptr;
-  pending_observers_.clear();
+  // EraseTransaction will delete |this|.
+  connection_->RemoveTransaction(id_);
 }
 
 bool IndexedDBTransaction::IsTaskQueueEmpty() const {
@@ -239,7 +235,7 @@ void IndexedDBTransaction::Start() {
       // front-end previously requested a commit; do the commit now, but not
       // re-entrantly as that may renter the coordinator.
       base::ThreadTaskRunnerHandle::Get()->PostTask(
-          FROM_HERE, base::Bind(&CommitUnused, make_scoped_refptr(this)));
+          FROM_HERE, base::Bind(&CommitUnused, ptr_factory_.GetWeakPtr()));
     }
     return;
   }
@@ -250,35 +246,49 @@ void IndexedDBTransaction::Start() {
 class BlobWriteCallbackImpl : public IndexedDBBackingStore::BlobWriteCallback {
  public:
   explicit BlobWriteCallbackImpl(
-      scoped_refptr<IndexedDBTransaction> transaction)
-      : transaction_(transaction) {}
-  void Run(bool succeeded) override {
-    transaction_->BlobWriteComplete(succeeded);
+      base::WeakPtr<IndexedDBTransaction> transaction)
+      : transaction_(std::move(transaction)) {}
+
+  leveldb::Status Run(IndexedDBBackingStore::BlobWriteResult result) override {
+    if (!transaction_) return leveldb::Status::OK();
+    return transaction_->BlobWriteComplete(result);
   }
 
  protected:
   ~BlobWriteCallbackImpl() override {}
 
  private:
-  scoped_refptr<IndexedDBTransaction> transaction_;
+  base::WeakPtr<IndexedDBTransaction> transaction_;
 };
 
-void IndexedDBTransaction::BlobWriteComplete(bool success) {
+leveldb::Status IndexedDBTransaction::BlobWriteComplete(
+    IndexedDBBackingStore::BlobWriteResult result) {
   IDB_TRACE("IndexedDBTransaction::BlobWriteComplete");
   if (state_ == FINISHED)  // aborted
-    return;
+    return leveldb::Status::OK();
   DCHECK_EQ(state_, COMMITTING);
 
-  if (!success) {
-    Abort(IndexedDBDatabaseError(blink::WebIDBDatabaseExceptionDataError,
-                                 "Failed to write blobs."));
-    return;
+  leveldb::Status s = leveldb::Status::OK();
+  // Switch statement to protect against adding new enum values.
+  switch (result) {
+    case IndexedDBBackingStore::BlobWriteResult::FAIILURE_ASYNC:
+      Abort(IndexedDBDatabaseError(blink::WebIDBDatabaseExceptionDataError,
+                                   "Failed to write blobs."));
+      return leveldb::Status::OK();
+    case IndexedDBBackingStore::BlobWriteResult::SUCCESS_ASYNC:
+    case IndexedDBBackingStore::BlobWriteResult::SUCCESS_SYNC: {
+      // Save the database as |this| can be destroyed in the next line. We also
+      // make
+      // sure to handle the error if we're not being called synchronously.
+      scoped_refptr<IndexedDBDatabase> database = database_;
+      s = CommitPhaseTwo();
+      if (!s.ok() &&
+          result == IndexedDBBackingStore::BlobWriteResult::SUCCESS_ASYNC)
+        ReportError(s, database, database->factory());
+      break;
+    }
   }
-  // Save the database as |this| can be destroyed in the next line.
-  scoped_refptr<IndexedDBDatabase> database = database_;
-  leveldb::Status s = CommitPhaseTwo();
-  if (!s.ok())
-    ReportError(s, database, database->factory());
+  return s;
 }
 
 leveldb::Status IndexedDBTransaction::Commit() {
@@ -315,13 +325,10 @@ leveldb::Status IndexedDBTransaction::Commit() {
     s = CommitPhaseTwo();
   } else {
     scoped_refptr<IndexedDBBackingStore::BlobWriteCallback> callback(
-        new BlobWriteCallbackImpl(this));
+        new BlobWriteCallbackImpl(ptr_factory_.GetWeakPtr()));
     // CommitPhaseOne will call the callback synchronously if there are no blobs
     // to write.
     s = transaction_->CommitPhaseOne(callback);
-    if (!s.ok())
-      Abort(IndexedDBDatabaseError(blink::WebIDBDatabaseExceptionDataError,
-                                   "Error processing blob journal."));
   }
 
   return s;
@@ -334,11 +341,6 @@ leveldb::Status IndexedDBTransaction::CommitPhaseTwo() {
 
   DCHECK_EQ(state_, COMMITTING);
 
-  // The last reference to this object may be released while performing the
-  // commit steps below. We therefore take a self reference to keep ourselves
-  // alive while executing this method.
-  scoped_refptr<IndexedDBTransaction> protect(this);
-
   state_ = FINISHED;
 
   leveldb::Status s;
@@ -375,7 +377,7 @@ leveldb::Status IndexedDBTransaction::CommitPhaseTwo() {
       IDB_TRACE1(
           "IndexedDBTransaction::CommitPhaseTwo.TransactionCompleteCallbacks",
           "txn.id", id());
-      callbacks_->OnComplete(id_);
+      callbacks_->OnComplete(*this);
     }
     if (!pending_observers_.empty() && connection_) {
       connection_->ActivatePendingObservers(std::move(pending_observers_));
@@ -383,6 +385,9 @@ leveldb::Status IndexedDBTransaction::CommitPhaseTwo() {
     }
 
     database_->TransactionFinished(this, true);
+    // EraseTransaction will delete |this|.

[cmumford, 2016/12/01 19:14:51]

s/EraseTransaction/RemoveTransaction/

down below too.

[dmurph, 2016/12/01 21:12:23]

Done.

+    connection_->RemoveTransaction(id_);
+    return s;
   } else {
     while (!abort_task_stack_.empty())
       abort_task_stack_.pop().Run();
@@ -396,12 +401,9 @@ leveldb::Status IndexedDBTransaction::CommitPhaseTwo() {
       error = IndexedDBDatabaseError(blink::WebIDBDatabaseExceptionUnknownError,
                                      "Internal error committing transaction.");
     }
-    callbacks_->OnAbort(id_, error);
-
+    callbacks_->OnAbort(*this, error);
     database_->TransactionFinished(this, false);
   }
-
-  database_ = NULL;
   return s;
 }
 
@@ -424,11 +426,6 @@ void IndexedDBTransaction::ProcessTaskQueue() {
     backing_store_transaction_begun_ = true;
   }
 
-  // The last reference to this object may be released while performing the
-  // tasks. Take take a self reference to keep this object alive so that
-  // the loop termination conditions can be checked.
-  scoped_refptr<IndexedDBTransaction> protect(this);
-
   TaskQueue* task_queue =
       pending_preemptive_events_ ? &preemptive_task_queue_ : &task_queue_;
   while (!task_queue->empty() && state_ != FINISHED) {
@@ -441,8 +438,7 @@ void IndexedDBTransaction::ProcessTaskQueue() {
     }
     if (!result.ok()) {
       processing_event_queue_ = false;
-      if (!result.ok())
-        ReportError(result, database_, database_->factory());
+      ReportError(result, database_, database_->factory());
       return;
     }
 
@@ -454,8 +450,9 @@ 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_) {
-    leveldb::Status result = Commit();
     processing_event_queue_ = false;
+    // This can delete |this|.
+    leveldb::Status result = Commit();
     if (!result.ok())
       ReportError(result, database_, database_->factory());
     return;
@@ -473,8 +470,9 @@ void IndexedDBTransaction::ProcessTaskQueue() {
   // never requests further activity. Read-only transactions don't
   // block other transactions, so don't time those out.
   if (mode_ != blink::WebIDBTransactionModeReadOnly) {
-    timeout_timer_.Start(FROM_HERE, GetInactivityTimeout(),
-                         base::Bind(&IndexedDBTransaction::Timeout, this));
+    timeout_timer_.Start(
+        FROM_HERE, GetInactivityTimeout(),
+        base::Bind(&IndexedDBTransaction::Timeout, ptr_factory_.GetWeakPtr()));
   }
   processing_event_queue_ = false;
 }