Re-work the thread restrictions on the DOM distiller database

In the current implementation, DomDistillerDatabase cannot be
deleted on the main thread, instead the user must call ::Destroy(),
and then let the database delete itself on its task runner. This is
awkward and sort of leaks implementation details out of that class.

Second, currently the parts of DomDistillerDatabase on each thread
are doing some ad hoc method of checking their thread restrictions.
Using ThreadChecker is better, though it loses some flexibility
from checking TaskRunner::RunsTasksOnCurrentThread (maybe there
should be something like a TaskRunnerChecker for that).

Now:
1. no functions are called on DomDistillerDatabase off of the main
thread
2. DomDistillerDatabase::LevelDB enforces that function calls and
destructor all run on the same thread
3. DomDistillerDatabase can be deleted on the main thread

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

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

components/dom_distiller/core/dom_distiller_database.cc

@@ -23,11 +23,17 @@ using base::SequencedTaskRunner;
 
 namespace dom_distiller {
 
-DomDistillerDatabase::LevelDB::LevelDB() {}
+DomDistillerDatabase::LevelDB::LevelDB() {
+  thread_checker_.DetachFromThread();
+}
 
-DomDistillerDatabase::LevelDB::~LevelDB() {}
+DomDistillerDatabase::LevelDB::~LevelDB() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+}
 
 bool DomDistillerDatabase::LevelDB::Init(const base::FilePath& database_dir) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   leveldb::Options options;
   options.create_if_missing = true;
   options.max_open_files = 0;  // Use minimum.
