Small: Use a weak pointer for callbacks to V4LocalDatabaseManager

components/safe_browsing_db/v4_local_database_manager.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.
 
 // This file should not be build on Android but is currently getting built.
 // TODO(vakh): Fix that: http://crbug.com/621647
 
 #include "components/safe_browsing_db/v4_local_database_manager.h"
 
 #include <vector>
@@ skipping 61 matching lines @@
 scoped_refptr<V4LocalDatabaseManager> V4LocalDatabaseManager::Create(
     const base::FilePath& base_path) {
   if (!V4FeatureList::IsLocalDatabaseManagerEnabled()) {
     return nullptr;
   }
 
   return make_scoped_refptr(new V4LocalDatabaseManager(base_path));
 }
 
 V4LocalDatabaseManager::V4LocalDatabaseManager(const base::FilePath& base_path)
-    : base_path_(base_path), enabled_(false), list_infos_(GetListInfos()) {
+    : base_path_(base_path),
+      enabled_(false),
+      list_infos_(GetListInfos()),
+      weak_factory_(this) {
   DCHECK(!base_path_.empty());
   DCHECK(!list_infos_.empty());
 
   DVLOG(1) << "V4LocalDatabaseManager::V4LocalDatabaseManager: "
            << "base_path_: " << base_path_.AsUTF8Unsafe();
 }
 
 V4LocalDatabaseManager::~V4LocalDatabaseManager() {
   DCHECK(!enabled_);
 }
@@ skipping 143 matching lines @@
 bool V4LocalDatabaseManager::IsSupported() const {
   return true;
 }
 
 void V4LocalDatabaseManager::StartOnIOThread(
     net::URLRequestContextGetter* request_context_getter,
     const V4ProtocolConfig& config) {
   SafeBrowsingDatabaseManager::StartOnIOThread(request_context_getter, config);
 
   db_updated_callback_ = base::Bind(&V4LocalDatabaseManager::DatabaseUpdated,
-                                    base::Unretained(this));
+                                    weak_factory_.GetWeakPtr());
 
   SetupUpdateProtocolManager(request_context_getter, config);
   SetupDatabase();
 
   enabled_ = true;
 }
 
 void V4LocalDatabaseManager::StopOnIOThread(bool shutdown) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   enabled_ = false;
 
   pending_clients_.clear();
 
   RespondSafeToQueuedChecks();
 
   // Delete the V4Database. Any pending writes to disk are completed.
   // This operation happens on the task_runner on which v4_database_ operates
   // and doesn't block the IO thread.
   V4Database::Destroy(std::move(v4_database_));
 
   // Delete the V4UpdateProtocolManager.
   // This cancels any in-flight update request.
   v4_update_protocol_manager_.reset();

[Nathan Parker, 2016/10/18 05:06:37]

Does this mean that any calls to the protocol_manager don't need to use a weak
ptr in callbacks?  In general weak ptrs are safe to use when unnecessary, so
it's not a big deal, but maybe redundant.

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

Not sure exactly what you mean but all callbacks in this class now use a weak
pointer. I'm going to look at other V4* classes (specifically V4Database) also
(not immediately) and consider using weak-ptrs there also, if needed.

 
   db_updated_callback_.Reset();
 
   SafeBrowsingDatabaseManager::StopOnIOThread(shutdown);
 }
 
 //
 // End: SafeBrowsingDatabaseManager implementation
 //
 
 void V4LocalDatabaseManager::DatabaseReadyForChecks(
     std::unique_ptr<V4Database> v4_database) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   // The following check is needed because it is possible that by the time the
   // database is ready, StopOnIOThread has been called.
   if (enabled_) {
     v4_database_ = std::move(v4_database);
 
     // The consistency of the stores read from the disk needs to verified. Post
     // that task on the task runner. It calls |DatabaseReadyForUpdates|
     // callback with the stores to reset, if any, and then we can schedule the
     // database updates.
     v4_database_->VerifyChecksum(
         base::Bind(&V4LocalDatabaseManager::DatabaseReadyForUpdates,
-                   base::Unretained(this)));
+                   weak_factory_.GetWeakPtr()));
 
     ProcessQueuedChecks();
   } else {
     // Schedule the deletion of v4_database off IO thread.
     V4Database::Destroy(std::move(v4_database));
   }
 }
 
 void V4LocalDatabaseManager::DatabaseReadyForUpdates(
     const std::vector<ListIdentifier>& stores_to_reset) {
@@ skipping 114 matching lines @@
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   DCHECK(enabled_);
   DCHECK(!full_hash_to_store_and_hash_prefixes.empty());
 
   pending_clients_.insert(check->client);
 
   v4_get_hash_protocol_manager_->GetFullHashes(
       full_hash_to_store_and_hash_prefixes,
       base::Bind(&V4LocalDatabaseManager::OnFullHashResponse,
-                 base::Unretained(this), base::Passed(std::move(check))));
+                 weak_factory_.GetWeakPtr(), base::Passed(std::move(check))));
 }
 
 void V4LocalDatabaseManager::ProcessQueuedChecks() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   for (auto& it : queued_checks_) {
     FullHashToStoreAndHashPrefixesMap full_hash_to_store_and_hash_prefixes;
     if (!GetPrefixMatches(it, &full_hash_to_store_and_hash_prefixes)) {
       RespondToClient(std::move(it));
     } else {
       PerformFullHashCheck(std::move(it), full_hash_to_store_and_hash_prefixes);
@@ skipping 27 matching lines @@
   // previously been started and stopped, a task runner could already exist.
   if (!task_runner_) {
     base::SequencedWorkerPool* pool = BrowserThread::GetBlockingPool();
     task_runner_ = pool->GetSequencedTaskRunnerWithShutdownBehavior(
         pool->GetSequenceToken(), base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
   }
 
   // Do not create the database on the IO thread since this may be an expensive
   // operation. Instead, do that on the task_runner and when the new database
   // has been created, swap it out on the IO thread.
-  NewDatabaseReadyCallback db_ready_callback = base::Bind(
-      &V4LocalDatabaseManager::DatabaseReadyForChecks, base::Unretained(this));
+  NewDatabaseReadyCallback db_ready_callback =
+      base::Bind(&V4LocalDatabaseManager::DatabaseReadyForChecks,
+                 weak_factory_.GetWeakPtr());
   V4Database::Create(task_runner_, base_path_, list_infos_, db_ready_callback);
 }
 
 void V4LocalDatabaseManager::SetupUpdateProtocolManager(
     net::URLRequestContextGetter* request_context_getter,
     const V4ProtocolConfig& config) {
-  V4UpdateCallback callback = base::Bind(
-      &V4LocalDatabaseManager::UpdateRequestCompleted, base::Unretained(this));
+  V4UpdateCallback callback =
+      base::Bind(&V4LocalDatabaseManager::UpdateRequestCompleted,
+                 weak_factory_.GetWeakPtr());
 
   v4_update_protocol_manager_ =
       V4UpdateProtocolManager::Create(request_context_getter, config, callback);
 }
 
 void V4LocalDatabaseManager::UpdateRequestCompleted(
     std::unique_ptr<ParsedServerResponse> parsed_server_response) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   v4_database_->ApplyUpdate(std::move(parsed_server_response),
                             db_updated_callback_);
 }
 
 }  // namespace safe_browsing