Re-work the thread restrictions on the DOM distiller database

components/dom_distiller/core/dom_distiller_database_unittest.cc

Index: components/dom_distiller/core/dom_distiller_database_unittest.cc
diff --git a/components/dom_distiller/core/dom_distiller_database_unittest.cc b/components/dom_distiller/core/dom_distiller_database_unittest.cc
index d3e3c0038bc19cf8a1e7b92cb6ee0e3e408245e9..1d467066503e0e821e6d5957918b04c42899f42d 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()));

[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.