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 7e8c4d6d134e07301ffd183a4c376d6d626c5889..a98e0b0586e2bfc9de95ba0fec8cde0f5be9040b 100644
--- a/components/safe_browsing_db/v4_local_database_manager_unittest.cc
+++ b/components/safe_browsing_db/v4_local_database_manager_unittest.cc
@@ -16,6 +16,20 @@
 
 namespace safe_browsing {
 
+namespace {
+
+// 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(
@@ -72,18 +86,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);

[Scott Hess - ex-Googler, 2016/12/15 01:14:13]

Any CancelCheck() with items in the queue during queue flushing causes problems.
 So I didn't go for something more realistic, like a closure to cancel against
the second client.

[Nathan Parker, 2016/12/16 22:37:13]

Acknowledged.

+    }
   }
 
   SBThreatType expected_sb_threat_type;
   GURL expected_url;
+  bool result_received_;
+  V4LocalDatabaseManager* manager_to_cancel_;
 };
 
 class FakeV4LocalDatabaseManager : public V4LocalDatabaseManager {
@@ -310,6 +335,62 @@ 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);
+  factory.SetFakeResponse(
+      GURL("https://safebrowsing.googleapis.com/v4/"
+           "fullHashes:find?$req="
+           "Cg8KCHVuaXR0ZXN0EgMxLjAaJwgBCAIIAwgGCAcICAgJCAoQBBAIGgcKBWVXGg-"
+           "pIAEgAyAEIAUgBg==&$ct=application/x-protobuf&key=test_key_param"),

[Scott Hess - ex-Googler, 2016/12/15 01:14:13]

I'd love suggestions about this.  AFAICT, FakeURLFetcherFactory can't bind a
prefix, it has to have the entire URL.  Doesn't seem to have a generic fallback
handler, either.  The best I could think of would be to expose the
URL-construction API so that this code could construct "The URL for the
appropriate fullhash", which also seemed intrusive.

[Or maybe I missed something about mocking the get-hash-protocol-manager.]

[Nathan Parker, 2016/12/16 22:37:13]

Matching a prefix or having default seems like it'd be pretty useful, I agree. I
think you could use a TestUrlFetcherFactory and then call OnURLFetchComplete on
the right fetcher, with the response. But that would run it on the test's
thread.

There is V4GetHashProtocolManager::RegisterFactory(..) which looks like it was
intended for this purpose, but was never used. You could create a mock or just
override the protocol-manager such that when GetFullHashes(..) is called, it
stashes the callback so you could invoke it later.

[Scott Hess - ex-Googler, 2017/01/05 00:02:14]

Varun, did you have an opinion on this hard-coded URL?

I'm somewhat inclined to just check things in and aggressively disable if
something goes awry, since I worry that if I step out to add more mocking, I'll
entirely forget what I'm doing!

+      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_);

[Scott Hess - ex-Googler, 2016/12/15 01:14:13]

Pre-patch, this expectation failed.

[Nathan Parker, 2016/12/16 22:37:13]

Wait, that seems to imply that all cancels were broken?

[vakh (use Gerrit instead), 2017/01/03 22:26:01]

No, it means that any CancelCheck calls made between calling CheckBrowseUrl and
execution of PerformFullHashChecks were broken.

+  }
+}
+
+// When the database load flushes the queued requests, make sure that
+// CancelCheck() is not fatal in the client callback.

[Scott Hess - ex-Googler, 2016/12/15 01:14:13]

This does not test the RespondSafeToQueuedChecks() path, which happens if
StopOnIOThread() is called with queued checks.  In that case, the code would
have to work around the DCHECK(enabled_) in CancelCheck().

[Nathan Parker, 2016/12/16 22:37:13]

Probably fine

+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.