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/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 "net/url_request/test_url_fetcher_factory.h"
 #include "testing/platform_test.h"
 
 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(
       const scoped_refptr<base::SequencedTaskRunner>& db_task_runner,
       std::unique_ptr<StoreMap> store_map,
       const StoreAndHashPrefixes& store_and_hash_prefixes,
       NewDatabaseReadyCallback new_db_callback) {
     // Mimics V4Database::Create
     const scoped_refptr<base::SingleThreadTaskRunner>& callback_task_runner =
         base::MessageLoop::current()->task_runner();
@@ skipping 36 matching lines @@
                  std::unique_ptr<StoreMap> store_map,
                  const StoreAndHashPrefixes& store_and_hash_prefixes)
       : V4Database(db_task_runner, std::move(store_map)),
         store_and_hash_prefixes_(store_and_hash_prefixes) {}
 
   const StoreAndHashPrefixes store_and_hash_prefixes_;
 };
 
 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 {
  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 206 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);
+  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.
 TEST_F(V4LocalDatabaseManagerTest, PerformFullHashCheckCalledAsync) {
   // StopLocalDatabaseManager before resetting it because that's what
   // ~V4LocalDatabaseManager expects.
   StopLocalDatabaseManager();
   v4_local_database_manager_ =
       make_scoped_refptr(new FakeV4LocalDatabaseManager(base_dir_.GetPath()));
   SetTaskRunnerForTest();
@@ skipping 78 matching lines @@
   EXPECT_FALSE(FakeV4LocalDatabaseManager::PerformFullHashCheckCalled(
       v4_local_database_manager_));
 
   // The fake database returns a matched hash prefix.
   EXPECT_TRUE(v4_local_database_manager_->MatchMalwareIP("192.168.1.2"));
   EXPECT_FALSE(FakeV4LocalDatabaseManager::PerformFullHashCheckCalled(
       v4_local_database_manager_));
 }
 
 }  // namespace safe_browsing