Fix a problem where queued checks' clients would never be called back if when the check finally ran it passed the prefix check (the common case). This was made more noticeable by r31950 since before that things wouldn't be queued unless we were resetting the database (rare) or caught a narrow timing window where |database_| was true but |database_loaded_| was false; after that, we could get this on startup, and with r32136, after closing the database as well.

This also removes some unnecessary bits now that we don't queue during a reset.

BUG=23400
TEST=none
Review URL: http://codereview.chromium.org/402004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@32212 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 74 insertions, 44 deletions

diff --git a/chrome/browser/safe_browsing/safe_browsing_service.cc b/chrome/browser/safe_browsing/safe_browsing_service.cc
index c829c50..a119e09 100644
--- a/chrome/browser/safe_browsing/safe_browsing_service.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_service.cc
@@ -108,6 +108,9 @@ bool SafeBrowsingService::CheckUrl(const GURL& url, Client* client) {
 void SafeBrowsingService::CancelCheck(Client* client) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
   for (CurrentChecks::iterator i = checks_.begin(); i != checks_.end(); ++i) {
+    // We can't delete matching checks here because the db thread has a copy of
+    // the pointer.  Instead, we simply NULL out the client, and when the db
+    // thread calls us back, we'll clean up the check.
     if ((*i)->client == client)
       (*i)->client = NULL;
   }
@@ -115,9 +118,13 @@ void SafeBrowsingService::CancelCheck(Client* client) {
   // Scan the queued clients store. Clients may be here if they requested a URL
   // check before the database has finished loading.
   for (std::deque<QueuedCheck>::iterator it(queued_checks_.begin());
-       it != queued_checks_.end(); ++it) {
+       it != queued_checks_.end(); ) {
+    // In this case it's safe to delete matches entirely since nothing has a
+    // pointer to them.
     if (it->client == client)
-      it->client = NULL;
+      it = queued_checks_.erase(it);
+    else
+      ++it;
   }
 }
 
@@ -264,12 +271,25 @@ void SafeBrowsingService::RegisterPrefs(PrefService* prefs) {
 void SafeBrowsingService::CloseDatabase() {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
 
-  // It's important not to queue up a second request; if we did, then
-  // |closing_database_| would be reset after handling the first request, and if
-  // any functions on the db thread recreated the database, we could start using
-  // it on the IO thread and then have the second request handler delete it out
-  // from under us.
-  if (closing_database_ || !database_)
+  // Cases to avoid:
+  //  * If |closing_database_| is true, continuing will queue up a second
+  //    request, |closing_database_| will be reset after handling the first
+  //    request, and if any functions on the db thread recreate the database, we
+  //    could start using it on the IO thread and then have the second request
+  //    handler delete it out from under us.
+  //  * If |database_| is NULL, then either no creation request is in flight, in
+  //    which case we don't need to do anything, or one is in flight, in which
+  //    case the database will be recreated before our deletion request is
+  //    handled, and could be used on the IO thread in that time period, leading
+  //    to the same problem as above.
+  //  * If |queued_checks_| is non-empty and |database_| is non-NULL, we're
+  //    about to be called back (in DatabaseLoadComplete()).  This will call
+  //    CheckUrl(), which will want the database.  Closing the database here
+  //    would lead to an infinite loop in DatabaseLoadComplete(), and even if it
+  //    didn't, it would be pointless since we'd just want to recreate.
+  //
+  // The first two cases above are handled by checking database_available().
+  if (!database_available() || !queued_checks_.empty())
     return;
 
   closing_database_ = true;
@@ -341,6 +361,15 @@ void SafeBrowsingService::OnIOShutdown() {
   delete protocol_manager_;
   protocol_manager_ = NULL;
 
+  // Delete queued checks, calling back any clients with 'URL_SAFE'.
+  // If we don't do this here we may fail to close the database below.
+  while (!queued_checks_.empty()) {
+    QueuedCheck check = queued_checks_.front();
+    if (check.client)
+      check.client->OnUrlCheckResult(check.url, URL_SAFE);
+    queued_checks_.pop_front();
+  }
+
   // Close the database.  We don't simply DeleteSoon() because if a close is
   // already pending, we'll double-free, and we don't set |database_| to NULL
   // because if there is still anything running on the db thread, it could
@@ -355,15 +384,9 @@ void SafeBrowsingService::OnIOShutdown() {
   // See comments on the declaration of |safe_browsing_thread_|.
   safe_browsing_thread_.reset();
 
-  // Delete queued and pending checks once the database thread is done, calling
-  // back any clients with 'URL_SAFE'.
-  while (!queued_checks_.empty()) {
-    QueuedCheck check = queued_checks_.front();
-    if (check.client)
-      check.client->OnUrlCheckResult(check.url, URL_SAFE);
-    queued_checks_.pop_front();
-  }
-
+  // Delete pending checks, calling back any clients with 'URL_SAFE'.  We have
+  // to do this after the db thread returns because methods on it can have
+  // copies of these pointers, so deleting them might lead to accessing garbage.
   for (CurrentChecks::iterator it = checks_.begin();
        it != checks_.end(); ++it) {
     if ((*it)->client)
@@ -383,7 +406,7 @@ bool SafeBrowsingService::MakeDatabaseAvailable() {
     safe_browsing_thread_->message_loop()->PostTask(FROM_HERE,
         NewRunnableMethod(this, &SafeBrowsingService::GetDatabase));
   }
-  return !closing_database_ && (database_ != NULL);
+  return database_available();
 }
 
 SafeBrowsingDatabase* SafeBrowsingService::GetDatabase() {
@@ -497,8 +520,23 @@ void SafeBrowsingService::DatabaseLoadComplete() {
   if (!enabled_)
     return;
 
-  // If we have any queued requests, we can now check them.
-  RunQueuedClients();
+  HISTOGRAM_COUNTS("SB.QueueDepth", queued_checks_.size());
+  if (queued_checks_.empty())
+    return;
+
+  // If the database isn't already available, calling CheckUrl() in the loop
+  // below will add the check back to the queue, and we'll infinite-loop.
+  DCHECK(database_available());
+  while (!queued_checks_.empty()) {
+    QueuedCheck check = queued_checks_.front();
+    HISTOGRAM_TIMES("SB.QueueDelay", Time::Now() - check.start);
+    // If CheckUrl() determines the URL is safe immediately, it doesn't call the
+    // client's handler function (because normally it's being directly called by
+    // the client).  Since we're not the client, we have to convey this result.
+    if (check.client && CheckUrl(check.url, check.client))
+      check.client->OnUrlCheckResult(check.url, URL_SAFE);
+    queued_checks_.pop_front();
+  }
 }
 
 void SafeBrowsingService::HandleChunkForDatabase(
@@ -568,7 +606,7 @@ void SafeBrowsingService::Start() {
 
 void SafeBrowsingService::OnCloseDatabase() {
   DCHECK(MessageLoop::current() == safe_browsing_thread_->message_loop());
-  DCHECK(closing_database_);
+  DCHECK(database_available());
 
   // Because |closing_database_| is true, nothing on the IO thread will be
   // accessing the database, so it's safe to delete and then NULL the pointer.
@@ -580,9 +618,6 @@ void SafeBrowsingService::OnCloseDatabase() {
 void SafeBrowsingService::OnResetDatabase() {
   DCHECK(MessageLoop::current() == safe_browsing_thread_->message_loop());
   GetDatabase()->ResetDatabase();
-  ChromeThread::PostTask(
-      ChromeThread::IO, FROM_HERE,
-      NewRunnableMethod(this, &SafeBrowsingService::DatabaseLoadComplete));
 }
 
 void SafeBrowsingService::CacheHashResults(
@@ -704,14 +739,3 @@ void SafeBrowsingService::ReportMalware(const GURL& malware_url,
 
   protocol_manager_->ReportMalware(malware_url, page_url, referrer_url);
 }
-
-void SafeBrowsingService::RunQueuedClients() {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
-  HISTOGRAM_COUNTS("SB.QueueDepth", queued_checks_.size());
-  while (!queued_checks_.empty()) {
-    QueuedCheck check = queued_checks_.front();
-    HISTOGRAM_TIMES("SB.QueueDelay", Time::Now() - check.start);
-    CheckUrl(check.url, check.client);
-    queued_checks_.pop_front();
-  }
-}
diff --git a/chrome/browser/safe_browsing/safe_browsing_service.h b/chrome/browser/safe_browsing/safe_browsing_service.h
index 9269083..f9c3ba9 100644
--- a/chrome/browser/safe_browsing/safe_browsing_service.h
+++ b/chrome/browser/safe_browsing/safe_browsing_service.h
@@ -139,8 +139,12 @@ class SafeBrowsingService
   // Preference handling.
   static void RegisterPrefs(PrefService* prefs);
 
-  // Called on the IO thread to close the database, freeing the memory
+  // Called on the IO thread to try to close the database, freeing the memory
   // associated with it.  The database will be automatically reopened as needed.
+  //
+  // NOTE: Actual database closure is asynchronous, and until it happens, the IO
+  // thread is not allowed to access it; may not actually trigger a close if one
+  // is already pending or doing so would cause problems.
   void CloseDatabase();
 
   // Called on the IO thread to reset the database.
@@ -184,8 +188,13 @@ class SafeBrowsingService
   // Called to shutdown operations on the io_thread.
   void OnIOShutdown();
 
-  // Called on the IO thread.  Returns whether |database_| exists.  If it does
-  // not exist, queues up a call on the db thread to create it.
+  // Returns whether |database_| exists and is accessible.
+  bool database_available() const {
+    return !closing_database_ && (database_ != NULL);
+  }
+
+  // Called on the IO thread.  If the database does not exist, queues up a call
+  // on the db thread to create it.  Returns whether the database is available.
   //
   // Note that this is only needed outside the db thread, since functions on the
   // db thread can call GetDatabase() directly.
@@ -211,7 +220,9 @@ class SafeBrowsingService
   // Called on the IO thread after the database reports that it added a chunk.
   void OnChunkInserted();
 
-  // Notification that the database is done loading its bloom filter.
+  // Notification that the database is done loading its bloom filter.  We may
+  // have had to queue checks until the database is ready, and if so, this
+  // checks them.
   void DatabaseLoadComplete();
 
   // Called on the database thread to add/remove chunks and host keys.
@@ -259,11 +270,6 @@ class SafeBrowsingService
                      const GURL& page_url,
                      const GURL& referrer_url);
 
-  // During a reset or the initial load we may have to queue checks until the
-  // database is ready. This method is run once the database has loaded (or if
-  // we shut down SafeBrowsing before the database has finished loading).
-  void RunQueuedClients();
-
   CurrentChecks checks_;
 
   // Used for issuing only one GetHash request for a given prefix.