Re-work the thread restrictions on the DOM distiller database

components/dom_distiller/core/dom_distiller_database_unittest.cc

 // Copyright 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 "components/dom_distiller/core/dom_distiller_database.h"
 
 #include <map>
 
 #include "base/bind.h"
 #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"
 
 using base::MessageLoop;
 using base::ScopedTempDir;
 using testing::Invoke;
 using testing::Return;
 using testing::_;
 
@@ skipping 57 matching lines @@
     std::string serialized_actual = actual[i].SerializeAsString();
     EXPECT_EQ(serialized_expected, serialized_actual);
     expected.erase(expected_it);
   }
 }
 
 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_;
 };
 
 // Test that DomDistillerDatabase calls Init on the underlying database and that
 // the caller's InitCallback is called with the correct value.
 TEST_F(DomDistillerDatabaseTest, TestDBInitSuccess) {
   base::FilePath path(FILE_PATH_LITERAL("/fake/path"));
 
   MockDB* mock_db = new MockDB();
   EXPECT_CALL(*mock_db, Init(path)).WillOnce(Return(true));
@@ skipping 140 matching lines @@
 
   EXPECT_CALL(*mock_db, Save(_)).WillOnce(Return(false));
   EXPECT_CALL(caller, SaveCallback(false));
   db_->SaveEntries(
       entries.Pass(),
       base::Bind(&MockDatabaseCaller::SaveCallback, base::Unretained(&caller)));
 
   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()));

[Nico, 2013/11/04 19:31:56]

Can this be on the stack too? (In a local block, since you want it to go away
before pumping the runloop)

[cjhopman, 2013/11/05 19:08:29]

It could be. I feel like that is subtle enough that I would want to add a
comment about why we need the local block, and then at that point it's simpler
to do the more explicit scoped_ptr w/ .reset().

+
+  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.
 void TestLevelDBSaveAndLoad(bool close_after_save) {
   ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
   EntryMap model = GetSmallModel();
   EntryVector save_entries;
   EntryVector load_entries;
@@ skipping 19 matching lines @@
 
 TEST(DomDistillerDatabaseLevelDBTest, TestDBSaveAndLoad) {
   TestLevelDBSaveAndLoad(false);
 }
 
 TEST(DomDistillerDatabaseLevelDBTest, TestDBCloseAndReopen) {
   TestLevelDBSaveAndLoad(true);
 }
 
 }  // namespace dom_distiller