indexeddb: Removed use of dangling ptr in writeBlobToFileOnIOThread.

content/browser/indexed_db/indexed_db_backing_store.cc

 // Copyright (c) 2013 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.
 
 #include "content/browser/indexed_db/indexed_db_backing_store.h"
 
 #include "base/file_util.h"
 #include "base/files/file_path.h"
 #include "base/format_macros.h"
 #include "base/json/json_reader.h"
@@ skipping 690 matching lines @@
   return true;
 }
 
 IndexedDBBackingStore::IndexedDBBackingStore(
     IndexedDBFactory* indexed_db_factory,
     const GURL& origin_url,
     const base::FilePath& blob_path,
     net::URLRequestContext* request_context,
     scoped_ptr<LevelDBDatabase> db,
     scoped_ptr<LevelDBComparator> comparator,
-    base::TaskRunner* task_runner)
+    base::SequencedTaskRunner* task_runner)
     : indexed_db_factory_(indexed_db_factory),
       origin_url_(origin_url),
       blob_path_(blob_path),
       origin_identifier_(ComputeOriginIdentifier(origin_url)),
       request_context_(request_context),
       task_runner_(task_runner),
       db_(db.Pass()),
       comparator_(comparator.Pass()),
       active_blob_registry_(this) {
 }
@@ skipping 51 matching lines @@
 
 // static
 scoped_refptr<IndexedDBBackingStore> IndexedDBBackingStore::Open(
     IndexedDBFactory* indexed_db_factory,
     const GURL& origin_url,
     const base::FilePath& path_base,
     net::URLRequestContext* request_context,
     blink::WebIDBDataLoss* data_loss,
     std::string* data_loss_message,
     bool* disk_full,
-    base::TaskRunner* task_runner,
+    base::SequencedTaskRunner* task_runner,
     bool clean_journal,
     leveldb::Status* status) {
   *data_loss = blink::WebIDBDataLossNone;
   DefaultLevelDBFactory leveldb_factory;
   return IndexedDBBackingStore::Open(indexed_db_factory,
                                      origin_url,
                                      path_base,
                                      request_context,
                                      data_loss,
                                      data_loss_message,
@@ skipping 134 matching lines @@
 // static
 scoped_refptr<IndexedDBBackingStore> IndexedDBBackingStore::Open(
     IndexedDBFactory* indexed_db_factory,
     const GURL& origin_url,
     const base::FilePath& path_base,
     net::URLRequestContext* request_context,
     blink::WebIDBDataLoss* data_loss,
     std::string* data_loss_message,
     bool* is_disk_full,
     LevelDBFactory* leveldb_factory,
-    base::TaskRunner* task_runner,
+    base::SequencedTaskRunner* task_runner,
     bool clean_journal,
     leveldb::Status* status) {
   IDB_TRACE("IndexedDBBackingStore::Open");
   DCHECK(!path_base.empty());
   *data_loss = blink::WebIDBDataLossNone;
   *data_loss_message = "";
   *is_disk_full = false;
 
   *status = leveldb::Status::OK();
 
@@ skipping 126 matching lines @@
     HistogramOpenStatus(
         INDEXED_DB_BACKING_STORE_OPEN_FAILED_CLEANUP_JOURNAL_ERROR, origin_url);
     return scoped_refptr<IndexedDBBackingStore>();
   }
   return backing_store;
 }
 
 // static
 scoped_refptr<IndexedDBBackingStore> IndexedDBBackingStore::OpenInMemory(
     const GURL& origin_url,
-    base::TaskRunner* task_runner,
+    base::SequencedTaskRunner* task_runner,
     leveldb::Status* status) {
   DefaultLevelDBFactory leveldb_factory;
   return IndexedDBBackingStore::OpenInMemory(
       origin_url, &leveldb_factory, task_runner, status);
 }
 
 // static
 scoped_refptr<IndexedDBBackingStore> IndexedDBBackingStore::OpenInMemory(
     const GURL& origin_url,
     LevelDBFactory* leveldb_factory,
-    base::TaskRunner* task_runner,
+    base::SequencedTaskRunner* task_runner,
     leveldb::Status* status) {
   IDB_TRACE("IndexedDBBackingStore::OpenInMemory");
 
   scoped_ptr<LevelDBComparator> comparator(new Comparator());
   scoped_ptr<LevelDBDatabase> db =
       LevelDBDatabase::OpenInMemory(comparator.get());
   if (!db) {
     LOG(ERROR) << "LevelDBDatabase::OpenInMemory failed.";
     HistogramOpenStatus(INDEXED_DB_BACKING_STORE_OPEN_MEMORY_FAILED,
                         origin_url);
@@ skipping 12 matching lines @@
 }
 
 // static
 scoped_refptr<IndexedDBBackingStore> IndexedDBBackingStore::Create(
     IndexedDBFactory* indexed_db_factory,
     const GURL& origin_url,
     const base::FilePath& blob_path,
     net::URLRequestContext* request_context,
     scoped_ptr<LevelDBDatabase> db,
     scoped_ptr<LevelDBComparator> comparator,
-    base::TaskRunner* task_runner,
+    base::SequencedTaskRunner* task_runner,
     leveldb::Status* status) {
   // TODO(jsbell): Handle comparator name changes.
   scoped_refptr<IndexedDBBackingStore> backing_store(
       new IndexedDBBackingStore(indexed_db_factory,
                                 origin_url,
                                 blob_path,
                                 request_context,
                                 db.Pass(),
                                 comparator.Pass(),
                                 task_runner));
@@ skipping 1106 matching lines @@
   int64 database_id_;
   IndexedDBBackingStore* backing_store_;
   scoped_refptr<IndexedDBBackingStore::BlobWriteCallback> callback_;
   scoped_ptr<FileWriterDelegate> delegate_;
   bool aborted_;
 
   DISALLOW_COPY_AND_ASSIGN(ChainedBlobWriterImpl);
 };
 
 class LocalWriteClosure : public FileWriterDelegate::DelegateWriteCallback,
-                          public base::RefCounted<LocalWriteClosure> {
+                          public base::RefCountedThreadSafe<LocalWriteClosure> {
  public:
   LocalWriteClosure(IndexedDBBackingStore::Transaction::ChainedBlobWriter*
                         chained_blob_writer,
-                    base::TaskRunner* task_runner)
+                    base::SequencedTaskRunner* task_runner)
       : chained_blob_writer_(chained_blob_writer),
         task_runner_(task_runner),
         bytes_written_(0) {}
 
   void Run(base::File::Error rv,
            int64 bytes,
            FileWriterDelegate::WriteProgressStatus write_status) {
     DCHECK_GE(bytes, 0);
     bytes_written_ += bytes;
     if (write_status == FileWriterDelegate::SUCCESS_IO_PENDING)
       return;  // We don't care about progress events.
     if (rv == base::File::FILE_OK) {
       DCHECK_EQ(write_status, FileWriterDelegate::SUCCESS_COMPLETED);
     } else {
       DCHECK(write_status == FileWriterDelegate::ERROR_WRITE_STARTED ||
              write_status == FileWriterDelegate::ERROR_WRITE_NOT_STARTED);
     }
     task_runner_->PostTask(
         FROM_HERE,
-        base::Bind(&LocalWriteClosure::callBlobCallbackOnIDBTaskRunner,
-                   this,
-                   write_status == FileWriterDelegate::SUCCESS_COMPLETED));
+        base::Bind(&IndexedDBBackingStore::Transaction::ChainedBlobWriter::

[michaeln, 2014/07/24 00:20:23]

aside: wow, didn't realize how deeply nested things are

+                       ReportWriteCompletion,
+                   chained_blob_writer_,
+                   write_status == FileWriterDelegate::SUCCESS_COMPLETED,
+                   bytes_written_));
   }
 
   void writeBlobToFileOnIOThread(const FilePath& file_path,
                                  const GURL& blob_url,
                                  net::URLRequestContext* request_context) {
     DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
     scoped_ptr<fileapi::FileStreamWriter> writer(
         fileapi::FileStreamWriter::CreateForLocalFile(
             task_runner_, file_path, 0,
             fileapi::FileStreamWriter::CREATE_NEW_FILE));
     scoped_ptr<FileWriterDelegate> delegate(
         new FileWriterDelegate(writer.Pass(),
                                FileWriterDelegate::FLUSH_ON_COMPLETION));
 
     DCHECK(blob_url.is_valid());
     scoped_ptr<net::URLRequest> blob_request(request_context->CreateRequest(
         blob_url, net::DEFAULT_PRIORITY, delegate.get(), NULL));
 
     delegate->Start(blob_request.Pass(),
                     base::Bind(&LocalWriteClosure::Run, this));
     chained_blob_writer_->set_delegate(delegate.Pass());
   }
 
  private:
-  virtual ~LocalWriteClosure() {}
-  friend class base::RefCounted<LocalWriteClosure>;
+  virtual ~LocalWriteClosure() {
+    // Make sure the last reference to a ChainedBlobWriter is released (and
+    // deleted) on the IDB thread since it owns a transaction which has thread
+    // affinity.
+    IndexedDBBackingStore::Transaction::ChainedBlobWriter* raw_tmp =
+        chained_blob_writer_.get();
+    raw_tmp->AddRef();
+    chained_blob_writer_ = NULL;
+    task_runner_->ReleaseSoon(FROM_HERE, raw_tmp);
+  }
+  friend class base::RefCountedThreadSafe<LocalWriteClosure>;
 
-  void callBlobCallbackOnIDBTaskRunner(bool succeeded) {
-    DCHECK(task_runner_->RunsTasksOnCurrentThread());
-    chained_blob_writer_->ReportWriteCompletion(succeeded, bytes_written_);
-  }
-
-  IndexedDBBackingStore::Transaction::ChainedBlobWriter* chained_blob_writer_;
-  base::TaskRunner* task_runner_;
+  scoped_refptr<IndexedDBBackingStore::Transaction::ChainedBlobWriter>
+      chained_blob_writer_;
+  base::SequencedTaskRunner* task_runner_;

[michaeln, 2014/07/24 00:20:23]

somewhat related, this probably s/b a scope_refptr<T> too

[cmumford, 2014/07/24 18:18:26]

Yeah, I counted 395 "task_runner_" members in Chromium classes - only 19 of them
are raw pointers.

   int64 bytes_written_;
 
   DISALLOW_COPY_AND_ASSIGN(LocalWriteClosure);
 };
 
 bool IndexedDBBackingStore::WriteBlobFile(
     int64 database_id,
     const Transaction::WriteDescriptor& descriptor,
     Transaction::ChainedBlobWriter* chained_blob_writer) {
 
@@ skipping 1932 matching lines @@
     int64_t size,
     base::Time last_modified)
     : is_file_(true),
       file_path_(file_path),
       key_(key),
       size_(size),
       last_modified_(last_modified) {
 }
 
 }  // namespace content