Fix race condition with CancelCheck().

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 3b03db0eb9fae6afb5949fb9bb63eadf70048838..3b1a686197eade5b30d754b7dc9cce6710895d39 100644
--- a/components/safe_browsing_db/v4_local_database_manager_unittest.cc
+++ b/components/safe_browsing_db/v4_local_database_manager_unittest.cc
@@ -6,6 +6,7 @@
 #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"
@@ -27,6 +28,16 @@ FullHash HashForUrl(const GURL& url) {
   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 {
@@ -96,18 +107,29 @@ class FakeV4Database : public V4Database {
 
 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 {
@@ -348,6 +370,80 @@ TEST_F(V4LocalDatabaseManagerTest, TestChecksAreQueued) {
   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.