Have a list of pending checks instead of pending clients

components/safe_browsing_db/v4_local_database_manager.cc

Index: components/safe_browsing_db/v4_local_database_manager.cc
diff --git a/components/safe_browsing_db/v4_local_database_manager.cc b/components/safe_browsing_db/v4_local_database_manager.cc
index 5bd770286d8d24e3759bb7f098570b663712bf58..cef4f53d82c53e5e448b75f1606545bf61b74264 100644
--- a/components/safe_browsing_db/v4_local_database_manager.cc
+++ b/components/safe_browsing_db/v4_local_database_manager.cc
@@ -160,9 +160,11 @@ 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 =
@@ -375,7 +377,7 @@ void V4LocalDatabaseManager::StopOnIOThread(bool shutdown) {
 
   enabled_ = false;
 
-  pending_clients_.clear();
+  pending_checks_.clear();
 
   RespondSafeToQueuedChecks();
 
@@ -534,8 +536,8 @@ bool V4LocalDatabaseManager::HandleCheck(std::unique_ptr<PendingCheck> check) {
     return true;
   }
 
-  // Post on the IO thread to enforce async behavior.
   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.
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&V4LocalDatabaseManager::PerformFullHashCheck, this,
@@ -572,27 +574,26 @@ bool V4LocalDatabaseManager::HandleUrlSynchronously(
 }
 
 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(
@@ -604,6 +605,8 @@ void V4LocalDatabaseManager::PerformFullHashCheck(
   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,
@@ -646,11 +649,6 @@ void V4LocalDatabaseManager::RespondToClient(
 
   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 ==