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

components/translate/core/browser/ranker_model_loader.h

Index: components/translate/core/browser/ranker_model_loader.h
diff --git a/components/translate/core/browser/ranker_model_loader.h b/components/translate/core/browser/ranker_model_loader.h
index 0fd88069dc8d04f7ce09c1362b989f05d8833a3d..99fe0445a6555417dd04255fd378b1ca4bec5fe9 100644
--- a/components/translate/core/browser/ranker_model_loader.h
+++ b/components/translate/core/browser/ranker_model_loader.h
@@ -8,15 +8,16 @@
 #include <memory>
 #include <string>
 
+#include "base/bind.h"

[fdoray, 2017/04/25 14:50:44]

Not used

[Roger McFarlane (Chromium), 2017/04/25 21:50:39]

Done.

 #include "base/callback.h"
+#include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
-
-class GURL;
+#include "base/time/time.h"
+#include "url/gurl.h"
 
 namespace base {
-class FilePath;
 class SequencedTaskRunner;
 }  // namespace base
 
@@ -26,6 +27,8 @@ class RankerModel;
 
 namespace translate {
 
+class TranslateURLFetcher;
+
 // Enumeration denoting the outcome of an attempt to download the model. This
 // must be kept in sync with the RankerModelStatus enum in histograms.xml
 enum class RankerModelStatus {
@@ -35,6 +38,8 @@ enum class RankerModelStatus {
   PARSE_FAILED = 3,
   VALIDATION_FAILED = 4,
   INCOMPATIBLE = 5,
+  LOAD_FROM_CACHE_FAILED = 6,
+  MODEL_LOADING_ABANDONED = 7,
 
   // Insert new values above this line.
   MAX
@@ -46,21 +51,20 @@ class RankerModelLoader {
  public:
   // Callback to validate a ranker model on behalf of the model loader client.
   // For example, the callback might validate that the model is compatible with
-  // the features generated when ranking translation offerings. This callback
-  // may be called on any sequence and must, therefore, be thread-safe. This
-  // callback may be invoked after the RankerModelLoader has been destroyed
-  // and must, therefore, only access memory that it can guarantee to be
-  // valid.
-  using ValidateModelCallback = base::Callback<RankerModelStatus(
-      const chrome_intelligence::RankerModel& model)>;
+  // the features generated when ranking translation offerings.  This will be
+  // called on the sequence on which the model loader was constructed.  This
+  // callback may be invoked after the RankerModelLoader has been destroyed and

[fdoray, 2017/04/25 14:50:44]

"may be invoked after the RankerModelLoader has been destroyed" no longer true

[Roger McFarlane (Chromium), 2017/04/25 21:50:39]

Done.

+  // must, therefore, only access memory that it can guarantee to be valid.
+  using ValidateModelCallback = base::RepeatingCallback<RankerModelStatus(
+      const chrome_intelligence::RankerModel&)>;
 
   // Called to transfer 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.  This callback may be invoked after the RankerModelLoader has

[fdoray, 2017/04/25 14:50:43]

"may be invoked after the RankerModelLoader has been destroyed" no longer true

[Roger McFarlane (Chromium), 2017/04/25 21:50:39]

Done.

   // been destroyed and must, therefore, only access memory that it can
   // guarantee to be valid.
-  using OnModelAvailableCallback = base::Callback<void(
-      std::unique_ptr<chrome_intelligence::RankerModel> model)>;
+  using OnModelAvailableCallback = base::RepeatingCallback<void(
+      std::unique_ptr<chrome_intelligence::RankerModel>)>;
 
   // |validate_model_callback| may be called on any sequence; it must be thread
   // safe.
@@ -87,44 +91,100 @@ class RankerModelLoader {
 
   ~RankerModelLoader();
 
-  // Asynchronously initiates loading the model from the cache file path and URL
-  // previously configured.
-  void Start();
-
   // Call this method periodically to notify the model loader the ranker is
-  // actively in use. The user's engagement with the feature is used as a proxy
-  // for network activity. If a model download is pending, this will trigger
-  // (subject to retry and frequency limits) a model download attempt.
+  // actively in use. The user's engagement with the ranked feature is used
+  // as a proxy for network availability and activity. If a model download
+  // is pending, this will trigger (subject to retry and frequency limits) a
+  // model download attempt.
   void NotifyOfRankerActivity();
 
  private:
-  // The model loader backend to which the actual loading functionality is
-  // delegated.
-  class Backend;
-  friend class Backend;
-
   // A enum to track the current loader state.
-  enum class LoaderState { NOT_STARTED, RUNNING, FINISHED };
+  enum class LoaderState {
+    // The loader is newly created and has not started trying to load the model.
+    // This state can transition to LOADING_FROM_FILE or, if model_path_ is
+    // empty, to LOADING_FROM_URL. If both model_path_ and model_url_ are
+    // empty/invalid then it can transition to FINISHED.
+    NOT_STARTED,
+    // The loader is busy loading the model from model_path_ in the background.
+    // This state can transition to FINISHED if the loaded model is compatible
+    // and up to date; otherwise, this state can transition to IDLE.
+    LOADING_FROM_FILE,
+    // The loader is not currently busy. The loader can transition to the
+    // LOADING_FROM_URL_ state if |model_url_| is valid; the loader can also
+    // transition to FINISHED if it the maximum number of download attempts
+    // has been reached.
+    IDLE,
+    // The loader is busy loading the model from model_url_ in the background.
+    // This state can transition to FINISHED if the loaded model is valid;
+    // otherwise, this state can re-transition to IDLE.
+    LOADING_FROM_URL,
+    // The loader has finished. This is the terminal state.
+    FINISHED
+  };
+
+  // Asynchronously initiates loading the model from model_path_;
+  void StartLoadFromFile();
+
+  // Called when the background worker has finished loading |data| from
+  // |model_path_|. If |data| is empty, the load from |model_path_| failed.
+  void OnFileLoaded(const std::string& data);
+
+  // Asynchronously initiates loading the model from model_url_;
+  void StartLoadFromURL();
+
+  // Called when |url_fetcher_| has finised loading |data| from |model_url_|.
+  //
+  // This call signature is mandated by TranslateURLFetcher.
+  //
+  // id - the id given to the TranslateURLFetcher on creation
+  // success - true of the download was successful
+  // data - the body of the downloads response
+  void OnURLFetched(int id, bool success, const std::string& data);
+
+  // Parse |data| and return a validated model. Returns nullptr on failure.
+  std::unique_ptr<chrome_intelligence::RankerModel> CreateAndValidateModel(
+      const std::string& data);
 
-  // Helper method to forward OnModelAvailable callbacks while noting whether
-  // or not the loader has finished its work.
-  void InternalOnModelAvailable(
-      const OnModelAvailableCallback& callback,
-      std::unique_ptr<chrome_intelligence::RankerModel> model,
-      bool finished);
+  // Helper function to log |model_status| to UMA and return it.
+  RankerModelStatus ReportModelStatus(RankerModelStatus model_status);
 
   // Validates that ranker model loader tasks are all performed on the same
   // sequence.
   base::SequenceChecker sequence_checker_;
 
-  // The task runner on which backend tasks are performed.
-  const scoped_refptr<base::SequencedTaskRunner> backend_task_runner_;
+  // The task runner on which background tasks are performed.
+  const scoped_refptr<base::SequencedTaskRunner> background_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 OnModelAvailableCallback 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_|.
+  std::unique_ptr<TranslateURLFetcher> url_fetcher_;
+
+  // The next time before which no new attempts to download the model should be
+  // attempted.
+  base::TimeTicks next_earliest_download_time_;
 
-  // The model loader backend to which the actual loading functionality is
-  // delegated. |backend_| may outlive its RankerModelLoader. When the
-  // RankerModelLoader is destroyed, ownership of |backend_| is transferred
-  // to a delete task posted |backend_task_runner_|.
-  std::unique_ptr<Backend> backend_;
+  // Tracks the last time of the last attempt to load a model, either form file
+  // of from URL. Used for UMA reporting of load duration.
+  base::TimeTicks load_start_time_;
 
   // The current state of the loader.
   LoaderState state_ = LoaderState::NOT_STARTED;