@@ -37,7 +43,6 @@ bool DomDistillerDatabase::LevelDB::Init(const base::FilePath& database_dir) {
   leveldb::DB* db = NULL;
   leveldb::Status status = leveldb::DB::Open(options, path, &db);
   if (status.IsCorruption()) {
-    LOG(WARNING) << "Deleting possibly-corrupt database";
     base::DeleteFile(database_dir, true);
     status = leveldb::DB::Open(options, path, &db);
   }
@@ -54,6 +59,8 @@ bool DomDistillerDatabase::LevelDB::Init(const base::FilePath& database_dir) {
 }
 
 bool DomDistillerDatabase::LevelDB::Save(const EntryVector& entries) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   leveldb::WriteBatch updates;
   for (EntryVector::const_iterator it = entries.begin(); it != entries.end();
        ++it) {
@@ -67,11 +74,14 @@ bool DomDistillerDatabase::LevelDB::Save(const EntryVector& entries) {
   if (status.ok())
     return true;
 
-  LOG(WARNING) << "Failed writing dom_distiller entries: " << status.ToString();
+  DLOG(WARNING) << "Failed writing dom_distiller entries: "
+                << status.ToString();
   return false;
 }
 
 bool DomDistillerDatabase::LevelDB::Load(EntryVector* entries) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   leveldb::ReadOptions options;
   scoped_ptr<leveldb::Iterator> db_iterator(db_->NewIterator(options));
   for (db_iterator->SeekToFirst(); db_iterator->Valid(); db_iterator->Next()) {
@@ -79,8 +89,8 @@ bool DomDistillerDatabase::LevelDB::Load(EntryVector* entries) {
 
     ArticleEntry entry;
     if (!entry.ParseFromArray(value_slice.data(), value_slice.size())) {
-      LOG(WARNING) << "Unable to parse dom_distiller entry "
-                   << db_iterator->key().ToString();
+      DLOG(WARNING) << "Unable to parse dom_distiller entry "
+                    << db_iterator->key().ToString();
       // TODO(cjhopman): Decide what to do about un-parseable entries.
     }
     entries->push_back(entry);
@@ -88,26 +98,6 @@ bool DomDistillerDatabase::LevelDB::Load(EntryVector* entries) {
   return true;
 }
 
-DomDistillerDatabase::DomDistillerDatabase(
-    scoped_refptr<base::SequencedTaskRunner> task_runner)
-    : task_runner_(task_runner), weak_ptr_factory_(this) {
-  main_loop_ = MessageLoop::current();
-}
-
-void DomDistillerDatabase::Destroy() {
-  DCHECK(IsRunOnMainLoop());
-  weak_ptr_factory_.InvalidateWeakPtrs();
-  task_runner_->PostNonNestableTask(
-      FROM_HERE,
-      base::Bind(&DomDistillerDatabase::DestroyFromTaskRunner,
-                 base::Unretained(this)));
-}
-
-void DomDistillerDatabase::Init(const base::FilePath& database_dir,
-                                InitCallback callback) {
-  InitWithDatabase(scoped_ptr<Database>(new LevelDB()), database_dir, callback);
-}
-
 namespace {
 
 void RunInitCallback(DomDistillerDatabaseInterface::InitCallback callback,
@@ -126,20 +116,57 @@ void RunLoadCallback(DomDistillerDatabaseInterface::LoadCallback callback,
   callback.Run(*success, entries.Pass());
 }
 
+void InitFromTaskRunner(DomDistillerDatabase::Database* database,
+                        const base::FilePath& database_dir,
+                        bool* success) {
+  DCHECK(success);
+
+  // TODO(cjhopman): Histogram for database size.
+  *success = database->Init(database_dir);
+}
+
+void SaveEntriesFromTaskRunner(DomDistillerDatabase::Database* database,
+                               scoped_ptr<EntryVector> entries,
+                               bool* success) {
+  DCHECK(success);
+  *success = database->Save(*entries);
+}
+
+void LoadEntriesFromTaskRunner(DomDistillerDatabase::Database* database,
+                               EntryVector* entries,
+                               bool* success) {
+  DCHECK(success);
+  DCHECK(entries);
+
+  entries->clear();
+  *success = database->Load(entries);
+}
+
 }  // namespace
 
+DomDistillerDatabase::DomDistillerDatabase(
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+    : task_runner_(task_runner) {
+}
+
+void DomDistillerDatabase::Init(const base::FilePath& database_dir,
+                                InitCallback callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  InitWithDatabase(scoped_ptr<Database>(new LevelDB()), database_dir, callback);
+}
+
 void DomDistillerDatabase::InitWithDatabase(scoped_ptr<Database> database,
                                             const base::FilePath& database_dir,
                                             InitCallback callback) {
-  DCHECK(IsRunOnMainLoop());
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!db_);
   DCHECK(database);
   db_.reset(database.release());
   bool* success = new bool(false);
   task_runner_->PostTaskAndReply(
       FROM_HERE,
-      base::Bind(&DomDistillerDatabase::InitFromTaskRunner,
-                 base::Unretained(this),
+      base::Bind(InitFromTaskRunner,
+                 base::Unretained(db_.get()),
                  database_dir,
                  success),
       base::Bind(RunInitCallback, callback, base::Owned(success)));
@@ -147,20 +174,19 @@ void DomDistillerDatabase::InitWithDatabase(scoped_ptr<Database> database,
 
 void DomDistillerDatabase::SaveEntries(scoped_ptr<EntryVector> entries,
                                        SaveCallback callback) {
-  DCHECK(IsRunOnMainLoop());
+  DCHECK(thread_checker_.CalledOnValidThread());
   bool* success = new bool(false);
   task_runner_->PostTaskAndReply(
       FROM_HERE,
-      base::Bind(&DomDistillerDatabase::SaveEntriesFromTaskRunner,
-                 base::Unretained(this),
+      base::Bind(SaveEntriesFromTaskRunner,
+                 base::Unretained(db_.get()),
                  base::Passed(&entries),
                  success),
       base::Bind(RunSaveCallback, callback, base::Owned(success)));
 }
 
 void DomDistillerDatabase::LoadEntries(LoadCallback callback) {
-  DCHECK(IsRunOnMainLoop());
-
+  DCHECK(thread_checker_.CalledOnValidThread());
   bool* success = new bool(false);
 
   scoped_ptr<EntryVector> entries(new EntryVector());
@@ -169,8 +195,8 @@ void DomDistillerDatabase::LoadEntries(LoadCallback callback) {
 
   task_runner_->PostTaskAndReply(
       FROM_HERE,
-      base::Bind(&DomDistillerDatabase::LoadEntriesFromTaskRunner,
-                 base::Unretained(this),
+      base::Bind(LoadEntriesFromTaskRunner,
+                 base::Unretained(db_.get()),
                  entries_ptr,
                  success),
       base::Bind(RunLoadCallback,
@@ -179,50 +205,11 @@ void DomDistillerDatabase::LoadEntries(LoadCallback callback) {
                  base::Passed(&entries)));
 }
 
-DomDistillerDatabase::~DomDistillerDatabase() { DCHECK(IsRunByTaskRunner()); }
-
-bool DomDistillerDatabase::IsRunByTaskRunner() const {
-  return task_runner_->RunsTasksOnCurrentThread();
-}
-
-bool DomDistillerDatabase::IsRunOnMainLoop() const {
-  return MessageLoop::current() == main_loop_;
-}
-
-void DomDistillerDatabase::DestroyFromTaskRunner() {
-  DCHECK(IsRunByTaskRunner());
-  delete this;
-}
-
-void DomDistillerDatabase::InitFromTaskRunner(
-    const base::FilePath& database_dir,
-    bool* success) {
-  DCHECK(IsRunByTaskRunner());
-  DCHECK(success);
-
-  VLOG(1) << "Opening " << database_dir.value();
-
-  // TODO(cjhopman): Histogram for database size.
-  *success = db_->Init(database_dir);
-}
-
-void DomDistillerDatabase::SaveEntriesFromTaskRunner(
-    scoped_ptr<EntryVector> entries,
-    bool* success) {
-  DCHECK(IsRunByTaskRunner());
-  DCHECK(success);
-  VLOG(1) << "Saving " << entries->size() << " entry(ies) to database ";
-  *success = db_->Save(*entries);
-}
-
-void DomDistillerDatabase::LoadEntriesFromTaskRunner(EntryVector* entries,
-                                                     bool* success) {
-  DCHECK(IsRunByTaskRunner());
-  DCHECK(success);
-  DCHECK(entries);
-
-  entries->clear();
-  *success = db_->Load(entries);
+DomDistillerDatabase::~DomDistillerDatabase() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (!task_runner_->DeleteSoon(FROM_HERE, db_.release())) {
+    DLOG(WARNING) << "DOM distiller database will not be deleted.";
+  }
 }
 
 }  // namespace dom_distiller

components/dom_distiller/core/dom_distiller_database.h

@@ -14,6 +14,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "base/memory/weak_ptr.h"
+#include "base/threading/thread_checker.h"
 #include "components/dom_distiller/core/article_entry.h"
 
 namespace base {
@@ -40,11 +41,6 @@ class DomDistillerDatabaseInterface {
 
   virtual ~DomDistillerDatabaseInterface() {}
 
-  // Asynchronously destroys the object after all in-progress file operations
-  // have completed. The callbacks for in-progress operations will still be
-  // called.
-  virtual void Destroy() {}
-
   // Asynchronously initializes the object. |callback| will be invoked on the UI
   // thread when complete.
   virtual void Init(const base::FilePath& database_dir,
@@ -60,6 +56,8 @@ class DomDistillerDatabaseInterface {
   virtual void LoadEntries(LoadCallback callback) = 0;
 };
 
+// When the DomDistillerDatabase instance is deleted, in-progress asynchronous
+// operations will be completed and the corresponding callbacks will be called.
 class DomDistillerDatabase
     : public DomDistillerDatabaseInterface {
  public:
@@ -72,6 +70,8 @@ class DomDistillerDatabase
     virtual ~Database() {}
   };
 
+  // Once constructed, function calls and destruction should all occur on the
+  // same thread (not necessarily the same as the constructor).
   class LevelDB : public Database {
    public:
     LevelDB();
@@ -81,7 +81,7 @@ class DomDistillerDatabase
     virtual bool Load(EntryVector* entries) OVERRIDE;
 
    private:
-
+    base::ThreadChecker thread_checker_;
     scoped_ptr<leveldb::DB> db_;
   };
 
@@ -90,7 +90,6 @@ class DomDistillerDatabase
   virtual ~DomDistillerDatabase();
 
   // DomDistillerDatabaseInterface implementation.
-  virtual void Destroy() OVERRIDE;
   virtual void Init(const base::FilePath& database_dir,
                     InitCallback callback) OVERRIDE;
   virtual void SaveEntries(scoped_ptr<EntryVector> entries_to_save,
@@ -103,37 +102,13 @@ class DomDistillerDatabase
                         InitCallback callback);
 
  private:
-  // Whether currently being run by |task_runner_|.
-  bool IsRunByTaskRunner() const;
-
-  // Whether currently being run on |main_loop_|.
-  bool IsRunOnMainLoop() const;
-
-  // Deletes |this|.
-  void DestroyFromTaskRunner();
-
-  // Initializes the database in |database_dir| and updates |success|.
-  void InitFromTaskRunner(const base::FilePath& database_dir, bool* success);
-
-  // Saves data to disk and updates |success|.
-  void SaveEntriesFromTaskRunner(scoped_ptr<EntryVector> entries_to_save,
-                                 bool* success);
-
-  // Loads entries from disk and updates |success|.
-  void LoadEntriesFromTaskRunner(EntryVector* entries, bool* success);
-
-  // The MessageLoop that the database was created on.
-  base::MessageLoop* main_loop_;
+  base::ThreadChecker thread_checker_;
 
   // Used to run blocking tasks in-order.
   scoped_refptr<base::SequencedTaskRunner> task_runner_;
 
   scoped_ptr<Database> db_;
 
-  // Note: This should remain the last member so it'll be destroyed and
-  // invalidate its weak pointers before any other members are destroyed.
-  base::WeakPtrFactory<DomDistillerDatabase> weak_ptr_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(DomDistillerDatabase);
 };
 

components/dom_distiller/core/dom_distiller_database_unittest.cc

@@ -10,6 +10,7 @@
 #include "base/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/run_loop.h"
+#include "base/threading/thread.h"
 #include "components/dom_distiller/core/article_entry.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -87,23 +88,16 @@ class DomDistillerDatabaseTest : public testing::Test {
  public:
   virtual void SetUp() {
     main_loop_.reset(new MessageLoop());
-    db_ = new DomDistillerDatabase(main_loop_->message_loop_proxy());
+    db_.reset(new DomDistillerDatabase(main_loop_->message_loop_proxy()));
   }
 
   virtual void TearDown() {
-    DestroyDB();
-    main_loop_.reset(NULL);
+    db_.reset();
+    base::RunLoop().RunUntilIdle();
+    main_loop_.reset();
   }
 
-  void DestroyDB() {
-    if (db_) {
-      db_->Destroy();
-      base::RunLoop().RunUntilIdle();
-      db_ = NULL;
-    }
-  }
-
-  DomDistillerDatabase* db_;
+  scoped_ptr<DomDistillerDatabase> db_;
   scoped_ptr<MessageLoop> main_loop_;
 };
 
@@ -264,6 +258,34 @@ TEST_F(DomDistillerDatabaseTest, TestDBSaveFailure) {
   base::RunLoop().RunUntilIdle();
 }
 
+// This tests that normal usage of the real database does not cause any
+// threading violations.
+TEST(DomDistillerDatabaseThreadingTest, TestDBDestruction) {
+  base::MessageLoop main_loop;
+
+  ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+
+  base::Thread db_thread("dbthread");
+  ASSERT_TRUE(db_thread.Start());
+
+  scoped_ptr<DomDistillerDatabase> db(
+      new DomDistillerDatabase(db_thread.message_loop_proxy()));
+
+  MockDatabaseCaller caller;
+  EXPECT_CALL(caller, InitCallback(_));
+  db->Init(
+      temp_dir.path(),
+      base::Bind(&MockDatabaseCaller::InitCallback, base::Unretained(&caller)));
+
+  db.reset();
+
+  base::RunLoop run_loop;
+  db_thread.message_loop_proxy()->PostTaskAndReply(
+      FROM_HERE, base::Bind(base::DoNothing), run_loop.QuitClosure());
+  run_loop.Run();
+}
+
 // Test that the LevelDB properly saves entries and that load returns the saved
 // entries. If |close_after_save| is true, the database will be closed after
 // saving and then re-opened to ensure that the data is properly persisted.

components/dom_distiller/core/dom_distiller_store_unittest.cc

@@ -55,8 +55,6 @@ class FakeDB : public DomDistillerDatabaseInterface {
  public:
   explicit FakeDB(EntryMap* db) : db_(db) {}
 
-  virtual void Destroy() OVERRIDE {}
-
   virtual void Init(
       const base::FilePath& database_dir,
       DomDistillerDatabaseInterface::InitCallback callback) OVERRIDE {