Fix race condition with CancelCheck().

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"
 #include "components/safe_browsing_db/v4_database.h"
 #include "components/safe_browsing_db/v4_local_database_manager.h"
 #include "components/safe_browsing_db/v4_test_util.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "crypto/sha2.h"
 #include "net/url_request/test_url_fetcher_factory.h"
 #include "testing/platform_test.h"
 
 namespace safe_browsing {
 
 namespace {
 
 // Utility function for populating hashes.
 FullHash HashForUrl(const GURL& url) {
   std::vector<FullHash> full_hashes;
   V4ProtocolManagerUtil::UrlToFullHashes(url, &full_hashes);
   // ASSERT_GE(full_hashes.size(), 1u);
   return full_hashes[0];
 }
 
+// A fullhash response containing no matches.
+std::string GetEmptyV4HashResponse() {
+  FindFullHashesResponse res;
+  res.mutable_negative_cache_duration()->set_seconds(600);
+
+  std::string res_data;
+  res.SerializeToString(&res_data);
+  return res_data;
+}
+
 }  // namespace
 
 class FakeV4Database : public V4Database {
  public:
   static void Create(
       const scoped_refptr<base::SequencedTaskRunner>& db_task_runner,
       std::unique_ptr<StoreMap> store_map,
       const StoreAndHashPrefixes& store_and_hash_prefixes,
       NewDatabaseReadyCallback new_db_callback,
       bool stores_available) {
@@ skipping 49 matching lines @@
       : 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 TestClient : public SafeBrowsingDatabaseManager::Client {
  public:
-  TestClient(SBThreatType sb_threat_type, const GURL& url)
-      : expected_sb_threat_type(sb_threat_type), expected_url(url) {}
+  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) {}
 
   void OnCheckBrowseUrlResult(const GURL& url,
                               SBThreatType threat_type,
                               const ThreatMetadata& metadata) override {
     DCHECK_EQ(expected_url, url);
     DCHECK_EQ(expected_sb_threat_type, threat_type);
+    result_received_ = true;
+    if (manager_to_cancel_) {
+      manager_to_cancel_->CancelCheck(this);
+    }
   }
 
   SBThreatType expected_sb_threat_type;
   GURL expected_url;
+  bool result_received_;
+  V4LocalDatabaseManager* manager_to_cancel_;
 };
 
 class FakeV4LocalDatabaseManager : public V4LocalDatabaseManager {
  public:
   void PerformFullHashCheck(std::unique_ptr<PendingCheck> check,
                             const FullHashToStoreAndHashPrefixesMap&
                                 full_hash_to_store_and_hash_prefixes) override {
     perform_full_hash_check_called_ = true;
   }
 
@@ skipping 220 matching lines @@
 
   ResetV4Database();
   v4_local_database_manager_->CheckBrowseUrl(url, &client);
   // The database is unavailable so the check should get queued.
   EXPECT_EQ(1ul, GetQueuedChecks().size());
 
   StopLocalDatabaseManager();
   EXPECT_TRUE(GetQueuedChecks().empty());
 }
 
+// Verify that a window where checks cannot be cancelled is closed.
+TEST_F(V4LocalDatabaseManagerTest, CancelPending) {
+  WaitForTasksOnTaskRunner();
+  net::FakeURLFetcherFactory factory(NULL);
+  // TODO(shess): Modify this to use a mock protocol manager instead

[vakh (use Gerrit instead), 2017/01/05 18:31:41]

you can change this to vakh, if you like and don't mind.

+  // of faking the requests.
+  const char* kReqs[] = {

[Scott Hess - ex-Googler, 2017/01/06 22:44:36]

QQ ... I kinda messed up my emacs init.el, so was having to do formatting
manually rather than using google-c-style.el.  So when git-cl said to run
git-cl-format to reform this, and it made it four-space indent, I was like
"What?  Maybe I just don't remember things right."

But now I've fixed things, and google-c-style.el definitely wants these to be
two-space indented.  Did something change?

+      // OSX
+      "Cg8KCHVuaXR0ZXN0EgMxLjAaJwgBCAIIAwgGCAcICAgJCAoQBBAIGgcKBWVXGg-"

[Nathan Parker, 2017/01/05 21:35:32]

wat.  Proto wire formats vary by platform?  Isn't that the point of... a
standard format?

Does this print the request if one is not found?  That'll make it easy to fix at
least.

Really the Factory needs a "SafeFakeDefaultResponse" method.  Really.

[Scott Hess - ex-Googler, 2017/01/05 23:23:12]

The proto is portable - but each platform puts a different type in there
(LINUX_PLATFORM, WINDOWS_PLATFORM, etc).
Yeah, I pulled these together from trybot logs.
Or a prefix-based version or a callback-based version.  But at least the
callback-based option seems so obvious that I have to imagine they explicitly
decided against it.

+      "pIAEgAyAEIAUgBg==",
+
+      // Linux
+      "Cg8KCHVuaXR0ZXN0EgMxLjAaJwgBCAIIAwgGCAcICAgJCAoQAhAIGgcKBWVXGg-"
+      "pIAEgAyAEIAUgBg==",
+
+      // Windows
+      "Cg8KCHVuaXR0ZXN0EgMxLjAaJwgBCAIIAwgGCAcICAgJCAoQARAIGgcKBWVXGg-"
+      "pIAEgAyAEIAUgBg==",
+  };
+  for (const char* req : kReqs) {
+    const GURL url(
+        base::StringPrintf("https://safebrowsing.googleapis.com/v4/"
+                           "fullHashes:find?$req=%s"
+                           "&$ct=application/x-protobuf&key=test_key_param",
+                           req));
+    factory.SetFakeResponse(url, GetEmptyV4HashResponse(), net::HTTP_OK,
+                            net::URLRequestStatus::SUCCESS);
+  }
+
+  const GURL url("http://example.com/a/");
+  const HashPrefix hash_prefix("eW\x1A\xF\xA9");
+
+  StoreAndHashPrefixes store_and_hash_prefixes;
+  store_and_hash_prefixes.emplace_back(GetUrlMalwareId(), hash_prefix);
+  ReplaceV4Database(store_and_hash_prefixes);
+
+  // Test that a request flows through to the callback.
+  {
+    TestClient client(SB_THREAT_TYPE_SAFE, url);
+    EXPECT_FALSE(v4_local_database_manager_->CheckBrowseUrl(url, &client));
+    EXPECT_FALSE(client.result_received_);
+    WaitForTasksOnTaskRunner();
+    EXPECT_TRUE(client.result_received_);
+  }
+
+  // Test that cancel prevents the callback from being called.
+  {
+    TestClient client(SB_THREAT_TYPE_SAFE, url);
+    EXPECT_FALSE(v4_local_database_manager_->CheckBrowseUrl(url, &client));
+    v4_local_database_manager_->CancelCheck(&client);
+    EXPECT_FALSE(client.result_received_);
+    WaitForTasksOnTaskRunner();
+    EXPECT_FALSE(client.result_received_);
+  }
+}
+
+// When the database load flushes the queued requests, make sure that
+// CancelCheck() is not fatal in the client callback.
+TEST_F(V4LocalDatabaseManagerTest, CancelQueued) {
+  const GURL url("http://example.com/a/");
+
+  TestClient client1(SB_THREAT_TYPE_SAFE, url,
+                     v4_local_database_manager_.get());
+  TestClient client2(SB_THREAT_TYPE_SAFE, url);
+  EXPECT_FALSE(v4_local_database_manager_->CheckBrowseUrl(url, &client1));
+  EXPECT_FALSE(v4_local_database_manager_->CheckBrowseUrl(url, &client2));
+  EXPECT_EQ(2ul, GetQueuedChecks().size());
+  EXPECT_FALSE(client1.result_received_);
+  EXPECT_FALSE(client2.result_received_);
+  WaitForTasksOnTaskRunner();
+  EXPECT_TRUE(client1.result_received_);
+  EXPECT_TRUE(client2.result_received_);
+}
+
 // This test is somewhat similar to TestCheckBrowseUrlWithFakeDbReturnsMatch but
 // it uses a fake V4LocalDatabaseManager to assert that PerformFullHashCheck is
 // called async.
 TEST_F(V4LocalDatabaseManagerTest, PerformFullHashCheckCalledAsync) {
   SetupFakeManager();
   net::TestURLFetcherFactory factory;
 
   StoreAndHashPrefixes store_and_hash_prefixes;
   store_and_hash_prefixes.emplace_back(GetUrlMalwareId(),
                                        HashPrefix("eW\x1A\xF\xA9"));
@@ skipping 158 matching lines @@
   EXPECT_FALSE(FakeV4LocalDatabaseManager::PerformFullHashCheckCalled(
       v4_local_database_manager_));
 }
 
 // TODO(nparker): Add tests for
 //   CheckDownloadUrl()
 //   CheckExtensionIDs()
 //   CheckResourceUrl()
 
 }  // namespace safe_browsing