Move SafeBrowsing to the blocking pool via an experiment.

chrome/browser/safe_browsing/database_manager.cc

 // Copyright (c) 2012 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 "chrome/browser/safe_browsing/database_manager.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/debug/leak_tracker.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/prerender/prerender_field_trial.h"
 #include "chrome/browser/safe_browsing/client_side_detection_service.h"
 #include "chrome/browser/safe_browsing/download_protection_service.h"
 #include "chrome/browser/safe_browsing/malware_details.h"
 #include "chrome/browser/safe_browsing/protocol_manager.h"
 #include "chrome/browser/safe_browsing/safe_browsing_database.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/safe_browsing/ui_manager.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
-#include "components/metrics/metrics_service.h"
 #include "components/startup_metric_utils/startup_metric_utils.h"
+#include "components/variations/variations_associated_data.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 #include "url/url_constants.h"
 
 using content::BrowserThread;
 
 namespace {
 
 // Timeout for match checks, e.g. download URLs, hashes.
 const int kCheckTimeoutMs = 10000;
@@ skipping 545 matching lines @@
 
   // Cache the GetHash results.
   if (cache_lifetime != base::TimeDelta() && MakeDatabaseAvailable())
     database_->CacheHashResults(prefixes, full_hashes, cache_lifetime);
 }
 
 void SafeBrowsingDatabaseManager::GetChunks(GetChunksCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(enabled_);
   DCHECK(!callback.is_null());
-  safe_browsing_thread_->message_loop()->PostTask(FROM_HERE, base::Bind(
-      &SafeBrowsingDatabaseManager::GetAllChunksFromDatabase, this, callback));
+  safe_browsing_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&SafeBrowsingDatabaseManager::GetAllChunksFromDatabase, this,
+                 callback));
 }
 
 void SafeBrowsingDatabaseManager::AddChunks(
     const std::string& list,
     scoped_ptr<ScopedVector<SBChunkData> > chunks,
     AddChunksCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(enabled_);
   DCHECK(!callback.is_null());
-  safe_browsing_thread_->message_loop()->PostTask(FROM_HERE, base::Bind(
-      &SafeBrowsingDatabaseManager::AddDatabaseChunks, this, list,
-      base::Passed(&chunks), callback));
+  safe_browsing_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&SafeBrowsingDatabaseManager::AddDatabaseChunks,
+                            this, list, base::Passed(&chunks), callback));
 }
 
 void SafeBrowsingDatabaseManager::DeleteChunks(
     scoped_ptr<std::vector<SBChunkDelete> > chunk_deletes) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(enabled_);
-  safe_browsing_thread_->message_loop()->PostTask(FROM_HERE, base::Bind(
-      &SafeBrowsingDatabaseManager::DeleteDatabaseChunks, this,
-      base::Passed(&chunk_deletes)));
+  safe_browsing_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&SafeBrowsingDatabaseManager::DeleteDatabaseChunks,
+                            this, base::Passed(&chunk_deletes)));
 }
 
 void SafeBrowsingDatabaseManager::UpdateStarted() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(enabled_);
   DCHECK(!update_in_progress_);
   update_in_progress_ = true;
 }
 
 void SafeBrowsingDatabaseManager::UpdateFinished(bool update_succeeded) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(enabled_);
   if (update_in_progress_) {
     update_in_progress_ = false;
-    safe_browsing_thread_->message_loop()->PostTask(FROM_HERE,
-      base::Bind(&SafeBrowsingDatabaseManager::DatabaseUpdateFinished,
-                 this, update_succeeded));
+    safe_browsing_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&SafeBrowsingDatabaseManager::DatabaseUpdateFinished, this,
+                   update_succeeded));
   }
 }
 
 void SafeBrowsingDatabaseManager::ResetDatabase() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(enabled_);
-  safe_browsing_thread_->message_loop()->PostTask(FROM_HERE, base::Bind(
-      &SafeBrowsingDatabaseManager::OnResetDatabase, this));
+  safe_browsing_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&SafeBrowsingDatabaseManager::OnResetDatabase, this));
 }
 
 void SafeBrowsingDatabaseManager::StartOnIOThread() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (enabled_)
     return;
 
-  DCHECK(!safe_browsing_thread_.get());
-  safe_browsing_thread_.reset(new base::Thread("Chrome_SafeBrowsingThread"));
-  if (!safe_browsing_thread_->Start())
-    return;
+  // Don't create the thread under "BlockingPool" experimental group - instead

[gab, 2015/02/19 14:38:22]

