Fix race condition with CancelCheck().

components/safe_browsing_db/v4_local_database_manager.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.
 
 // This file should not be build on Android but is currently getting built.
 // TODO(vakh): Fix that: http://crbug.com/621647
 
 #include "components/safe_browsing_db/v4_local_database_manager.h"
 
 #include <vector>
@@ skipping 498 matching lines @@
     queued_checks_.push_back(std::move(check));
     return false;
   }
 
   FullHashToStoreAndHashPrefixesMap full_hash_to_store_and_hash_prefixes;
   if (!GetPrefixMatches(check, &full_hash_to_store_and_hash_prefixes)) {
     return true;
   }
 
   // Post on the IO thread to enforce async behavior.
+  pending_clients_.insert(check->client);
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&V4LocalDatabaseManager::PerformFullHashCheck, this,
                  base::Passed(std::move(check)),
                  full_hash_to_store_and_hash_prefixes));
 
   return false;
 }
 
 bool V4LocalDatabaseManager::HandleHashSynchronously(
@@ skipping 46 matching lines @@
   RespondToClient(std::move(pending_check));
 }
 
 void V4LocalDatabaseManager::PerformFullHashCheck(
     std::unique_ptr<PendingCheck> check,
     const FullHashToStoreAndHashPrefixesMap&
         full_hash_to_store_and_hash_prefixes) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   DCHECK(enabled_);
   DCHECK(!full_hash_to_store_and_hash_prefixes.empty());

[Nathan Parker, 2016/12/14 18:03:03]

Maybe add a comment that the caller should have added the client to
pending_clients_?  

[Scott Hess - ex-Googler, 2016/12/14 20:45:58]

Should have, but if CancelCheck() was called between task post and task run, it
won't be there.  I suppose this code could check if cancel occurred and drop the
request.  Not sure if that's worth additional code on every request to catch an
edge case which probably never happens, but pending_checks_ should be pretty hot
and empty, right?

 
-  pending_clients_.insert(check->client);
-
   v4_get_hash_protocol_manager_->GetFullHashes(
       full_hash_to_store_and_hash_prefixes,
       base::Bind(&V4LocalDatabaseManager::OnFullHashResponse,
                  weak_factory_.GetWeakPtr(), base::Passed(std::move(check))));
 }
 
 void V4LocalDatabaseManager::ProcessQueuedChecks() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  for (auto& it : queued_checks_) {
+
+  // Steal the queue to protect against reentrant CancelCheck() calls.

[Nathan Parker, 2016/12/14 18:03:04]

Is this if they call Cancel() from within RespondToClient -->
client->OnCheckBrowseUrl? Otherwise, the Cancel call has to come on the IO
thread so it wouldn't happen here.

[Scott Hess - ex-Googler, 2016/12/14 20:45:58]

Yes, that is my concern.  We're calling out to external code and relying on it
to not make certain calls which would disrupt internal structures while in use. 
I don't _think_ any clients chain requests together oddly (with one response
cancelling a different request), but I think this case could even come up if the
client being processed cancels itself (perhaps in destruction).

In both functions, we could run past the end of the queue, so lacking any
diagnostics complaining about that probably indicates it's not happening.  But I
don't know how the foreach iterator works if the container changes contents in
the body.

[Scott Hess - ex-Googler, 2016/12/14 20:56:33]

OK, it looks like for(:) implementation expects to be able to cache end(), so it
does not adjust to operations such as erase.  So the prior code is plausibly
incorrect, but the fact that we've not seen crashes suggesting that we're
processing past the end of the queue probably means it never comes up in
practice.

That said, swap-to-temporary should be approximately the same cost.  Maybe even
a bit better, since it gets rid of the space for items in queued_checks_, which
will reduce Chrome's memory usage by a trillionth.

+  QueuedChecks checks;
+  checks.swap(queued_checks_);
+
+  for (auto& it : checks) {
     FullHashToStoreAndHashPrefixesMap full_hash_to_store_and_hash_prefixes;
     if (!GetPrefixMatches(it, &full_hash_to_store_and_hash_prefixes)) {
       RespondToClient(std::move(it));

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

Ha, it took some work, but the amusing result is that this code is moving the
pointer from the vector, so if RespondToClient() leads to a CancelCheck() call,
you get a crash because of a NULL-deref.

     } else {
+      pending_clients_.insert(it->client);
       PerformFullHashCheck(std::move(it), full_hash_to_store_and_hash_prefixes);
     }
   }
-  queued_checks_.clear();
 }
 
 void V4LocalDatabaseManager::RespondSafeToQueuedChecks() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  for (std::unique_ptr<PendingCheck>& it : queued_checks_) {
+
+  // Steal the queue to protect against reentrant CancelCheck() calls.
+  QueuedChecks checks;
+  checks.swap(queued_checks_);
+
+  for (std::unique_ptr<PendingCheck>& it : checks) {
     RespondToClient(std::move(it));
   }
-  queued_checks_.clear();
 }
 
 void V4LocalDatabaseManager::RespondToClient(
     std::unique_ptr<PendingCheck> check) {
   DCHECK(check.get());
 
   if (check->client_callback_type == ClientCallbackType::CHECK_BROWSE_URL) {
     DCHECK_EQ(1u, check->urls.size());
     // TODO(vakh): Remove these CHECKs after fixing bugs 660293, 660359.
     CHECK(check.get());
@@ skipping 50 matching lines @@
 }
 
 void V4LocalDatabaseManager::UpdateRequestCompleted(
     std::unique_ptr<ParsedServerResponse> parsed_server_response) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   v4_database_->ApplyUpdate(std::move(parsed_server_response),
                             db_updated_callback_);
 }
 
 }  // namespace safe_browsing