Avoid touching bookmarks::ModelLoader's ref count before it's fully constructed

ModelLoader is a ref counted type, and its first reference used to be
taken in its constructor through base::BindOnce. The reference was
passed to a task runner, and released after the task has run.

However, if the PostTask failed or the posted task ran soon before the
construction had completed, the ModelLoader instance can be destroyed
before another reference is made on the original sequence. So,
`new ModelLoader` can return a stale pointer.

This CL adds a static constructor to ModelLoader, and makes the first
reference on the original sequence before passing a reference to the
other sequence.

Bug: 866456
Change-Id: I4d3c954ca39b7187fbd651c498e17273024c9968
Reviewed-on: https://chromium-review.googlesource.com/1151173
Commit-Queue: Taiju Tsuiki <tzik@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578537}

components/bookmarks/browser/bookmark_model.cc

@@ -154,7 +154,7 @@ void BookmarkModel::Load(
   store_ = std::make_unique<BookmarkStorage>(this, profile_path,
                                              io_task_runner.get());
   // Creating ModelLoader schedules the load on |io_task_runner|.
-  model_loader_ = base::MakeRefCounted<ModelLoader>(
+  model_loader_ = ModelLoader::Create(
       profile_path.Append(kBookmarksFileName), io_task_runner.get(),
       std::make_unique<BookmarkLoadDetails>(client_.get()),
       base::BindOnce(&BookmarkModel::DoneLoading, weak_factory_.GetWeakPtr()));

components/bookmarks/browser/model_loader.cc

@@ -12,23 +12,31 @@
 
 namespace bookmarks {
 
-ModelLoader::ModelLoader(const base::FilePath& profile_path,
-                         base::SequencedTaskRunner* load_sequenced_task_runner,
-                         std::unique_ptr<BookmarkLoadDetails> details,
-                         LoadCallback callback)
-    : loaded_signal_(base::WaitableEvent::ResetPolicy::MANUAL,
-                     base::WaitableEvent::InitialState::NOT_SIGNALED) {
+// static
+scoped_refptr<ModelLoader> ModelLoader::Create(
+    const base::FilePath& profile_path,
+    base::SequencedTaskRunner* load_sequenced_task_runner,
+    std::unique_ptr<BookmarkLoadDetails> details,
+    LoadCallback callback) {
+  // Note: base::MakeRefCounted is not available here, as ModelLoader's
+  // constructor is private.
+  auto model_loader = base::WrapRefCounted(new ModelLoader());
   load_sequenced_task_runner->PostTask(
       FROM_HERE,
-      base::BindOnce(&ModelLoader::DoLoadOnBackgroundThread, this, profile_path,
-                     base::ThreadTaskRunnerHandle::Get(), std::move(details),
-                     std::move(callback)));
+      base::BindOnce(&ModelLoader::DoLoadOnBackgroundThread, model_loader,
+                     profile_path, base::ThreadTaskRunnerHandle::Get(),
+                     std::move(details), std::move(callback)));
+  return model_loader;
 }
 
 void ModelLoader::BlockTillLoaded() {
   loaded_signal_.Wait();
 }
 
+ModelLoader::ModelLoader()
+    : loaded_signal_(base::WaitableEvent::ResetPolicy::MANUAL,
+                     base::WaitableEvent::InitialState::NOT_SIGNALED) {}
+
 ModelLoader::~ModelLoader() = default;
 
 void ModelLoader::DoLoadOnBackgroundThread(

components/bookmarks/browser/model_loader.h

@@ -33,10 +33,11 @@ class ModelLoader : public base::RefCountedThreadSafe<ModelLoader> {
   // Creates the ModelLoader, and schedules loading on
   // |load_sequenced_task_runner|. |callback| is run once loading
   // completes (on the main thread).
-  ModelLoader(const base::FilePath& profile_path,
-              base::SequencedTaskRunner* load_sequenced_task_runner,
-              std::unique_ptr<BookmarkLoadDetails> details,
-              LoadCallback callback);
+  static scoped_refptr<ModelLoader> Create(
+      const base::FilePath& profile_path,
+      base::SequencedTaskRunner* load_sequenced_task_runner,
+      std::unique_ptr<BookmarkLoadDetails> details,
+      LoadCallback callback);
 
   // Blocks until loaded. This is intended for usage on a thread other than
   // the main thread.
@@ -50,6 +51,7 @@ class ModelLoader : public base::RefCountedThreadSafe<ModelLoader> {
 
  private:
   friend class base::RefCountedThreadSafe<ModelLoader>;
+  ModelLoader();
   ~ModelLoader();
 
   // Performs the load on a background thread.