Small: Use a weak pointer for callbacks to V4LocalDatabaseManager

components/safe_browsing_db/v4_local_database_manager_unittest.cc

Index: components/safe_browsing_db/v4_local_database_manager_unittest.cc
diff --git a/components/safe_browsing_db/v4_local_database_manager_unittest.cc b/components/safe_browsing_db/v4_local_database_manager_unittest.cc
index 61ca2bb8801102e40425704295e0d71d4980062e..e9e7b8b31a852319a585ff33f56411a5612be724 100644
--- a/components/safe_browsing_db/v4_local_database_manager_unittest.cc
+++ b/components/safe_browsing_db/v4_local_database_manager_unittest.cc
@@ -172,7 +172,9 @@ class V4LocalDatabaseManagerTest : public PlatformTest {
   }
 
   void StopLocalDatabaseManager() {
-    v4_local_database_manager_->StopOnIOThread(true);
+    if (v4_local_database_manager_) {
+      v4_local_database_manager_->StopOnIOThread(true);
+    }
 
     // Force destruction of the database.
     task_runner_->RunPendingTasks();
@@ -332,4 +334,31 @@ TEST_F(V4LocalDatabaseManagerTest, PerformFullHashCheckCalledAsync) {
       v4_local_database_manager_));
 }
 
+TEST_F(V4LocalDatabaseManagerTest, UsingWeakPtrDropsCallback) {
+  // StopLocalDatabaseManager before resetting it because that's what
+  // ~V4LocalDatabaseManager expects.
+  StopLocalDatabaseManager();
+  v4_local_database_manager_ =
+      make_scoped_refptr(new FakeV4LocalDatabaseManager(base_dir_.GetPath()));
+  SetTaskRunnerForTest();
+  StartLocalDatabaseManager();
+  WaitForTasksOnTaskRunner();
+  net::TestURLFetcherFactory factory;
+
+  StoreAndHashPrefixes store_and_hash_prefixes;
+  store_and_hash_prefixes.emplace_back(GetUrlMalwareId(), HashPrefix("aaaa"));
+  ReplaceV4Database(store_and_hash_prefixes);
+
+  // The fake database returns a matched hash prefix.
+  EXPECT_FALSE(v4_local_database_manager_->CheckBrowseUrl(
+      GURL("http://example.com/a/"), nullptr));
+  v4_local_database_manager_->StopOnIOThread(true);
+
+  // Release the V4LocalDatabaseManager object right away before the callback
+  // gets called. When the callback gets called, without using a weak-ptr
+  // factory, this leads to a use after free. However, using the weak-ptr means
+  // that the callback is simply dropped.
+  v4_local_database_manager_ = nullptr;
+}

[Scott Hess - ex-Googler, 2016/10/18 03:13:00]

This seems like StopOnIOThread() maybe has posted things to run on the task
runner which can go past the end of the test function.  Should this
WaitForTasksOnTaskRunner() to keep things hermetic?

I want to suggest also somehow verifying that the callback didn't happen, but
... how?  Can't think of any way to isolate that without being grossly intrusive
implementation changes which would probably cause more errors than this would
ever catch.

[vakh (use Gerrit instead), 2016/10/18 17:37:44]

Good idea. Done.
It is already done in TearDown, but it is good for the test to be explicit about
it.
Yes, I agree. I think it's going to be more trouble than the value it'd add.
Skipping for now.

[Scott Hess - ex-Googler, 2016/10/18 20:09:56]

Aha, I see what you mean, I hadn't connected the helper method with TearDown(),
but it makes sense.

Still, I think I prefer the inline version, unless the test file is pretty
uniformly depending on the setup of the test class.  Which I think would be
tough in this case, since some tests need to drain the tasks before performing
additional checks.

+
 }  // namespace safe_browsing