Bind ModelLoader to a sequence, not a thread.
BUG=714191
Review-Url: https://codereview.chromium.org/2831183004
Cr-Commit-Position: refs/heads/master@{#468252}
diff --git a/chrome/browser/safe_browsing/client_side_model_loader.cc b/chrome/browser/safe_browsing/client_side_model_loader.cc
index ea151e8..8e249d0 100644
--- a/chrome/browser/safe_browsing/client_side_model_loader.cc
+++ b/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() {
+ // 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 @@
// 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::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::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::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.
diff --git a/chrome/browser/safe_browsing/client_side_model_loader.h b/chrome/browser/safe_browsing/client_side_model_loader.h
index 3fc7183..af942bfb 100644
--- a/chrome/browser/safe_browsing/client_side_model_loader.h
+++ b/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 @@
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 @@
// 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 @@
// 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_;
