indexeddb: Removed use of dangling ptr in writeBlobToFileOnIOThread.

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 9c220f24533b9ba1e3d92283916316cb30f465dd..6e5efdbd7c001cdea140c4d00a800713f09d70ce 100644
--- a/content/browser/indexed_db/indexed_db_backing_store.cc
+++ b/content/browser/indexed_db/indexed_db_backing_store.cc
@@ -2253,7 +2253,7 @@ class IndexedDBBackingStore::Transaction::ChainedBlobWriterImpl
 };
 
 class LocalWriteClosure : public FileWriterDelegate::DelegateWriteCallback,
-                          public base::RefCounted<LocalWriteClosure> {
+                          public base::RefCountedThreadSafe<LocalWriteClosure> {

[michaeln1, 2014/07/23 20:15:15]

yikes! good catch

  public:
   LocalWriteClosure(IndexedDBBackingStore::Transaction::ChainedBlobWriter*
                         chained_blob_writer,
@@ -2282,6 +2282,14 @@ class LocalWriteClosure : public FileWriterDelegate::DelegateWriteCallback,
                    write_status == FileWriterDelegate::SUCCESS_COMPLETED));
   }
 
+  static void ReleaseWriterOnIDBThread(
+      scoped_refptr<IndexedDBBackingStore::Transaction::ChainedBlobWriter>
+          chained_blob_writer) {
+    // Don't actually release the writer (the closure will do that). Merely
+    // posting the ref counted pointer over to this thread ensures that it is
+    // deleted on the correct thread.
+  }
+
   void writeBlobToFileOnIOThread(const FilePath& file_path,
                                  const GURL& blob_url,
                                  net::URLRequestContext* request_context) {
@@ -2304,15 +2312,23 @@ class LocalWriteClosure : public FileWriterDelegate::DelegateWriteCallback,
   }
 
  private:
-  virtual ~LocalWriteClosure() {}
-  friend class base::RefCounted<LocalWriteClosure>;
+  virtual ~LocalWriteClosure() {
+    // Make sure the ChainedBlobWriter is derefed (and deleted) on the IDB
+    // thread since it owns a transaction which has thread affinity.
+    task_runner_->PostTask(

[michaeln1, 2014/07/23 20:15:15]

probably should use ReleaseSoon here

as coded if the task runs prior to return from PostTask, i think the final
release of chained_blob_writer_ will happen on the current thread

+        FROM_HERE,
+        base::Bind(&LocalWriteClosure::ReleaseWriterOnIDBThread,
+                   chained_blob_writer_));
+  }
+  friend class base::RefCountedThreadSafe<LocalWriteClosure>;

[michaeln1, 2014/07/23 20:15:15]

style nit: i think we usually put friend decls prior to ctor/dtor/methods?

[jsbell, 2014/07/23 20:17:47]

I thought so to... but oddly enough it's not in the style guide, and content/
seems inconsistent about it. :(

Since we both think they should be first (just after private:) then +1 to moving
them up.

[cmumford, 2014/07/23 23:26:58]

Done.

 
   void callBlobCallbackOnIDBTaskRunner(bool succeeded) {

[michaeln1, 2014/07/23 20:15:15]

this intermediary may not be needed, at the callsite PostTask() directly to
ReportWriteCompletion() instead?

[cmumford, 2014/07/23 23:26:58]

You're correct - it's not necessary. It's unrelated, but a small change so I'll
do it in this CL.

     DCHECK(task_runner_->RunsTasksOnCurrentThread());
     chained_blob_writer_->ReportWriteCompletion(succeeded, bytes_written_);
   }
 
-  IndexedDBBackingStore::Transaction::ChainedBlobWriter* chained_blob_writer_;
+  scoped_refptr<IndexedDBBackingStore::Transaction::ChainedBlobWriter>
+      chained_blob_writer_;
   base::TaskRunner* task_runner_;
   int64 bytes_written_;