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());

[gab, 2015/02/23 16:33:55]

This DCHECK ensured StartOnIOThread() was only ever called once, perhaps add
DCHECK(!safe_browsing_task_runner_) at the top of this method to keep this
check?

[Alexei Svitkine (slow), 2015/02/23 19:03:27]

The "if (enabled_) return;" code suggests that this actually can be called
multiple times. I'd rather not change that in this CL, so added the check below
that to match the previous check location.

[mattm, 2015/02/23 19:27:36]

Actually the start/stop methods can be called multiple times (ex. if
safebrowsing is disabled and re-enabled). So if you want to dcheck that, you
should also clear safe_browsing_task_runner_ in DoStopOnIOThread.

[gab, 2015/02/23 19:53:32]

Right that's what I meant.

[gab, 2015/02/23 19:53:32]

Ah, good point, makes sense to me.

[Alexei Svitkine (slow), 2015/02/24 18:08:57]

Done.

-  safe_browsing_thread_.reset(new base::Thread("Chrome_SafeBrowsingThread"));
-  if (!safe_browsing_thread_->Start())
-    return;
+  // Use the blocking pool instead of a dedicated thread for safe browsing work,
+  // if specified by an experiment.
+  const std::string sb_threading_mode =

[gab, 2015/02/23 16:33:55]

How about 

const bool use_blocking_pool = variations::...() == "BlockingPool";

instead of splitting the logic between this statement and the conditional below?

[Alexei Svitkine (slow), 2015/02/23 19:03:27]

Done.

+      variations::GetVariationParamValue("LightSpeed", "SBThreadingMode");
+  if (sb_threading_mode == "BlockingPool") {
+    base::SequencedWorkerPool* pool = BrowserThread::GetBlockingPool();
+    safe_browsing_task_runner_ =
+        pool->GetSequencedTaskRunnerWithShutdownBehavior(
+            pool->GetSequenceToken(),
+            base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
+  } 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();
   }
 
   // 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
@@ skipping 13 matching lines @@
 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 59 matching lines @@
         IsExpectedThreat(threat, check->expected_threats)) {
       check->full_hash_results[i] = threat;
       is_threat = true;
     }
   }
 
   SafeBrowsingCheckDone(check);
   return is_threat;
 }
 
 void SafeBrowsingDatabaseManager::CheckDownloadUrlOnSBThread(

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

Should also keep an eye on SB2.DownloadUrlCheckDuration histogram to see if
moving the checks to the blocking pool has a negative effect on that (Dunno how
much contention there is for the blocking pool.)

[gab, 2015/02/23 16:33:55]

Side-note: we are also contemplating reducing thread priority on the blocking
pool, perhaps the download list should just be updated on the
|safe_browsing_task_runner_| while being thread-safe to lookup (like most other
lists)?

[Alexei Svitkine (slow), 2015/02/23 19:03:26]

Acknowledged.

     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_);
 }