diff options
4 files changed, 108 insertions, 126 deletions
diff --git a/components/dom_distiller/core/dom_distiller_database.cc b/components/dom_distiller/core/dom_distiller_database.cc index a4d599c..cda79fb 100644 --- a/components/dom_distiller/core/dom_distiller_database.cc +++ b/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 diff --git a/components/dom_distiller/core/dom_distiller_database.h b/components/dom_distiller/core/dom_distiller_database.h index 0f70d2e..d50568b 100644 --- a/components/dom_distiller/core/dom_distiller_database.h +++ b/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); }; diff --git a/components/dom_distiller/core/dom_distiller_database_unittest.cc b/components/dom_distiller/core/dom_distiller_database_unittest.cc index d3e3c00..1d46706 100644 --- a/components/dom_distiller/core/dom_distiller_database_unittest.cc +++ b/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. diff --git a/components/dom_distiller/core/dom_distiller_store_unittest.cc b/components/dom_distiller/core/dom_distiller_store_unittest.cc index 9e4cf35..db777e9 100644 --- a/components/dom_distiller/core/dom_distiller_store_unittest.cc +++ b/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 { |