[S] Use a weak_factory for V4Database callbacks + test

components/safe_browsing_db/v4_database.cc

Index: components/safe_browsing_db/v4_database.cc
diff --git a/components/safe_browsing_db/v4_database.cc b/components/safe_browsing_db/v4_database.cc
index 5edeb3e9422df61daf2dfe31e5f12d811af37627..c4e2e4de2094f9e3cef214bd3cb3422337fa046e 100644
--- a/components/safe_browsing_db/v4_database.cc
+++ b/components/safe_browsing_db/v4_database.cc
@@ -91,7 +91,8 @@ V4Database::V4Database(
     std::unique_ptr<StoreMap> store_map)
     : db_task_runner_(db_task_runner),
       store_map_(std::move(store_map)),
-      pending_store_updates_(0) {
+      pending_store_updates_(0),
+      weak_factory_(this) {
   DCHECK(db_task_runner->RunsTasksOnCurrentThread());
 }
 
@@ -100,6 +101,7 @@ void V4Database::Destroy(std::unique_ptr<V4Database> v4_database) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   V4Database* v4_database_raw = v4_database.release();
   if (v4_database_raw) {
+    v4_database_raw->weak_factory_.InvalidateWeakPtrs();
     v4_database_raw->db_task_runner_->DeleteSoon(FROM_HERE, v4_database_raw);
   }
 }
@@ -130,8 +132,9 @@ void V4Database::ApplyUpdate(
       if (old_store->state() != response->new_client_state()) {
         // A different state implies there are updates to process.
         pending_store_updates_++;
-        UpdatedStoreReadyCallback store_ready_callback = base::Bind(
-            &V4Database::UpdatedStoreReady, base::Unretained(this), identifier);
+        UpdatedStoreReadyCallback store_ready_callback =
+            base::Bind(&V4Database::UpdatedStoreReady,
+                       weak_factory_.GetWeakPtr(), identifier);
         db_task_runner_->PostTask(
             FROM_HERE,
             base::Bind(&V4Store::ApplyUpdate, base::Unretained(old_store.get()),
@@ -221,7 +224,7 @@ void V4Database::VerifyChecksum(
       base::ThreadTaskRunnerHandle::Get();
   db_task_runner_->PostTask(
       FROM_HERE, base::Bind(&V4Database::VerifyChecksumOnTaskRunner,
-                            base::Unretained(this), callback_task_runner,
+                            weak_factory_.GetWeakPtr(), callback_task_runner,

[Nathan Parker, 2017/01/21 01:01:08]

Hmm, but this one is going to get checked on the task runner, rather than the IO
thread, ya?

[vakh (use Gerrit instead), 2017/01/21 01:16:31]

The GetWeakPtr method has been called on the IO thread, and that's what seems to
matter, based on what I gather from reading that comment.

Trying various ASAN and MSAN trybots to see what happens.

[Scott Hess - ex-Googler, 2017/01/21 04:18:20]

Oh no!  I think this is what I was worrying about!

I think this usage will deref the weak pointer (can get()) on the wrong thread. 
This can be an opaque parameter, but not a target.

That said, it should narrow the race condition.  So I'm not sure about making a
value judgement about shipping it (it should get strictly better, though it's
still wrong).  I think fixing this should gate turning the experiment up for
more people, though.

I think this is back to the db_task_runner_ thread needs to create the weak
pointer and post it over for the foreground to reference.  Maybe for the short
term it is easiest to just have the create callback return both an owning
unique_ptr and a weak_ptr used for messaging.  That's ugly, but I think it
allows correct operation.  Basically, the unique_ptr is stored and _only_ passed
to Destroy().  On the other end, the destructor will destroy the weak-pointer
factory, and any in-flight postings will short-circuit.

                             db_ready_for_updates_callback));
 }