Bind ModelLoader to a sequence, not a thread.

chrome/browser/safe_browsing/client_side_model_loader.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/client_side_model_loader.h"
 
 #include <memory>
 
 #include "base/bind.h"
 #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"
 #include "components/data_use_measurement/core/data_use_user_data.h"
 #include "components/safe_browsing/common/safebrowsing_messages.h"
 #include "components/safe_browsing/common/safebrowsing_switches.h"
 #include "components/safe_browsing/csd.pb.h"
 #include "components/variations/variations_associated_data.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_response_headers.h"
@@ skipping 54 matching lines @@
       return false;
     }
   }
   return true;
 }
 
 // Model name and URL are a function of is_extended_reporting and Finch.
 ModelLoader::ModelLoader(base::Closure update_renderers_callback,
                          net::URLRequestContextGetter* request_context_getter,
                          bool is_extended_reporting)
-    : name_(FillInModelName(is_extended_reporting, GetModelNumber())),
+    : 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_(request_context_getter),
       weak_factory_(this) {
   DCHECK(url_.is_valid());
-}
 
-// For testing only
-ModelLoader::ModelLoader(base::Closure update_renderers_callback,
-                         const std::string& model_name)
-    : name_(model_name),
-      url_(kClientModelUrlPrefix + name_),
-      update_renderers_callback_(update_renderers_callback),
-      request_context_getter_(NULL),
-      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() {
 }
 
 void ModelLoader::StartFetch() {
   // Start fetching the model either from the cache or possibly from the
   // network if the model isn't in the cache.
 
   // 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(
       fetcher_.get(), data_use_measurement::DataUseUserData::SAFE_BROWSING);
   fetcher_->SetRequestContext(request_context_getter_);
   fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SAVE_COOKIES |
                          net::LOAD_DO_NOT_SEND_COOKIES);
   fetcher_->Start();
 }
 
 void ModelLoader::OnURLFetchComplete(const net::URLFetcher* source) {
+  DCHECK(fetch_sequence_checker_.CalledOnValidSequence());
   DCHECK_EQ(fetcher_.get(), source);
   DCHECK_EQ(url_, source->GetURL());
 
   std::string data;
   source->GetResponseAsString(&data);
   net::URLRequestStatus status = source->GetStatus();
   const bool is_success = status.is_success();
   const int response_code = source->GetResponseCode();
   SafeBrowsingProtocolManager::RecordHttpResponseOrErrorCode(
       kUmaModelDownloadResponseMetricName, status, response_code);
@@ skipping 26 matching lines @@
   } else {
     // The model is valid => replace the existing model with the new one.
     model_str_.assign(data);
     model_.swap(model);
     model_status = MODEL_SUCCESS;
   }
   EndFetch(model_status, max_age);
 }
 
 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,
                             MODEL_STATUS_MAX);
   if (status == MODEL_SUCCESS) {
     update_renderers_callback_.Run();
   }
   int delay_ms = kClientModelFetchIntervalMs;
   // If the most recently fetched model had a valid max-age and the model was
   // valid we're scheduling the next model update for after the max-age expired.
   if (!max_age.is_zero() &&
       (status == MODEL_SUCCESS || status == MODEL_NOT_CHANGED)) {
     // We're adding 60s of additional delay to make sure we're past
     // the model's age.
     max_age += base::TimeDelta::FromMinutes(1);
     delay_ms = max_age.InMilliseconds();
   }
 
   // Reset |fetcher_| as it will be re-created on next fetch.
   fetcher_.reset();
 
   // Schedule the next model reload.
   ScheduleFetch(delay_ms);
 }
 
 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.
   fetcher_.reset();
 }
 
 }  // namespace safe_browsing