Fixed use-after-free in LoadCallback in bookmark_storage.cc

Note: this is a reland of https://codereview.chromium.org/373153002/
after plugging a memory leak.

BookmarkStorage isn't ref counted anymore since
https://codereview.chromium.org/370323002, and the LoadCallback() task
now gets a WeakPtr to the owning BookmarkStorage. However, it gets a
raw pointer to the BookmarkLoadDetails object, which is still owned
by BookmarkStorage and may have been destroyed when the background
task runs.

This happened on iOS tests after a recent merge.

TBR=sky@chromium.org
BUG=165760

Review URL: https://codereview.chromium.org/376203002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@282097 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/bookmarks/chrome_bookmark_client.cc

@@ -17,8 +17,6 @@
 #include "chrome/browser/policy/profile_policy_connector.h"
 #include "chrome/browser/policy/profile_policy_connector_factory.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/profiles/startup_task_runner_service.h"
-#include "chrome/browser/profiles/startup_task_runner_service_factory.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/browser/bookmark_node.h"
 #include "components/history/core/browser/url_database.h"
@@ -171,8 +169,6 @@ bookmarks::LoadExtraCallback ChromeBookmarkClient::GetLoadExtraNodesCallback() {
 
   return base::Bind(
       &ChromeBookmarkClient::LoadExtraNodes,
-      StartupTaskRunnerServiceFactory::GetForProfile(profile_)
-          ->GetBookmarkTaskRunner(),
       base::Passed(&managed),
       base::Passed(managed_bookmarks_tracker_->GetInitialManagedBookmarks()));
 }
@@ -238,11 +234,9 @@ void ChromeBookmarkClient::BookmarkModelLoaded(BookmarkModel* model,
 
 // static
 bookmarks::BookmarkPermanentNodeList ChromeBookmarkClient::LoadExtraNodes(
-    const scoped_refptr<base::DeferredSequencedTaskRunner>& profile_io_runner,
     scoped_ptr<BookmarkPermanentNode> managed_node,
     scoped_ptr<base::ListValue> initial_managed_bookmarks,
     int64* next_node_id) {
-  DCHECK(profile_io_runner->RunsTasksOnCurrentThread());
   // Load the initial contents of the |managed_node| now, and assign it an
   // unused ID.
   int64 managed_id = *next_node_id;

chrome/browser/bookmarks/chrome_bookmark_client.h

@@ -9,7 +9,6 @@
 
 #include "base/deferred_sequenced_task_runner.h"
 #include "base/macros.h"
-#include "base/memory/ref_counted.h"
 #include "components/bookmarks/browser/base_bookmark_model_observer.h"
 #include "components/bookmarks/browser/bookmark_client.h"
 #include "components/policy/core/browser/managed_bookmarks_tracker.h"
@@ -83,7 +82,6 @@ class ChromeBookmarkClient : public bookmarks::BookmarkClient,
 
   // Helper for GetLoadExtraNodesCallback().
   static bookmarks::BookmarkPermanentNodeList LoadExtraNodes(
-      const scoped_refptr<base::DeferredSequencedTaskRunner>& profile_io_runner,
       scoped_ptr<BookmarkPermanentNode> managed_node,
       scoped_ptr<base::ListValue> initial_managed_bookmarks,
       int64* next_node_id);

components/bookmarks/browser/bookmark_storage.cc

@@ -49,7 +49,7 @@ void AddBookmarksToIndex(BookmarkLoadDetails* details,
 
 void LoadCallback(const base::FilePath& path,
                   const base::WeakPtr<BookmarkStorage>& storage,
-                  BookmarkLoadDetails* details,
+                  scoped_ptr<BookmarkLoadDetails> details,
                   base::SequencedTaskRunner* task_runner) {
   startup_metric_utils::ScopedSlowStartupUMA
       scoped_timer("Startup.SlowStartupBookmarksLoad");
@@ -96,17 +96,18 @@ void LoadCallback(const base::FilePath& path,
 
   if (load_index) {
     TimeTicks start_time = TimeTicks::Now();
-    AddBookmarksToIndex(details, details->bb_node());
-    AddBookmarksToIndex(details, details->other_folder_node());
-    AddBookmarksToIndex(details, details->mobile_folder_node());
+    AddBookmarksToIndex(details.get(), details->bb_node());
+    AddBookmarksToIndex(details.get(), details->other_folder_node());
+    AddBookmarksToIndex(details.get(), details->mobile_folder_node());
     for (size_t i = 0; i < extra_nodes.size(); ++i)
-      AddBookmarksToIndex(details, extra_nodes[i]);
+      AddBookmarksToIndex(details.get(), extra_nodes[i]);
     UMA_HISTOGRAM_TIMES("Bookmarks.CreateBookmarkIndexTime",
                         TimeTicks::Now() - start_time);
   }
 
   task_runner->PostTask(FROM_HERE,
-                        base::Bind(&BookmarkStorage::OnLoadFinished, storage));
+                        base::Bind(&BookmarkStorage::OnLoadFinished, storage,
+                                   base::Passed(&details)));
 }
 
 }  // namespace
@@ -162,14 +163,11 @@ BookmarkStorage::~BookmarkStorage() {
 void BookmarkStorage::LoadBookmarks(
     scoped_ptr<BookmarkLoadDetails> details,
     const scoped_refptr<base::SequencedTaskRunner>& task_runner) {
-  DCHECK(!details_.get());
-  DCHECK(details);
-  details_ = details.Pass();
   sequenced_task_runner_->PostTask(FROM_HERE,
                                    base::Bind(&LoadCallback,
                                               writer_.path(),
                                               weak_factory_.GetWeakPtr(),
-                                              details_.get(),
+                                              base::Passed(&details),
                                               task_runner));
 }
 
@@ -193,11 +191,11 @@ bool BookmarkStorage::SerializeData(std::string* output) {
   return serializer.Serialize(*(value.get()));
 }
 
-void BookmarkStorage::OnLoadFinished() {
+void BookmarkStorage::OnLoadFinished(scoped_ptr<BookmarkLoadDetails> details) {
   if (!model_)
     return;
 
-  model_->DoneLoading(details_.Pass());
+  model_->DoneLoading(details.Pass());
 }
 
 bool BookmarkStorage::SaveNow() {

components/bookmarks/browser/bookmark_storage.h

@@ -159,7 +159,7 @@ class BookmarkStorage : public base::ImportantFileWriter::DataSerializer {
   void BookmarkModelDeleted();
 
   // Callback from backend after loading the bookmark file.
-  void OnLoadFinished();
+  void OnLoadFinished(scoped_ptr<BookmarkLoadDetails> details);
 
   // ImportantFileWriter::DataSerializer implementation.
   virtual bool SerializeData(std::string* output) OVERRIDE;
@@ -175,9 +175,6 @@ class BookmarkStorage : public base::ImportantFileWriter::DataSerializer {
   // Helper to write bookmark data safely.
   base::ImportantFileWriter writer_;
 
-  // See class description of BookmarkLoadDetails for details on this.
-  scoped_ptr<BookmarkLoadDetails> details_;
-
   // Sequenced task runner where file I/O operations will be performed at.
   scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_;