Have a list of pending checks instead of pending clients

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 142 matching lines @@
 }
 
 //
 // Start: SafeBrowsingDatabaseManager implementation
 //
 
 void V4LocalDatabaseManager::CancelCheck(Client* client) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(enabled_);
 
-  auto it = pending_clients_.find(client);
-  if (it != pending_clients_.end()) {
-    pending_clients_.erase(it);
+  auto pending_it = std::find_if(
+      std::begin(pending_checks_), std::end(pending_checks_),
+      [&client](const PendingCheck* check) { return check->client == client; });

[Scott Hess - ex-Googler, 2017/01/06 00:42:02]

Does capture-by-value (no &) work fine for client, here?  I think that would
look less interesting (in the "do I need to worry about that?" sense).

[vakh (use Gerrit instead), 2017/01/09 19:21:32]

Done.

+  if (pending_it != pending_checks_.end()) {
+    pending_checks_.erase(pending_it);
   }
 
   auto queued_it =
       std::find_if(std::begin(queued_checks_), std::end(queued_checks_),
                    [&client](const std::unique_ptr<PendingCheck>& check) {
                      return check->client == client;
                    });
   if (queued_it != queued_checks_.end()) {
     queued_checks_.erase(queued_it);
   }
@@ skipping 192 matching lines @@
   SetupDatabase();
 
   enabled_ = true;
 }
 
 void V4LocalDatabaseManager::StopOnIOThread(bool shutdown) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   enabled_ = false;
 
-  pending_clients_.clear();
+  pending_checks_.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.
@@ skipping 138 matching lines @@
   if (!v4_database_) {
     queued_checks_.push_back(std::move(check));
     return false;
   }
 
   FullHashToStoreAndHashPrefixesMap full_hash_to_store_and_hash_prefixes;
   if (!GetPrefixMatches(check, &full_hash_to_store_and_hash_prefixes)) {
     return true;
   }
 
+  pending_clients_.insert(check->client);

[Scott Hess - ex-Googler, 2017/01/06 00:42:02]

Something going wrong, here.  Perhaps a merge issue?

If you're planning to move the pending_checks_ insert from here to
PerformFullHashCheck(), note that that means that you could no longer cancel a
check between when HandleCheck() is called and PerformFullHashCheck() is posted.

Actually, I think it's reasonable to keep the check-in code here, because the
fact that you need to pump things internally shouldn't be exposed to the
clients.

[vakh (use Gerrit instead), 2017/01/06 00:43:49]

Yup, bad merge. See new patch.

   // Post on the IO thread to enforce async behavior.
-  pending_clients_.insert(check->client);
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&V4LocalDatabaseManager::PerformFullHashCheck, this,
                  base::Passed(std::move(check)),
                  full_hash_to_store_and_hash_prefixes));
 
   return false;
 }
 
 bool V4LocalDatabaseManager::HandleHashSynchronously(
@@ skipping 16 matching lines @@
 
   std::unique_ptr<PendingCheck> check = base::MakeUnique<PendingCheck>(
       nullptr, ClientCallbackType::CHECK_OTHER, stores_to_check,
       std::vector<GURL>(1, url));
 
   FullHashToStoreAndHashPrefixesMap full_hash_to_store_and_hash_prefixes;
   return GetPrefixMatches(check, &full_hash_to_store_and_hash_prefixes);
 }
 
 void V4LocalDatabaseManager::OnFullHashResponse(
-    std::unique_ptr<PendingCheck> pending_check,
+    std::unique_ptr<PendingCheck> check,
     const std::vector<FullHashInfo>& full_hash_infos) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (!enabled_) {
-    DCHECK(pending_clients_.empty());
+    DCHECK(pending_checks_.empty());
     return;
   }
 
-  const auto it = pending_clients_.find(pending_check->client);
-  if (it == pending_clients_.end()) {
+  const auto it = pending_checks_.find(check.get());
+  if (it == pending_checks_.end()) {
     // The check has since been cancelled.
     return;
   }
 
   // Find out the most severe threat, if any, to report to the client.
-  GetSeverestThreatTypeAndMetadata(&pending_check->result_threat_type,
-                                   &pending_check->url_metadata,
-                                   full_hash_infos);
-  pending_clients_.erase(it);
-  RespondToClient(std::move(pending_check));
+  GetSeverestThreatTypeAndMetadata(&check->result_threat_type,
+                                   &check->url_metadata, full_hash_infos);
+  pending_checks_.erase(it);
+  RespondToClient(std::move(check));
 }
 
 void V4LocalDatabaseManager::PerformFullHashCheck(
     std::unique_ptr<PendingCheck> check,
     const FullHashToStoreAndHashPrefixesMap&
         full_hash_to_store_and_hash_prefixes) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   DCHECK(enabled_);
   DCHECK(!full_hash_to_store_and_hash_prefixes.empty());
 
+  pending_checks_.insert(check.get());
+
   v4_get_hash_protocol_manager_->GetFullHashes(
       full_hash_to_store_and_hash_prefixes,
       base::Bind(&V4LocalDatabaseManager::OnFullHashResponse,
                  weak_factory_.GetWeakPtr(), base::Passed(std::move(check))));
 }
 
 void V4LocalDatabaseManager::ProcessQueuedChecks() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   // Steal the queue to protect against reentrant CancelCheck() calls.
@@ skipping 22 matching lines @@
     RespondToClient(std::move(it));
   }
 }
 
 void V4LocalDatabaseManager::RespondToClient(
     std::unique_ptr<PendingCheck> check) {
   DCHECK(check.get());
 
   if (check->client_callback_type == ClientCallbackType::CHECK_BROWSE_URL) {
     DCHECK_EQ(1u, check->urls.size());
-    // TODO(vakh): Remove these CHECKs after fixing bugs 660293, 660359.
-    CHECK(check.get());
-    CHECK(check->client);
-    CHECK_LE(1u, check->urls.size());
-    CHECK(check->urls[0].is_valid());
     check->client->OnCheckBrowseUrlResult(
         check->urls[0], check->result_threat_type, check->url_metadata);
   } else if (check->client_callback_type ==
              ClientCallbackType::CHECK_DOWNLOAD_URLS) {
     check->client->OnCheckDownloadUrlResult(check->urls,
                                             check->result_threat_type);
   } else if (check->client_callback_type ==
              ClientCallbackType::CHECK_EXTENSION_IDS) {
     const std::set<FullHash> extension_ids(check->full_hashes.begin(),
                                            check->full_hashes.end());
@@ skipping 43 matching lines @@
                             db_updated_callback_);
 }
 
 bool V4LocalDatabaseManager::AreStoresAvailableNow(
     const StoresToCheck& stores_to_check) const {
   return enabled_ && v4_database_ &&
          v4_database_->AreStoresAvailable(stores_to_check);
 }
 
 }  // namespace safe_browsing