[S] Use a weak_factory for V4Database callbacks + test

components/safe_browsing_db/v4_database.cc

 // Copyright 2016 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 <memory>
 
 #include "base/callback.h"
 #include "base/debug/leak_annotations.h"
 #include "base/files/file_util.h"
 #include "base/memory/ptr_util.h"
@@ skipping 73 matching lines @@
 
   UMA_HISTOGRAM_TIMES("SafeBrowsing.V4DatabaseOpen.Time",
                       TimeTicks::Now() - create_start_time);
 }
 
 V4Database::V4Database(
     const scoped_refptr<base::SequencedTaskRunner>& db_task_runner,
     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());
 }
 
 // static
 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);
   }
 }
 
 V4Database::~V4Database() {
   DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
 }
 
 void V4Database::ApplyUpdate(
     std::unique_ptr<ParsedServerResponse> parsed_server_response,
@@ skipping 10 matching lines @@
       base::ThreadTaskRunnerHandle::Get();
   for (std::unique_ptr<ListUpdateResponse>& response :
        *parsed_server_response) {
     ListIdentifier identifier(*response);
     StoreMap::const_iterator iter = store_map_->find(identifier);
     if (iter != store_map_->end()) {
       const std::unique_ptr<V4Store>& old_store = iter->second;
       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()),
                        base::Passed(std::move(response)), current_task_runner,
                        store_ready_callback));
       }
     } else {
       NOTREACHED() << "Got update for unexpected identifier: " << identifier;
     }
   }
@@ skipping 69 matching lines @@
 }
 
 void V4Database::VerifyChecksum(
     DatabaseReadyForUpdatesCallback db_ready_for_updates_callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   // TODO(vakh): Consider using PostTaskAndReply instead.
   const scoped_refptr<base::SingleThreadTaskRunner> callback_task_runner =
       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));
 }
 
 void V4Database::VerifyChecksumOnTaskRunner(
     const scoped_refptr<base::SingleThreadTaskRunner>& callback_task_runner,
     DatabaseReadyForUpdatesCallback db_ready_for_updates_callback) {
   std::vector<ListIdentifier> stores_to_reset;
   for (const auto& store_map_iter : *store_map_) {
     if (!store_map_iter.second->VerifyChecksum()) {
       stores_to_reset.push_back(store_map_iter.first);
@@ skipping 23 matching lines @@
       filename_(filename),
       list_id_(list_id),
       sb_threat_type_(sb_threat_type) {
   DCHECK(!fetch_updates_ || !filename_.empty());
   DCHECK_NE(SB_THREAT_TYPE_SAFE, sb_threat_type_);
 }
 
 ListInfo::~ListInfo() {}
 
 }  // namespace safe_browsing