"BlockingPool" is not really an experimental group, is it? It's more a variation
of LightSpeed.SBThreadingMode? (you know more than I, but this wording feels
wrong to me)

[Alexei Svitkine (slow), 2015/02/20 15:42:44]

Updated comment.

+  // the blocking pool will be used for safe browsing work.
+  const std::string sb_threading_mode =
+      variations::GetVariationParamValue("LightSpeed", "SBThreadingMode");
+  if (sb_threading_mode == "BlockingPool") {
+    base::SequencedWorkerPool* pool = BrowserThread::GetBlockingPool();
+    safe_browsing_task_runner_ =
+        pool->GetSequencedTaskRunner(pool->GetSequenceToken());

[gab, 2015/02/19 14:38:22]

I think we also want SKIP_ON_SHUTDOWN behaviour here.

[Alexei Svitkine (slow), 2015/02/20 15:42:44]

Done.

+  } else {
+    DCHECK(!safe_browsing_thread_.get());
+    safe_browsing_thread_.reset(new base::Thread("Chrome_SafeBrowsingThread"));
+    if (!safe_browsing_thread_->Start())
+      return;
+    safe_browsing_task_runner_ = safe_browsing_thread_->task_runner();
+  }
   enabled_ = true;
 
   MakeDatabaseAvailable();
 }
 
 void SafeBrowsingDatabaseManager::StopOnIOThread(bool shutdown) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   DoStopOnIOThread();
   if (shutdown) {
@@ skipping 55 matching lines @@
   //    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.
   // Checking DatabaseAvailable() avoids both of these.
   if (DatabaseAvailable()) {
     closing_database_ = true;
-    safe_browsing_thread_->message_loop()->PostTask(FROM_HERE,
+    safe_browsing_task_runner_->PostTask(
+        FROM_HERE,
         base::Bind(&SafeBrowsingDatabaseManager::OnCloseDatabase, this));
   }
 
   // Flush the database thread. Any in-progress database check results will be
   // ignored and cleaned up below.
   //
   // Note that to avoid leaking the database, we rely on the fact that no new
   // tasks will be added to the db thread between the call above and this one.
   // See comments on the declaration of |safe_browsing_thread_|.
-  {
+  if (safe_browsing_thread_) {
     // A ScopedAllowIO object is required to join the thread when calling Stop.
     // See http://crbug.com/72696. Note that we call Stop() first to clear out
     // any remaining tasks before clearing safe_browsing_thread_.
     base::ThreadRestrictions::ScopedAllowIO allow_io_for_thread_join;
     safe_browsing_thread_->Stop();
     safe_browsing_thread_.reset();

[gab, 2015/02/19 14:38:22]

Previously this guaranteed that the |safe_browsing_thread_| went away before the
IO thread (which was not necessarily a good thing as blocking the IO thread is
not typically desired, but it did provide this guarantee).

Now with the BlockingPool, I'm not sure what the shutdown properties are, worth
digging into to make sure it's still okay.

[Alexei Svitkine (slow), 2015/02/20 15:42:44]

Hmm, so by switching to  SKIP_ON_SHUTDOWN as you suggested in another comment,
this will be a behavior change. This previous code would actually run all
pending tasks (synchronously on IO thread).

SKIP_ON_SHUTDOWN will just skip running them.

I think ideally we want SKIP_ON_SHUTDOWN - i.e. if we can be in a state where
just ignoring any pending tasks doesn't result in any corruption, etc. I guess I
don't know enough about SB to know whether this is the case - however since this
is equivalent to what would happen on e.g. a crash, I think there's a good
chance that it actually should be OK (else crashes would cause a similar type of
corruption and expose the same problems).

Also, I took a cursory look at the ownership model and ultimately it looks like
the tree of SB objects is own by SB service which hangs off the
g_browser_process which is shut down on the UI thread. I believe UI thread is
the last to go, so this should be safe in terms of ownership (i.e. the
underlying SB objects shouldn't go away while tasks are still trying to run). At
least, as far as I understand.

So I propose to go ahead with SKIP_ON_SHUTDOWN behavior and if there are any
problems (e.g. crashes), the experiment will reveal them.

[mattm, 2015/02/21 02:03:06]

Should also keep an eye on the FAILURE_DATABASE_CORRUPT_HANDLER histogram to see
if people in the experiment have any higher instance of needing to reset the
database.

   }
 
   // Delete pending checks, calling back any clients with 'SB_THREAT_TYPE_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) {
     SafeBrowsingCheck* check = *it;
     if (check->client)
       check->client->OnSafeBrowsingResult(*check);
   }
   STLDeleteElements(&checks_);
 
   gethash_requests_.clear();
 }
 
 bool SafeBrowsingDatabaseManager::DatabaseAvailable() const {
   base::AutoLock lock(database_lock_);
   return !closing_database_ && (database_ != NULL);
 }
 
 bool SafeBrowsingDatabaseManager::MakeDatabaseAvailable() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(enabled_);
   if (DatabaseAvailable())
     return true;
-  safe_browsing_thread_->message_loop()->PostTask(
+  safe_browsing_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(base::IgnoreResult(&SafeBrowsingDatabaseManager::GetDatabase),
                  this));
   return false;
 }
 
 SafeBrowsingDatabase* SafeBrowsingDatabaseManager::GetDatabase() {
-  DCHECK_EQ(base::MessageLoop::current(),
-            safe_browsing_thread_->message_loop());
+  DCHECK(safe_browsing_task_runner_->RunsTasksOnCurrentThread());
+
   if (database_)
     return database_;
   startup_metric_utils::ScopedSlowStartupUMA
       scoped_timer("Startup.SlowStartupSafeBrowsingGetDatabase");
   const base::TimeTicks before = base::TimeTicks::Now();
 
-  SafeBrowsingDatabase* database =
-      SafeBrowsingDatabase::Create(enable_download_protection_,
-                                   enable_csd_whitelist_,
-                                   enable_download_whitelist_,
-                                   enable_extension_blacklist_,
-                                   enable_side_effect_free_whitelist_,
-                                   enable_ip_blacklist_,
-                                   enable_unwanted_software_blacklist_);
+  SafeBrowsingDatabase* database = SafeBrowsingDatabase::Create(
+      safe_browsing_task_runner_, enable_download_protection_,
+      enable_csd_whitelist_, enable_download_whitelist_,
+      enable_extension_blacklist_, enable_side_effect_free_whitelist_,
+      enable_ip_blacklist_, enable_unwanted_software_blacklist_);
 
   database->Init(SafeBrowsingService::GetBaseFilename());
   {
     // Acquiring the lock here guarantees correct ordering between the writes to
     // the new database object above, and the setting of |database_| below.
     base::AutoLock lock(database_lock_);
     database_ = database;
   }
 
   BrowserThread::PostTask(
@@ skipping 51 matching lines @@
         is_download);
   } else {
     // We may have cached results for previous GetHash queries.  Since
     // this data comes from cache, don't histogram hits.
     HandleOneCheck(check, check->cache_hits);
   }
 }
 
 void SafeBrowsingDatabaseManager::GetAllChunksFromDatabase(
     GetChunksCallback callback) {
-  DCHECK_EQ(base::MessageLoop::current(),
-            safe_browsing_thread_->message_loop());
+  DCHECK(safe_browsing_task_runner_->RunsTasksOnCurrentThread());
 
   bool database_error = true;
   std::vector<SBListChunkRanges> lists;
   DCHECK(!database_update_in_progress_);
   database_update_in_progress_ = true;
   GetDatabase();  // This guarantees that |database_| is non-NULL.
   if (database_->UpdateStarted(&lists)) {
     database_error = false;
   } else {
     database_->UpdateFinished(false);
@@ skipping 49 matching lines @@
       check.client->OnSafeBrowsingResult(sb_check);
     }
     queued_checks_.pop_front();
   }
 }
 
 void SafeBrowsingDatabaseManager::AddDatabaseChunks(
     const std::string& list_name,
     scoped_ptr<ScopedVector<SBChunkData> > chunks,
     AddChunksCallback callback) {
-  DCHECK_EQ(base::MessageLoop::current(),
-            safe_browsing_thread_->message_loop());
+  DCHECK(safe_browsing_task_runner_->RunsTasksOnCurrentThread());
   if (chunks)
     GetDatabase()->InsertChunks(list_name, chunks->get());
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&SafeBrowsingDatabaseManager::OnAddChunksComplete, this,
                  callback));
 }
 
 void SafeBrowsingDatabaseManager::DeleteDatabaseChunks(
     scoped_ptr<std::vector<SBChunkDelete> > chunk_deletes) {
-  DCHECK_EQ(base::MessageLoop::current(),
-            safe_browsing_thread_->message_loop());
+  DCHECK(safe_browsing_task_runner_->RunsTasksOnCurrentThread());
   if (chunk_deletes)
     GetDatabase()->DeleteChunks(*chunk_deletes);
 }
 
 void SafeBrowsingDatabaseManager::DatabaseUpdateFinished(
     bool update_succeeded) {
-  DCHECK_EQ(base::MessageLoop::current(),
-            safe_browsing_thread_->message_loop());
+  DCHECK(safe_browsing_task_runner_->RunsTasksOnCurrentThread());
   GetDatabase()->UpdateFinished(update_succeeded);
   DCHECK(database_update_in_progress_);
   database_update_in_progress_ = false;
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&SafeBrowsingDatabaseManager::NotifyDatabaseUpdateFinished,
                  this, update_succeeded));
 }
 
 void SafeBrowsingDatabaseManager::OnCloseDatabase() {
-  DCHECK_EQ(base::MessageLoop::current(),
-            safe_browsing_thread_->message_loop());
+  DCHECK(safe_browsing_task_runner_->RunsTasksOnCurrentThread());
   DCHECK(closing_database_);
 
   // 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.
   delete database_;
   database_ = NULL;
 
   // Acquiring the lock here guarantees correct ordering between the resetting
   // of |database_| above and of |closing_database_| below, which ensures there
   // won't be a window during which the IO thread falsely believes the database
   // is available.
   base::AutoLock lock(database_lock_);
   closing_database_ = false;
 }
 
 void SafeBrowsingDatabaseManager::OnResetDatabase() {
-  DCHECK_EQ(base::MessageLoop::current(),
-            safe_browsing_thread_->message_loop());
+  DCHECK(safe_browsing_task_runner_->RunsTasksOnCurrentThread());
+
   GetDatabase()->ResetDatabase();
 }
 
 void SafeBrowsingDatabaseManager::OnHandleGetHashResults(
     SafeBrowsingCheck* check,
     const std::vector<SBFullHashResult>& full_hashes) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   safe_browsing_util::ListType check_type = check->check_type;
   SBPrefix prefix = check->prefix_hits[0];
   GetHashRequests::iterator it = gethash_requests_.find(prefix);
