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

content/browser/indexed_db/indexed_db_backing_store.cc

Index: content/browser/indexed_db/indexed_db_backing_store.cc
diff --git a/content/browser/indexed_db/indexed_db_backing_store.cc b/content/browser/indexed_db/indexed_db_backing_store.cc
index e09458913785c85a1b5e963db68819147f48c3a4..71ad0448b3634420bf76c9ee89c14bc18b7ca2ec 100644
--- a/content/browser/indexed_db/indexed_db_backing_store.cc
+++ b/content/browser/indexed_db/indexed_db_backing_store.cc
@@ -4066,8 +4066,10 @@ IndexedDBBackingStore::OpenIndexCursor(
 
 IndexedDBBackingStore::Transaction::Transaction(
     IndexedDBBackingStore* backing_store)
-    : backing_store_(backing_store), database_id_(-1), committing_(false) {
-}
+    : backing_store_(backing_store),
+      database_id_(-1),
+      committing_(false),
+      ptr_factory_(this) {}
 
 IndexedDBBackingStore::Transaction::~Transaction() {
   DCHECK(!committing_);
@@ -4341,22 +4343,26 @@ leveldb::Status IndexedDBBackingStore::Transaction::CommitPhaseTwo() {
 class IndexedDBBackingStore::Transaction::BlobWriteCallbackWrapper
     : public IndexedDBBackingStore::BlobWriteCallback {
  public:
-  BlobWriteCallbackWrapper(IndexedDBBackingStore::Transaction* transaction,
-                           scoped_refptr<BlobWriteCallback> callback)
-      : transaction_(transaction), callback_(callback) {}
+  BlobWriteCallbackWrapper(
+      base::WeakPtr<IndexedDBBackingStore::Transaction> transaction,
+      scoped_refptr<BlobWriteCallback> callback)
+      : transaction_(std::move(transaction)),
+        transaction_ptr_(transaction_.get()),
+        callback_(callback) {}
   void Run(bool succeeded) override {
     IDB_ASYNC_TRACE_END("IndexedDBBackingStore::Transaction::WriteNewBlobs",
-                        transaction_);
+                        transaction_ptr_);
     callback_->Run(succeeded);
-    if (succeeded)  // Else it's already been deleted during rollback.
-      transaction_->chained_blob_writer_ = NULL;
+    if (transaction_ && succeeded)
+      transaction_->chained_blob_writer_ = nullptr;
   }
 
  private:
   ~BlobWriteCallbackWrapper() override {}
   friend class base::RefCounted<IndexedDBBackingStore::BlobWriteCallback>;
 
-  IndexedDBBackingStore::Transaction* transaction_;
+  base::WeakPtr<IndexedDBBackingStore::Transaction> transaction_;

[cmumford, 2016/11/04 23:33:12]

What about the other places in this file where transaction_ is used w/o checking
for null. If not required then adding a comment, and DCHECK, would help.

[dmurph, 2016/11/07 20:05:22]

I'm not quite sure what you mean here. In this specific case, we have a task
scheduled that's separate from the regular lifecycle, and since the Transaction*
is owned by the IDBTransaction, this can be destructed.

So it looks like we have a Transaction* in Cursor, is that what you mean? That
doesn't have weakptrs but I can add a factory. BUT we already guarentee we don't
touch cursor operations if the transaction is dead in the dispatcher host.

So what do you prefer here?

+  IndexedDBBackingStore::Transaction* transaction_ptr_;

[jsbell, 2016/11/04 17:48:19]

Nit: comment that this is only for ASYNC_TRACE macros.

(Can we make it void* to make it obvious this is not to be used for anything
real?)

[dmurph, 2016/11/04 22:52:24]

Done.

[cmumford, 2016/11/04 23:33:12]

Also, maybe make it:

  const IndexedDBBackingStore::Transaction* const transaction_ptr_;

To deter use by a future maintainer.

[dmurph, 2016/11/07 20:05:22]

Done.

   scoped_refptr<BlobWriteCallback> callback_;
 
   DISALLOW_COPY_AND_ASSIGN(BlobWriteCallbackWrapper);
@@ -4379,12 +4385,11 @@ void IndexedDBBackingStore::Transaction::WriteNewBlobs(
       transaction_->Put(blob_entry_iter.first.Encode(),
                         &blob_entry_iter.second);
   }
-  // Creating the writer will start it going asynchronously.
-  chained_blob_writer_ =
-      new ChainedBlobWriterImpl(database_id_,
-                                backing_store_,
-                                new_files_to_write,
-                                new BlobWriteCallbackWrapper(this, callback));
+  // Creating the writer will start it going asynchronously. The transaction
+  // can be destructed before the callback is triggered.
+  chained_blob_writer_ = new ChainedBlobWriterImpl(
+      database_id_, backing_store_, new_files_to_write,
+      new BlobWriteCallbackWrapper(ptr_factory_.GetWeakPtr(), callback));
 }
 
 void IndexedDBBackingStore::Transaction::Rollback() {

[cmumford, 2016/11/04 23:33:12]

Not your change, but on line 4407 you can simplify:

  if (transaction_.get() == NULL)

to:

  if (!transaction_)

[dmurph, 2016/11/07 20:05:22]

Done.