[translate] Fix shutdown race for translate ranker model loader.

components/translate/core/browser/ranker_model_loader.cc

Index: components/translate/core/browser/ranker_model_loader.cc
diff --git a/components/translate/core/browser/ranker_model_loader.cc b/components/translate/core/browser/ranker_model_loader.cc
index 4a8074467518a4f780c58049c829529f578edd5f..5497968383916664235d0ab743fb4b5ca1ab4f26 100644
--- a/components/translate/core/browser/ranker_model_loader.cc
+++ b/components/translate/core/browser/ranker_model_loader.cc
@@ -7,7 +7,6 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
-#include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/important_file_writer.h"
 #include "base/macros.h"
@@ -16,12 +15,12 @@
 #include "base/profiler/scoped_tracker.h"
 #include "base/sequenced_task_runner.h"
 #include "base/strings/string_util.h"
+#include "base/task_runner_util.h"
 #include "base/task_scheduler/post_task.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "components/translate/core/browser/proto/ranker_model.pb.h"
 #include "components/translate/core/browser/ranker_model.h"
 #include "components/translate/core/browser/translate_url_fetcher.h"
-#include "url/gurl.h"
 
 namespace translate {
 namespace {
@@ -31,10 +30,6 @@ using chrome_intelligence::RankerModelProto;
 
 constexpr int kUrlFetcherId = 2;
 
-// The maximum number of model download attempts to make. Download may fail
-// due to server error or network availability issues.
-constexpr int kMaxDownloadAttempts = 8;
-
 // The minimum duration, in minutes, between download attempts.
 constexpr int kMinRetryDelayMins = 3;
 
@@ -45,6 +40,16 @@ const char kDownloadTimerHistogram[] = ".Timer.DownloadModel";
 const char kParsetimerHistogram[] = ".Timer.ParseModel";
 const char kModelStatusHistogram[] = ".Model.Status";
 
+// Helper function to UMA log a timer histograms.
+void RecordTimerHistogram(const std::string& name, base::TimeDelta duration) {
+  base::HistogramBase* counter = base::Histogram::FactoryTimeGet(
+      name, base::TimeDelta::FromMilliseconds(10),
+      base::TimeDelta::FromMilliseconds(200000), 100,
+      base::HistogramBase::kUmaTargetedHistogramFlag);
+  DCHECK(counter);
+  counter->AddTime(duration);
+}
+
 // A helper class to produce a scoped timer histogram that supports using a
 // non-static-const name.
 class MyScopedHistogramTimer {
@@ -53,13 +58,7 @@ class MyScopedHistogramTimer {
       : name_(name.begin(), name.end()), start_(base::TimeTicks::Now()) {}
 
   ~MyScopedHistogramTimer() {
-    base::TimeDelta duration = base::TimeTicks::Now() - start_;
-    base::HistogramBase* counter = base::Histogram::FactoryTimeGet(
-        name_, base::TimeDelta::FromMilliseconds(10),
-        base::TimeDelta::FromMilliseconds(200000), 100,
-        base::HistogramBase::kUmaTargetedHistogramFlag);
-    if (counter)
-      counter->AddTime(duration);
+    RecordTimerHistogram(name_, base::TimeTicks::Now() - start_);
   }
 
  private:
@@ -69,353 +68,243 @@ class MyScopedHistogramTimer {
   DISALLOW_COPY_AND_ASSIGN(MyScopedHistogramTimer);
 };
 
-}  // namespace
-
-// =============================================================================
-// RankerModelLoader::Backend
-
-class RankerModelLoader::Backend {
- public:
-  // An internal version of RankerModelLoader::OnModelAvailableCallback that
-  // bundles calling the real callback with a notification of whether or not
-  // tha backend is finished.
-  using InternalOnModelAvailableCallback =
-      base::Callback<void(std::unique_ptr<RankerModel>, bool)>;
-
-  Backend(const ValidateModelCallback& validate_model_cb,
-          const InternalOnModelAvailableCallback& on_model_available_cb,
-          const base::FilePath& model_path,
-          const GURL& model_url,
-          const std::string& uma_prefix);
-  ~Backend();
-
-  // Reads the model from |model_path_|.
-  void LoadFromFile();
+std::string LoadFromFile(const base::FilePath& model_path) {
+  DCHECK(!model_path.empty());
+  DVLOG(2) << "Reading data from: " << model_path.value();
+  std::string data;
+  if (!base::ReadFileToString(model_path, &data) || data.empty()) {
+    DVLOG(2) << "Failed to read data from: " << model_path.value();
+    data.clear();
+  }
+  return data;
+}
 
-  // Reads the model from |model_url_|.
-  void AsyncLoadFromURL();
+void SaveToFile(const GURL& model_url,
+                const base::FilePath& model_path,
+                const std::string& model_data,
+                const std::string& uma_prefix) {
+  DVLOG(2) << "Saving model from '" << model_url << "'' to '"
+           << model_path.value() << "'.";
+  MyScopedHistogramTimer timer(uma_prefix + kWriteTimerHistogram);
+  base::ImportantFileWriter::WriteFileAtomically(model_path, model_data);
+}
 
- private:
-  // Log and return the result of loading a model to UMA.
-  RankerModelStatus ReportModelStatus(RankerModelStatus model_status);
-
-  // Constructs a model from the given |data|.
-  std::unique_ptr<chrome_intelligence::RankerModel> CreateModel(
-      const std::string& data);
-
-  // Accepts downloaded model data. This signature is mandated by the callback
-  // defined by TransalteURLFetcher.
-  //
-  // id - the id given to the TranslateURLFetcher on creation
-  // success - true of the download was successful
-  // data - the body of the downloads response
-  void OnDownloadComplete(int id, bool success, const std::string& data);
-
-  // Transfers ownership of |model| to the client using the
-  // |internal_on_model_available_cb_|. |is_finished| denotes whether the
-  // backend is finished (or has given up on) loading the model.
-  void TransferModelToClient(
-      std::unique_ptr<chrome_intelligence::RankerModel> model,
-      bool is_finished);
-
-  // Validates that ranker model loader backend tasks are all performed on the
-  // same sequence.
-  base::SequenceChecker sequence_checker_;
-
-  // The TaskRunner on which |this| was constructed.
-  const scoped_refptr<base::SequencedTaskRunner> origin_task_runner_;
-
-  // Validates a ranker model on behalf of the model loader client. This may
-  // be called on any sequence and must, therefore, be thread-safe.
-  const ValidateModelCallback validate_model_cb_;
-
-  // Transfers ownership of a loaded model back to the model loader client.
-  // This will be called on the sequence on which the model loader was
-  // constructed.
-  const InternalOnModelAvailableCallback internal_on_model_available_cb_;
-
-  // The path at which the model is (or should be) cached.
-  const base::FilePath model_path_;
-
-  // The URL from which to download the model if the model is not in the cache
-  // or the cached model is invalid/expired.
-  const GURL model_url_;
-
-  // This will prefix all UMA metrics generated by the model loader.
-  const std::string uma_prefix_;
-
-  // Used to download model data from |model_url_|.
-  // TODO(rogerm): Use net::URLFetcher directly?
-  std::unique_ptr<TranslateURLFetcher> url_fetcher_;
-
-  // The number of times the backend has attempted to download the model.
-  int download_attempts_;
-
-  // The next time before which no new attempts to download the model should be
-  // attempted.
-  base::TimeTicks next_earliest_download_time_;
-
-  // Tracks the last time of the last attempt to download a model. Used for UMA
-  // reporting of download duration.
-  base::TimeTicks download_start_time_;
-
-  DISALLOW_COPY_AND_ASSIGN(Backend);
-};
+}  // namespace
 
-RankerModelLoader::Backend::Backend(
+RankerModelLoader::RankerModelLoader(
     const ValidateModelCallback& validate_model_cb,

[fdoray, 2017/04/25 22:09:08]

It is more efficient to take callbacks by value and std::move them into the
member variable.

Details: base::Callback is a wrapper around a ref-counted state object.
std::move into a member variable avoids a ref-count bump and doesn't really copy
data (just copy a pointer).

[Roger McFarlane (Chromium), 2017/04/26 02:18:50]

Done.

-    const InternalOnModelAvailableCallback& internal_on_model_available_cb,
+    const OnModelAvailableCallback& on_model_available_cb,
     const base::FilePath& model_path,
     const GURL& model_url,
     const std::string& uma_prefix)
-    : origin_task_runner_(base::SequencedTaskRunnerHandle::Get()),
+    : background_task_runner_(base::CreateSequencedTaskRunnerWithTraits(
+          base::TaskTraits()
+              .MayBlock()
+              .WithPriority(base::TaskPriority::BACKGROUND)
+              .WithShutdownBehavior(
+                  base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN))),
       validate_model_cb_(validate_model_cb),
-      internal_on_model_available_cb_(internal_on_model_available_cb),
+      on_model_available_cb_(on_model_available_cb),
       model_path_(model_path),
       model_url_(model_url),
       uma_prefix_(uma_prefix),
       url_fetcher_(base::MakeUnique<TranslateURLFetcher>(kUrlFetcherId)),
-      download_attempts_(0) {
-  sequence_checker_.DetachFromSequence();
-}
-
-RankerModelLoader::Backend::~Backend() {}
+      weak_ptr_factory_(this) {}
 
-RankerModelStatus RankerModelLoader::Backend::ReportModelStatus(
-    RankerModelStatus model_status) {
-  base::HistogramBase* histogram = base::LinearHistogram::FactoryGet(
-      uma_prefix_ + kModelStatusHistogram, 1,
-      static_cast<int>(RankerModelStatus::MAX),
-      static_cast<int>(RankerModelStatus::MAX) + 1,
-      base::HistogramBase::kUmaTargetedHistogramFlag);
-  if (histogram)
-    histogram->Add(static_cast<int>(model_status));
-  return model_status;
+RankerModelLoader::~RankerModelLoader() {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
 }
 
-std::unique_ptr<chrome_intelligence::RankerModel>
-RankerModelLoader::Backend::CreateModel(const std::string& data) {
+void RankerModelLoader::NotifyOfRankerActivity() {
   DCHECK(sequence_checker_.CalledOnValidSequence());
-  MyScopedHistogramTimer timer(uma_prefix_ + kParsetimerHistogram);
-  auto model = RankerModel::FromString(data);
-  if (ReportModelStatus(model ? validate_model_cb_.Run(*model)
-                              : RankerModelStatus::PARSE_FAILED) !=
-      RankerModelStatus::OK) {
-    return nullptr;
+  switch (state_) {
+    case LoaderState::NOT_STARTED:
+      if (!model_path_.empty()) {
+        StartLoadFromFile();
+        break;
+      }
+      // There was no configured model path. Switch the state to IDLE and
+      // fall through to consider the URL.
+      state_ = LoaderState::IDLE;
+    case LoaderState::IDLE:
+      if (model_url_.is_valid()) {
+        StartLoadFromURL();
+        break;
+      }
+      // There was no configured model URL. Switch the state to FINISHED and
+      // fall through.
+      state_ = LoaderState::FINISHED;
+    case LoaderState::FINISHED:
+    case LoaderState::LOADING_FROM_FILE:
+    case LoaderState::LOADING_FROM_URL:
+      // Nothing to do.
+      break;
   }
-  return model;
 }
 
-void RankerModelLoader::Backend::LoadFromFile() {
+void RankerModelLoader::StartLoadFromFile() {
   DCHECK(sequence_checker_.CalledOnValidSequence());
+  DCHECK_EQ(state_, LoaderState::NOT_STARTED);
+  DCHECK(!model_path_.empty());
+  state_ = LoaderState::LOADING_FROM_FILE;
+  load_start_time_ = base::TimeTicks::Now();
+  base::PostTaskAndReplyWithResult(background_task_runner_.get(), FROM_HERE,
+                                   base::Bind(&LoadFromFile, model_path_),

[fdoray, 2017/04/25 22:09:08]

base::BindOnce

[Roger McFarlane (Chromium), 2017/04/26 02:18:50]

Alas, this fails to compile (on Linux at least) if either or both of these is
BindOnce.

+                                   base::Bind(&RankerModelLoader::OnFileLoaded,
+                                              weak_ptr_factory_.GetWeakPtr()));
+}
 
-  // If there is not cache path set, move on to loading the model by URL.
-  if (model_path_.empty()) {
-    AsyncLoadFromURL();
-    return;
-  }
-
-  DVLOG(2) << "Attempting to load model from: " << model_path_.value();
-
-  std::string data;
+void RankerModelLoader::OnFileLoaded(const std::string& data) {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+  DCHECK_EQ(state_, LoaderState::LOADING_FROM_FILE);
 
-  {
-    MyScopedHistogramTimer timer(uma_prefix_ + kReadTimerHistogram);
-    if (!base::ReadFileToString(model_path_, &data) || data.empty()) {
-      DVLOG(2) << "Failed to read model from: " << model_path_.value();
-      data.clear();
-    }
+  // Record the duration of the download.
+  RecordTimerHistogram(uma_prefix_ + kReadTimerHistogram,
+                       base::TimeTicks::Now() - load_start_time_);
+
+  // Empty data means |model_path| wasn't successfully read. Otherwise,
+  // parse and validate the model.
+  std::unique_ptr<RankerModel> model;
+  if (data.empty()) {
+    ReportModelStatus(RankerModelStatus::LOAD_FROM_CACHE_FAILED);
+  } else {
+    model = CreateAndValidateModel(data);
   }
 
-  // If model data was loaded, check if it can be parsed to a valid model.
-  if (!data.empty()) {
-    auto model = CreateModel(data);
-    if (model) {
-      // The model is valid. The client is willing/able to use it. Keep track
-      // of where it originated and whether or not is has expired.
-      std::string url_spec = model->GetSourceURL();
-      bool is_expired = model->IsExpired();
-      bool is_finished = url_spec == model_url_.spec() && !is_expired;
-
-      DVLOG(2) << (is_expired ? "Expired m" : "M") << "odel in '"
-               << model_path_.value() << "' was originally downloaded from '"
-               << url_spec << "'.";
-
-      // Transfer the model to the client. Beyond this line, |model| is invalid.
-      TransferModelToClient(std::move(model), is_finished);
-
-      // If the cached model came from currently configured |model_url_| and has
-      // not expired, there is no need schedule a model download.
-      if (is_finished)
-        return;
-
-      // Otherwise, fall out of this block to schedule a download. The client
-      // can continue to use the valid but expired model until the download
-      // completes.
-    }
+  // If |model| is nullptr, then data is empty or the parse failed. Transition
+  // to IDLE, from which URL download can be attempted.
+  if (!model) {
+    state_ = LoaderState::IDLE;
+  } else {
+    // The model is valid. The client is willing/able to use it. Keep track
+    // of where it originated and whether or not is has expired.
+    std::string url_spec = model->GetSourceURL();
+    bool is_expired = model->IsExpired();
+    bool is_finished = url_spec == model_url_.spec() && !is_expired;
+
+    DVLOG(2) << (is_expired ? "Expired m" : "M") << "odel in '"
+             << model_path_.value() << "' was originally downloaded from '"
+             << url_spec << "'.";
+
+    // If the cached model came from currently configured |model_url_| and has
+    // not expired, transition to FINISHED, as there is no need for a model
+    // download; otherwise, transition to IDLE.
+    state_ = is_finished ? LoaderState::FINISHED : LoaderState::IDLE;
+
+    // Transfer the model to the client.
+    on_model_available_cb_.Run(std::move(model));
   }
 
-  // Reaching this point means that a model download is required. If there is
-  // no download URL configured, then there is nothing further to do.
-  AsyncLoadFromURL();
+  // Notify the state machine. This will immediately kick off a download if
+  // one is required, instead of waiting for the next organic detection of
+  // ranker activity.
+  NotifyOfRankerActivity();
 }
 
-void RankerModelLoader::Backend::AsyncLoadFromURL() {
+void RankerModelLoader::StartLoadFromURL() {
   DCHECK(sequence_checker_.CalledOnValidSequence());
-
-  if (!model_url_.is_valid()) {
-    // Notify the loader that model loading has been abandoned.
-    TransferModelToClient(nullptr, true);
-    return;
-  }
+  DCHECK_EQ(state_, LoaderState::IDLE);
+  DCHECK(model_url_.is_valid());
 
   // Do nothing if the download attempts should be throttled.
   if (base::TimeTicks::Now() < next_earliest_download_time_) {
     DVLOG(2) << "Last download attempt was too recent.";
+    ReportModelStatus(RankerModelStatus::DOWNLOAD_THROTTLED);
     return;
   }
 
-  // If a request is already in flight, do not issue a new one.
-  if (url_fetcher_->state() == TranslateURLFetcher::REQUESTING) {
-    DVLOG(2) << "Download is in progress.";
-    return;
-  }
-
-  // If all allowed attempts have been exhausted, notify the loader that the
-  // backend has abandoned the download.
-  if (++download_attempts_ > kMaxDownloadAttempts) {
-    DVLOG(2) << "Model download abandoned.";
-    ReportModelStatus(RankerModelStatus::DOWNLOAD_FAILED);
-    TransferModelToClient(nullptr, true);
-    return;
-  }
-
-  DVLOG(2) << "Downloading model from: " << model_url_;
-
   // Kick off the next download attempt and reset the time of the next earliest
   // allowable download attempt.
-  download_start_time_ = base::TimeTicks::Now();
+  DVLOG(2) << "Downloading model from: " << model_url_;
+  state_ = LoaderState::LOADING_FROM_URL;
+  load_start_time_ = base::TimeTicks::Now();
   next_earliest_download_time_ =
-      download_start_time_ + base::TimeDelta::FromMinutes(kMinRetryDelayMins);
-  url_fetcher_->Request(
-      model_url_, base::Bind(&RankerModelLoader::Backend::OnDownloadComplete,
-                             base::Unretained(this)));
+      load_start_time_ + base::TimeDelta::FromMinutes(kMinRetryDelayMins);
+  bool request_started = url_fetcher_->Request(
+      model_url_, base::Bind(&RankerModelLoader::OnURLFetched,
+                             weak_ptr_factory_.GetWeakPtr()));
+
+  // |url_fetcher_| maintains a request retry counter. If all allowed attempts
+  // have already been exhausted, then the loader is finished and has abandoned
+  // loading the model.
+  if (!request_started) {
+    DVLOG(2) << "Model download abandoned.";
+    ReportModelStatus(RankerModelStatus::MODEL_LOADING_ABANDONED);
+    state_ = LoaderState::FINISHED;
+  }
 }
 
-void RankerModelLoader::Backend::OnDownloadComplete(int /* id */,
-                                                    bool success,
-                                                    const std::string& data) {
+void RankerModelLoader::OnURLFetched(int /* id */,
+                                     bool success,
+                                     const std::string& data) {
   DCHECK(sequence_checker_.CalledOnValidSequence());
+  DCHECK_EQ(state_, LoaderState::LOADING_FROM_URL);
 
   // Record the duration of the download.
-  base::TimeDelta duration = base::TimeTicks::Now() - download_start_time_;
-  base::HistogramBase* counter = base::Histogram::FactoryTimeGet(
-      uma_prefix_ + kDownloadTimerHistogram,
-      base::TimeDelta::FromMilliseconds(10),
-      base::TimeDelta::FromMilliseconds(200000), 100,
-      base::HistogramBase::kUmaTargetedHistogramFlag);
-  if (counter)
-    counter->AddTime(duration);
+  RecordTimerHistogram(uma_prefix_ + kDownloadTimerHistogram,
+                       base::TimeTicks::Now() - load_start_time_);
 
-  // On failure, we just abort. The TranslateRanker will retry on a subsequent
-  // translation opportunity. The TranslateURLFetcher enforces a limit for
-  // retried requests.
+  // On request failure, transition back to IDLE. The loader will retry, or
+  // enforce the max download attempts, later.
   if (!success || data.empty()) {
     DVLOG(2) << "Download from '" << model_url_ << "'' failed.";
+    ReportModelStatus(RankerModelStatus::DOWNLOAD_FAILED);
+    state_ = LoaderState::IDLE;
     return;
   }
 
-  auto model = CreateModel(data);
+  // Attempt to loads the model. If this fails, transition back to IDLE. The
+  // loader will retry, or enfore the max download attempts, later.
+  auto model = CreateAndValidateModel(data);
   if (!model) {
     DVLOG(2) << "Model from '" << model_url_ << "'' not valid.";
+    state_ = LoaderState::IDLE;
     return;
   }
 
-  url_fetcher_.reset();
-
+  // The model is valid. Update the metadata to track the source URL and
+  // download timestamp.
   auto* metadata = model->mutable_proto()->mutable_metadata();
   metadata->set_source(model_url_.spec());
   metadata->set_last_modified_sec(
       (base::Time::Now() - base::Time()).InSeconds());
 
+  // Cache the model to model_path_, in the background.
   if (!model_path_.empty()) {
-    DVLOG(2) << "Saving model from '" << model_url_ << "'' to '"
-             << model_path_.value() << "'.";
-    MyScopedHistogramTimer timer(uma_prefix_ + kWriteTimerHistogram);
-    base::ImportantFileWriter::WriteFileAtomically(model_path_,
-                                                   model->SerializeAsString());
+    background_task_runner_->PostTask(
+        FROM_HERE, base::BindOnce(&SaveToFile, model_url_, model_path_,
+                                  model->SerializeAsString(), uma_prefix_));
   }
 
-  // Notify the owner that a compatible model is available.
-  TransferModelToClient(std::move(model), true);
-}
-
-void RankerModelLoader::Backend::TransferModelToClient(
-    std::unique_ptr<chrome_intelligence::RankerModel> model,
-    bool is_finished) {
-  DCHECK(sequence_checker_.CalledOnValidSequence());
-  origin_task_runner_->PostTask(
-      FROM_HERE, base::Bind(internal_on_model_available_cb_,
-                            base::Passed(std::move(model)), is_finished));
-}
-
-// =============================================================================
-// RankerModelLoader
-
-RankerModelLoader::RankerModelLoader(
-    const ValidateModelCallback& validate_model_cb,
-    const OnModelAvailableCallback& on_model_available_cb,
-    const base::FilePath& model_path,
-    const GURL& model_url,
-    const std::string& uma_prefix)
-    : backend_task_runner_(base::CreateSequencedTaskRunnerWithTraits(
-          base::TaskTraits()
-              .MayBlock()
-              .WithPriority(base::TaskPriority::BACKGROUND)
-              .WithShutdownBehavior(
-                  base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN))),
-      weak_ptr_factory_(this) {
-  auto internal_on_model_available_cb =
-      base::Bind(&RankerModelLoader::InternalOnModelAvailable,
-                 weak_ptr_factory_.GetWeakPtr(), on_model_available_cb);
-  backend_ = base::MakeUnique<Backend>(validate_model_cb,
-                                       internal_on_model_available_cb,
-                                       model_path, model_url, uma_prefix);
-}
-
-RankerModelLoader::~RankerModelLoader() {
-  DCHECK(sequence_checker_.CalledOnValidSequence());
-  // This is guaranteed to be sequenced after any pending backend operation.
-  backend_task_runner_->DeleteSoon(FROM_HERE, backend_.release());
-}
-
-void RankerModelLoader::Start() {
-  DCHECK(sequence_checker_.CalledOnValidSequence());
-  DCHECK_EQ(state_, LoaderState::NOT_STARTED);
-  state_ = LoaderState::RUNNING;
-  backend_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&RankerModelLoader::Backend::LoadFromFile,
-                            base::Unretained(backend_.get())));
+  // The loader is finished. Transfer the model to the client.
+  state_ = LoaderState::FINISHED;
+  on_model_available_cb_.Run(std::move(model));
 }
 
-void RankerModelLoader::NotifyOfRankerActivity() {
+std::unique_ptr<chrome_intelligence::RankerModel>
+RankerModelLoader::CreateAndValidateModel(const std::string& data) {
   DCHECK(sequence_checker_.CalledOnValidSequence());
-  if (state_ == LoaderState::RUNNING) {
-    backend_task_runner_->PostTask(
-        FROM_HERE, base::Bind(&RankerModelLoader::Backend::AsyncLoadFromURL,
-                              base::Unretained(backend_.get())));
+  MyScopedHistogramTimer timer(uma_prefix_ + kParsetimerHistogram);
+  auto model = RankerModel::FromString(data);
+  if (ReportModelStatus(model ? validate_model_cb_.Run(*model)
+                              : RankerModelStatus::PARSE_FAILED) !=
+      RankerModelStatus::OK) {
+    return nullptr;
   }
+  return model;
 }
 
-void RankerModelLoader::InternalOnModelAvailable(
-    const OnModelAvailableCallback& callback,
-    std::unique_ptr<RankerModel> model,
-    bool finished) {
+RankerModelStatus RankerModelLoader::ReportModelStatus(
+    RankerModelStatus model_status) {
   DCHECK(sequence_checker_.CalledOnValidSequence());
-  if (finished)
-    state_ = LoaderState::FINISHED;
-  if (model)
-    callback.Run(std::move(model));
+  base::HistogramBase* histogram = base::LinearHistogram::FactoryGet(
+      uma_prefix_ + kModelStatusHistogram, 1,
+      static_cast<int>(RankerModelStatus::MAX),
+      static_cast<int>(RankerModelStatus::MAX) + 1,
+      base::HistogramBase::kUmaTargetedHistogramFlag);
+  if (histogram)
+    histogram->Add(static_cast<int>(model_status));
+  return model_status;
 }
 
 }  // namespace translate