@@ skipping 61 matching lines @@
       is_threat = true;
     }
   }
 
   SafeBrowsingCheckDone(check);
   return is_threat;
 }
 
 void SafeBrowsingDatabaseManager::CheckDownloadUrlOnSBThread(
     SafeBrowsingCheck* check) {
-  DCHECK_EQ(base::MessageLoop::current(),
-            safe_browsing_thread_->message_loop());
+  DCHECK(safe_browsing_task_runner_->RunsTasksOnCurrentThread());
   DCHECK(enable_download_protection_);
 
   std::vector<SBPrefix> prefix_hits;
 
   if (!database_->ContainsDownloadUrl(check->urls, &prefix_hits)) {
     // Good, we don't have hash for this url prefix.
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,
         base::Bind(&SafeBrowsingDatabaseManager::CheckDownloadUrlDone, this,
                    check));
     return;
   }
 
   check->need_get_hash = true;
   check->prefix_hits.clear();
   check->prefix_hits = prefix_hits;
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&SafeBrowsingDatabaseManager::OnCheckDone, this, check));
 }
 
 void SafeBrowsingDatabaseManager::CheckExtensionIDsOnSBThread(
     SafeBrowsingCheck* check) {
-  DCHECK_EQ(base::MessageLoop::current(),
-            safe_browsing_thread_->message_loop());
+  DCHECK(safe_browsing_task_runner_->RunsTasksOnCurrentThread());
 
   std::vector<SBPrefix> prefixes;
   for (std::vector<SBFullHash>::iterator it = check->full_hashes.begin();
        it != check->full_hashes.end(); ++it) {
     prefixes.push_back((*it).prefix);
   }
   database_->ContainsExtensionPrefixes(prefixes, &check->prefix_hits);
 
   if (check->prefix_hits.empty()) {
     // No matches for any extensions.
@@ skipping 50 matching lines @@
 }
 
 void SafeBrowsingDatabaseManager::StartSafeBrowsingCheck(
     SafeBrowsingCheck* check,
     const base::Closure& task) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   check->timeout_factory_.reset(
       new base::WeakPtrFactory<SafeBrowsingDatabaseManager>(this));
   checks_.insert(check);
 
-  safe_browsing_thread_->message_loop()->PostTask(FROM_HERE, task);
+  safe_browsing_task_runner_->PostTask(FROM_HERE, task);
 
   base::MessageLoop::current()->PostDelayedTask(FROM_HERE,
       base::Bind(&SafeBrowsingDatabaseManager::TimeoutCallback,
                  check->timeout_factory_->GetWeakPtr(), check),
       check_timeout_);
 }