Fix race condition with CancelCheck().

components/safe_browsing_db/v4_local_database_manager.cc

Index: components/safe_browsing_db/v4_local_database_manager.cc
diff --git a/components/safe_browsing_db/v4_local_database_manager.cc b/components/safe_browsing_db/v4_local_database_manager.cc
index ce0831247ec8a796607415a0af839678f6ba3522..5c91e34b9b1b05d0bcdbdc6da288eecf12fd0146 100644
--- a/components/safe_browsing_db/v4_local_database_manager.cc
+++ b/components/safe_browsing_db/v4_local_database_manager.cc
@@ -516,6 +516,7 @@ bool V4LocalDatabaseManager::HandleCheck(std::unique_ptr<PendingCheck> check) {
   }
 
   // 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,
@@ -584,8 +585,6 @@ void V4LocalDatabaseManager::PerformFullHashCheck(
   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,
@@ -594,23 +593,32 @@ void V4LocalDatabaseManager::PerformFullHashCheck(
 
 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(