Have a list of pending checks instead of pending clients

components/safe_browsing_db/v4_local_database_manager_unittest.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.
 
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "base/test/test_simple_task_runner.h"
@@ skipping 87 matching lines @@
                  const StoreAndHashPrefixes& store_and_hash_prefixes,
                  bool stores_available)
       : V4Database(db_task_runner, std::move(store_map)),
         store_and_hash_prefixes_(store_and_hash_prefixes),
         stores_available_(stores_available) {}
 
   const StoreAndHashPrefixes store_and_hash_prefixes_;
   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,
              const GURL& url,
              V4LocalDatabaseManager* manager_to_cancel = nullptr)
       : expected_sb_threat_type(sb_threat_type),
         expected_url(url),
         result_received_(false),
         manager_to_cancel_(manager_to_cancel) {}
 
@@ skipping 89 matching lines @@
 
     NewDatabaseReadyCallback db_ready_callback =
         base::Bind(&V4LocalDatabaseManager::DatabaseReadyForChecks,
                    base::Unretained(v4_local_database_manager_.get()));
     FakeV4Database::Create(task_runner_, base::MakeUnique<StoreMap>(),
                            store_and_hash_prefixes, db_ready_callback,
                            stores_available);
     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_));
   }
 
   void SetTaskRunnerForTest() {
     v4_local_database_manager_->SetTaskRunnerForTest(task_runner_);
   }
 
   void StartLocalDatabaseManager() {
     v4_local_database_manager_->StartOnIOThread(NULL,
@@ skipping 382 matching lines @@
   EXPECT_FALSE(
       v4_local_database_manager_->MatchModuleWhitelistString("badstuff.dll"));
   // Whitelisted.
   EXPECT_TRUE(
       v4_local_database_manager_->MatchModuleWhitelistString("chrome.dll"));
 
   EXPECT_FALSE(FakeV4LocalDatabaseManager::PerformFullHashCheckCalled(
       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()
 //   CheckResourceUrl()
 
 }  // namespace safe_browsing