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

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/373153002

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

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_;