Bind ModelLoader to a sequence, not a thread.

chrome/browser/safe_browsing/client_side_model_loader.cc

Index: chrome/browser/safe_browsing/client_side_model_loader.cc
diff --git a/chrome/browser/safe_browsing/client_side_model_loader.cc b/chrome/browser/safe_browsing/client_side_model_loader.cc
index ea151e82f5e09dcf0f7d823afcab0deaa1070eef..6ca5045fc955f923f9b36987d67c7c2181ecb294 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"
@@ -88,23 +87,28 @@ bool ModelLoader::ModelHasValidHashIds(const ClientSideModel& model) {
 ModelLoader::ModelLoader(base::Closure update_renderers_callback,
                          net::URLRequestContextGetter* request_context_getter,
                          bool is_extended_reporting)
-    : name_(FillInModelName(is_extended_reporting, GetModelNumber())),
-      url_(kClientModelUrlPrefix + name_),
-      update_renderers_callback_(update_renderers_callback),
-      request_context_getter_(request_context_getter),
-      weak_factory_(this) {
-  DCHECK(url_.is_valid());
-}
+    : ModelLoader(update_renderers_callback,
+                  request_context_getter,
+                  FillInModelName(is_extended_reporting, GetModelNumber())) {}
 
 // For testing only
 ModelLoader::ModelLoader(base::Closure update_renderers_callback,
                          const std::string& model_name)
+    : ModelLoader(update_renderers_callback, nullptr, model_name) {}
+
+ModelLoader::ModelLoader(base::Closure update_renderers_callback,
+                         net::URLRequestContextGetter* request_context_getter,
+                         const std::string& model_name)

[gab, 2017/04/25 14:34:08]

Can avoid a string copy by taking a std::string by value and std::move()'ing it
into |name_| here :)

[Joe Mason, 2017/04/25 18:05:48]

I don't follow. Either we pass it in by reference, and it's copied when
assigning to name_, or it's copied during the pass-by-value.

[gab, 2017/04/25 18:38:27]

In the non-test constructor, it's constructed as a temporary string. Taking it
by reference here means that this temporary string is copied into |name_|
whereas moving the temporary into |name_| would avoid that.

[Joe Mason, 2017/04/28 13:54:19]

But if I take it by value, wouldn't it be copied from the temporary to
|model_name|, and then moved from |model_name| to |name_|?

[gab, 2017/04/28 16:29:53]

No because the temporary is an r-value and will be moved implicitly.

[Joe Mason, 2017/04/28 19:43:55]

I see. Without the DetachFromSequence call there's not much point in the
combined constructor, though, so discarding this.

     : name_(model_name),
       url_(kClientModelUrlPrefix + name_),
       update_renderers_callback_(update_renderers_callback),
-      request_context_getter_(NULL),
+      request_context_getter_(request_context_getter),
       weak_factory_(this) {
   DCHECK(url_.is_valid());
+
+  // ScheduleFetch and CancelFetcher must be called on the same sequence, but
+  // it doesn't have to be the same one the ModelLoader is constructed on.

[gab, 2017/04/25 14:34:08]

I typically call that the "creation sequence", i.e.

"but it doesn't have to be on the creation sequence."

[Joe Mason, 2017/04/28 19:43:55]

Removed the Detach call.

+  fetch_sequence_checker_.DetachFromSequence();
 }
 
 ModelLoader::~ModelLoader() {
@@ -117,6 +121,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 +133,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 +180,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 +207,20 @@ void ModelLoader::EndFetch(ClientModelStatus status, base::TimeDelta max_age) {
 }
 
 void ModelLoader::ScheduleFetch(int64_t delay_ms) {
+  // Bind the sequence checker to the current sequence. CancelFetcher must be
+  // called on the same sequence because it invalidates the WeakPtr.

[gab, 2017/04/25 14:34:08]

PS: ~ModelLoader() also invalidates WeakPtrs and should maybe have this check as
well? Unless you can guarantee destruction always occurs after completion ||
ModelLoader::CancelFetcher()? Destruction/construction sequence is usually the
same so it'll probably be hard to DCHECK on destruction. Either way WeakPtr's
DCHECK verify it's used properly (destroyed off-sequence only with no remaining
references from factory). In either case I like the explicit sequence checks
here for documentation :)

[Joe Mason, 2017/04/25 18:05:48]

Good catch. I'll double check on this before committing.

[Joe Mason, 2017/04/28 13:54:19]

I think I'll remove the DetachFromSequence in the constructor, so that it
asserts construction, all WeakPtr-using methods, and destruction happen on the
same sequence. It's a simpler semantic even though technically a caller COULD
construct it on one sequence and then pass ownership of the ModelLoader to
another sequence, no callers do that now. What do you think?

[gab, 2017/04/28 16:29:52]

Yeah let's not detach in constructor if current users don't require it.

[Jialiu Lin, 2017/04/28 17:52:47]

I'm OK with this. We are no longer actively changing this part of code. So a
simpler approach would be good enough. 

[Joe Mason, 2017/04/28 19:43:55]

Done.

+  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() {
+  DCHECK(fetch_sequence_checker_.CalledOnValidSequence());
   // Invalidate any scheduled request.
   weak_factory_.InvalidateWeakPtrs();
   // Cancel any request in progress.