Have a list of pending checks instead of pending clients since checks are

unique, but clients aren't.

Also, dump whether the SB check for the URL returned by the DBManager had timed
out.
Also, make the dump buffer smaller hoping to get more minidumps.

BUG=660293

Review-Url: https://codereview.chromium.org/2616653002
Cr-Commit-Position: refs/heads/master@{#442731}

components/safe_browsing/base_safe_browsing_resource_throttle.cc

@@ -238,9 +238,10 @@ void BaseSafeBrowsingResourceThrottle::OnCheckBrowseUrlResult(
   CHECK(url.is_valid());
   CHECK(url_being_checked_.is_valid());
   if (url != url_being_checked_) {
-    char buf[2000];
-    snprintf(buf, sizeof(buf), "sbtr::ocbur:%s -- %s\n", url.spec().c_str(),
-             url_being_checked_.spec().c_str());
+    bool url_had_timed_out = timed_out_urls_.count(url) > 0;
+    char buf[1000];
+    snprintf(buf, sizeof(buf), "sbtr::ocbur:%d:%s -- %s\n", url_had_timed_out,
+             url.spec().c_str(), url_being_checked_.spec().c_str());
     base::debug::Alias(buf);
     CHECK(false) << "buf: " << buf;
   }
@@ -413,6 +414,8 @@ void BaseSafeBrowsingResourceThrottle::OnCheckUrlTimeout() {
 
   OnCheckBrowseUrlResult(url_being_checked_, safe_browsing::SB_THREAT_TYPE_SAFE,
                          safe_browsing::ThreatMetadata());
+
+  timed_out_urls_.insert(url_being_checked_);
 }
 
 void BaseSafeBrowsingResourceThrottle::ResumeRequest() {

components/safe_browsing/base_safe_browsing_resource_throttle.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_SAFE_BROWSING_BASE_SAFE_BROWSING_RESOURCE_THROTTLE_H_
 #define COMPONENTS_SAFE_BROWSING_BASE_SAFE_BROWSING_RESOURCE_THROTTLE_H_
 
+#include <set>
 #include <string>
 #include <vector>
 
@@ -172,6 +173,11 @@ class BaseSafeBrowsingResourceThrottle
   const content::ResourceType resource_type_;
   net::NetLogWithSource net_log_with_source_;
 
+  // TODO(vakh): The following set should be removed after fixing
+  // http://crbug.com/660293
+  // URLs that timed out waiting for a SafeBrowsing reputation check.
+  std::set<GURL> timed_out_urls_;
+
   DISALLOW_COPY_AND_ASSIGN(BaseSafeBrowsingResourceThrottle);
 };
 

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; });
+  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,12 @@ bool V4LocalDatabaseManager::HandleCheck(std::unique_ptr<PendingCheck> check) {
     return true;
   }
 
+  // Add check to pending_checks_ before scheduling PerformFullHashCheck so that
+  // even if the client calls CancelCheck before PerformFullHashCheck gets
+  // called, the check can be found in pending_checks_.
+  pending_checks_.insert(check.get());
+
   // 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,
@@ -572,27 +578,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(
@@ -622,7 +627,7 @@ void V4LocalDatabaseManager::ProcessQueuedChecks() {
     if (!GetPrefixMatches(it, &full_hash_to_store_and_hash_prefixes)) {
       RespondToClient(std::move(it));
     } else {
-      pending_clients_.insert(it->client);
+      pending_checks_.insert(it.get());
       PerformFullHashCheck(std::move(it), full_hash_to_store_and_hash_prefixes);
     }
   }
@@ -646,11 +651,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 ==

components/safe_browsing_db/v4_local_database_manager.h

@@ -9,6 +9,7 @@
 // and database that holds the downloaded updates.
 
 #include <memory>
+#include <unordered_set>
 
 #include "base/memory/weak_ptr.h"
 #include "components/safe_browsing_db/database_manager.h"
@@ -160,9 +161,8 @@ class V4LocalDatabaseManager : public SafeBrowsingDatabaseManager {
   FRIEND_TEST_ALL_PREFIXES(V4LocalDatabaseManagerTest,
                            TestGetSeverestThreatTypeAndMetadata);
 
-  // The set of clients awaiting a full hash response. It is used for tracking
-  // which clients have cancelled their outstanding request.
-  typedef std::unordered_set<const Client*> PendingClients;
+  // The checks awaiting a full hash response from SafeBrowsing service.
+  typedef std::unordered_set<const PendingCheck*> PendingChecks;
 
   // Called when all the stores managed by the database have been read from
   // disk after startup and the database is ready for checking resource
@@ -269,9 +269,8 @@ class V4LocalDatabaseManager : public SafeBrowsingDatabaseManager {
   // name of the file on disk that would contain the prefixes, if applicable.
   ListInfos list_infos_;
 
-  // The set of clients that are waiting for a full hash response from the
-  // SafeBrowsing service.
-  PendingClients pending_clients_;
+  // The checks awaiting for a full hash response from the SafeBrowsing service.
+  PendingChecks pending_checks_;
 
   // The checks that need to be scheduled when the database becomes ready for
   // use.

components/safe_browsing_db/v4_local_database_manager_unittest.cc

@@ -641,6 +641,39 @@ TEST_F(V4LocalDatabaseManagerTest, TestMatchModuleWhitelist) {
       v4_local_database_manager_));
 }
 
+// This verifies the fix for race in http://crbug.com/660293
+TEST_F(V4LocalDatabaseManagerTest, TestCheckBrowseUrlWithSameClientAndCancel) {
+  ScopedFakeGetHashProtocolManagerFactory pin;
+  // Reset the database manager so it picks up the replacement protocol manager.
+  ResetLocalDatabaseManager();
+  WaitForTasksOnTaskRunner();
+
+  StoreAndHashPrefixes store_and_hash_prefixes;
+  store_and_hash_prefixes.emplace_back(GetUrlMalwareId(),
+                                       HashPrefix("sن\340\t\006_"));
+  ReplaceV4Database(store_and_hash_prefixes);
+
+  GURL first_url("http://example.com/a");
+  GURL second_url("http://example.com/");
+  TestClient client(SB_THREAT_TYPE_SAFE, first_url);
+  // The fake database returns a matched hash prefix.
+  EXPECT_FALSE(v4_local_database_manager_->CheckBrowseUrl(first_url, &client));
+
+  // That check gets queued. Now, let's cancel the check. After this, we should
+  // not receive a call for |OnCheckBrowseUrlResult| with |first_url|.
+  v4_local_database_manager_->CancelCheck(&client);
+
+  // Now, re-use that client but for |second_url|.
+  client.expected_url = second_url;
+  EXPECT_FALSE(v4_local_database_manager_->CheckBrowseUrl(second_url, &client));
+
+  // Wait for PerformFullHashCheck to complete.
+  WaitForTasksOnTaskRunner();
+  // |result_received_| is true only if OnCheckBrowseUrlResult gets called with
+  // the |url| equal to |expected_url|, which is |second_url| in this test.
+  EXPECT_TRUE(client.result_received_);
+}
+
 // TODO(nparker): Add tests for
 //   CheckDownloadUrl()
 //   CheckExtensionIDs()