Have a list of pending checks instead of pending clients

components/safe_browsing_db/v4_local_database_manager_unittest.cc

Index: components/safe_browsing_db/v4_local_database_manager_unittest.cc
diff --git a/components/safe_browsing_db/v4_local_database_manager_unittest.cc b/components/safe_browsing_db/v4_local_database_manager_unittest.cc
index 3b1a686197eade5b30d754b7dc9cce6710895d39..8c3701a905747d5f95eeffb584e75ec30768b7bf 100644
--- a/components/safe_browsing_db/v4_local_database_manager_unittest.cc
+++ b/components/safe_browsing_db/v4_local_database_manager_unittest.cc
@@ -105,6 +105,39 @@ class FakeV4Database : public V4Database {
   const bool stores_available_;
 };
 
+class FakeV4GetHashProtocolManager : public V4GetHashProtocolManager {
+ public:
+  FakeV4GetHashProtocolManager(
+      net::URLRequestContextGetter* request_context_getter,
+      const StoresToCheck& stores_to_check,
+      const V4ProtocolConfig& config,
+      const std::vector<FullHashInfo>& expected_response,
+      const scoped_refptr<base::TestSimpleTaskRunner>& task_runner)
+      : V4GetHashProtocolManager(request_context_getter,
+                                 stores_to_check,
+                                 config),
+        expected_response_(expected_response),
+        task_runner_(task_runner) {}
+
+  void WaitForTasksOnTaskRunner() {

[Nathan Parker, 2017/01/09 19:46:28]

Make private, or better yet just inline it.

[vakh (use Gerrit instead), 2017/01/10 01:54:53]

Removed

+    // Wait for tasks on the task runner so we're sure that the
+    // V4LocalDatabaseManager has read the data from disk.
+    task_runner_->RunPendingTasks();
+    base::RunLoop().RunUntilIdle();
+  }
+
+  void GetFullHashes(const FullHashToStoreAndHashPrefixesMap&
+                         full_hash_to_matching_hash_prefixes,
+                     FullHashCallback callback) override {
+    WaitForTasksOnTaskRunner();

[Nathan Parker, 2017/01/09 19:46:28]

I'm not clear on why this is necessary -- the one at the end of
ReplaceV4Database should ensure you're in a known state, ya?

[vakh (use Gerrit instead), 2017/01/10 01:54:53]

Removed

+    callback.Run(expected_response_);
+  }
+
+ private:
+  const std::vector<FullHashInfo> expected_response_;
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+};
+
 class TestClient : public SafeBrowsingDatabaseManager::Client {
  public:
   TestClient(SBThreatType sb_threat_type,
@@ -214,6 +247,13 @@ class V4LocalDatabaseManagerTest : public PlatformTest {
     WaitForTasksOnTaskRunner();
   }
 
+  void ReplaceV4GetHashProtocolManager(const StoresToCheck& stores_to_check) {
+    v4_local_database_manager_->v4_get_hash_protocol_manager_ =
+        base::MakeUnique<FakeV4GetHashProtocolManager>(
+            nullptr, stores_to_check, GetTestV4ProtocolConfig(),
+            std::vector<FullHashInfo>{}, task_runner_);
+  }
+
   void ResetV4Database() {
     V4Database::Destroy(std::move(v4_local_database_manager_->v4_database_));
   }
@@ -616,6 +656,38 @@ TEST_F(V4LocalDatabaseManagerTest, TestMatchModuleWhitelist) {
       v4_local_database_manager_));
 }
 
+// See http://crbug.com/660293

[Nathan Parker, 2017/01/09 19:46:28]

nit: "This verifies the fix for race in http://...

[vakh (use Gerrit instead), 2017/01/10 01:54:53]

Done.

+TEST_F(V4LocalDatabaseManagerTest, TestCheckBrowseUrlWithSameClientAndCancel) {
+  WaitForTasksOnTaskRunner();
+  net::TestURLFetcherFactory factory;
+
+  StoreAndHashPrefixes store_and_hash_prefixes;
+  store_and_hash_prefixes.emplace_back(GetUrlMalwareId(),
+                                       HashPrefix("sن\340\t\006_"));
+  ReplaceV4Database(store_and_hash_prefixes);
+  ReplaceV4GetHashProtocolManager(StoresToCheck({GetUrlMalwareId()}));
+
+  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()