Bind ModelLoader to a sequence, not a thread.

BUG=714191

Review-Url: https://codereview.chromium.org/2831183004
Cr-Commit-Position: refs/heads/master@{#468252}

chrome/browser/safe_browsing/client_side_model_loader.cc

@@ -10,10 +10,9 @@
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/metrics/histogram_macros.h"
-#include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
-#include "base/threading/thread_task_runner_handle.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/time/time.h"
 #include "chrome/browser/safe_browsing/protocol_manager.h"
 #include "chrome/common/safe_browsing/client_model.pb.h"
@@ -108,6 +107,9 @@ ModelLoader::ModelLoader(base::Closure update_renderers_callback,
 }
 
 ModelLoader::~ModelLoader() {
+  // This must happen on the same sequence as ScheduleFetch because it
+  // invalidates any WeakPtrs allocated there.
+  DCHECK(fetch_sequence_checker_.CalledOnValidSequence());
 }
 
 void ModelLoader::StartFetch() {
@@ -117,6 +119,7 @@ void ModelLoader::StartFetch() {
   // TODO(nparker): If no profile needs this model, we shouldn't fetch it.
   // Then only re-fetch when a profile setting changes to need it.
   // This will save on the order of ~50KB/week/client of bandwidth.
+  DCHECK(fetch_sequence_checker_.CalledOnValidSequence());
   fetcher_ = net::URLFetcher::Create(0 /* ID used for testing */, url_,
                                      net::URLFetcher::GET, this);
   data_use_measurement::DataUseUserData::AttachToFetcher(
@@ -128,6 +131,7 @@ void ModelLoader::StartFetch() {
 }
 
 void ModelLoader::OnURLFetchComplete(const net::URLFetcher* source) {
+  DCHECK(fetch_sequence_checker_.CalledOnValidSequence());
   DCHECK_EQ(fetcher_.get(), source);
   DCHECK_EQ(url_, source->GetURL());
 
@@ -174,6 +178,7 @@ void ModelLoader::OnURLFetchComplete(const net::URLFetcher* source) {
 }
 
 void ModelLoader::EndFetch(ClientModelStatus status, base::TimeDelta max_age) {
+  DCHECK(fetch_sequence_checker_.CalledOnValidSequence());
   // We don't differentiate models in the UMA stats.
   UMA_HISTOGRAM_ENUMERATION("SBClientPhishing.ClientModelStatus",
                             status,
@@ -200,16 +205,20 @@ void ModelLoader::EndFetch(ClientModelStatus status, base::TimeDelta max_age) {
 }
 
 void ModelLoader::ScheduleFetch(int64_t delay_ms) {
+  DCHECK(fetch_sequence_checker_.CalledOnValidSequence());
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           safe_browsing::switches::kSbDisableAutoUpdate))
     return;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+  base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE,
       base::BindOnce(&ModelLoader::StartFetch, weak_factory_.GetWeakPtr()),
       base::TimeDelta::FromMilliseconds(delay_ms));
 }
 
 void ModelLoader::CancelFetcher() {
+  // This must be called on the same sequence as ScheduleFetch because it
+  // invalidates any WeakPtrs allocated there.
+  DCHECK(fetch_sequence_checker_.CalledOnValidSequence());
   // Invalidate any scheduled request.
   weak_factory_.InvalidateWeakPtrs();
   // Cancel any request in progress.

chrome/browser/safe_browsing/client_side_model_loader.h

@@ -4,9 +4,6 @@
 //
 // Helper class loads models for client-side phishing detection
 // from the the SafeBrowsing backends.
-//
-// This class is not thread-safe and expects all calls to be made on the UI
-// thread.
 
 #ifndef CHROME_BROWSER_SAFE_BROWSING_CLIENT_SIDE_MODEL_LOADER_H_
 #define CHROME_BROWSER_SAFE_BROWSING_CLIENT_SIDE_MODEL_LOADER_H_
@@ -22,7 +19,7 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
-#include "content/public/browser/browser_thread.h"
+#include "base/sequence_checker.h"
 #include "net/url_request/url_fetcher_delegate.h"
 #include "url/gurl.h"
 
@@ -49,6 +46,9 @@ class ModelLoader : public net::URLFetcherDelegate {
   static const char kClientModelUrlPrefix[];
   static const char kClientModelNamePattern[];
 
+  // Constructs a model loader to fetch a model using |request_context_getter|.
+  // When ScheduleFetch is called, |update_renderers| will be called on the
+  // same sequence if the fetch is successful.
   ModelLoader(base::Closure update_renderers,
               net::URLRequestContextGetter* request_context_getter,
               bool is_extended_reporting);
@@ -60,7 +60,8 @@ class ModelLoader : public net::URLFetcherDelegate {
   // Schedules the next fetch of the model.
   virtual void ScheduleFetch(int64_t delay_ms);
 
-  // Cancel any pending model fetch.
+  // Cancels any pending model fetch. This must be called from the same
+  // sequence as ScheduleFetch.
   virtual void CancelFetcher();
 
   const std::string& model_str() const { return model_str_; }
@@ -122,6 +123,10 @@ class ModelLoader : public net::URLFetcherDelegate {
   // Not owned, must outlive this obj or be NULL.
   net::URLRequestContextGetter* request_context_getter_;
 
+  // Used to check that ScheduleFetch and CancelFetcher are called on the same
+  // sequence.
+  base::SequenceChecker fetch_sequence_checker_;
+
   // Used to protect the delayed callback to StartFetchModel()
   base::WeakPtrFactory<ModelLoader> weak_factory_;