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

chrome/browser/bookmarks/chrome_bookmark_client.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/bookmarks/chrome_bookmark_client.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "base/values.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/favicon/favicon_changed_details.h"
 #include "chrome/browser/favicon/favicon_service.h"
 #include "chrome/browser/favicon/favicon_service_factory.h"
 #include "chrome/browser/history/history_service.h"
 #include "chrome/browser/history/history_service_factory.h"
 #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"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/user_metrics.h"
 #include "grit/components_strings.h"
 #include "policy/policy_constants.h"
 #include "ui/base/l10n/l10n_util.h"
@@ skipping 132 matching lines @@
 bookmarks::LoadExtraCallback ChromeBookmarkClient::GetLoadExtraNodesCallback() {
   // Create the managed_node now; it will be populated in the LoadExtraNodes
   // callback.
   // The ownership of managed_node_ is in limbo until LoadExtraNodes runs,
   // so we leave it in the care of the closure meanwhile.
   scoped_ptr<BookmarkPermanentNode> managed(new BookmarkPermanentNode(0));
   managed_node_ = managed.get();
 
   return base::Bind(
       &ChromeBookmarkClient::LoadExtraNodes,
-      StartupTaskRunnerServiceFactory::GetForProfile(profile_)
-          ->GetBookmarkTaskRunner(),

[Joao da Silva, 2014/07/09 13:58:16]

This is what caused the leak:

- this callback is owned by BookmarkLoadDetails

- BookmarkLoadDetails is owned by the LoadCallback task, which is sent to this
task runner

- some tests delete their ProfileImpls before spinning the loops, so the Profile
that owns the StartupTaskRunnerService is deleted (and the service too), but now
the deferred_task_runner_ that was created by the service stays around because
it contains a task that references the runner itself

- that task never runs (and thus never releases its reference to the task
runner) because StartupTaskRunnerService::Start() is never called (this is
usually done by the profile_manager, but some tests create Profile directly and
bypass the manager)

       base::Passed(&managed),
       base::Passed(managed_bookmarks_tracker_->GetInitialManagedBookmarks()));
 }
 
 bool ChromeBookmarkClient::CanSetPermanentNodeTitle(
     const BookmarkNode* permanent_node) {
   // The |managed_node_| can have its title updated if the user signs in or
   // out.
   return !IsDescendantOfManagedNode(permanent_node) ||
          permanent_node == managed_node_;
@@ skipping 45 matching lines @@
 void ChromeBookmarkClient::BookmarkModelLoaded(BookmarkModel* model,
                                                bool ids_reassigned) {
   // Start tracking the managed bookmarks. This will detect any changes that
   // may have occurred while the initial managed bookmarks were being loaded
   // on the background.
   managed_bookmarks_tracker_->Init(managed_node_);
 }
 
 // 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;
   managed_node->set_id(managed_id);
   *next_node_id = policy::ManagedBookmarksTracker::LoadInitial(
       managed_node.get(), initial_managed_bookmarks.get(), managed_id + 1);
   managed_node->set_visible(!managed_node->empty());
   managed_node->SetTitle(
       l10n_util::GetStringUTF16(IDS_BOOKMARK_BAR_MANAGED_FOLDER_DEFAULT_NAME));
 
   bookmarks::BookmarkPermanentNodeList extra_nodes;
   // Ownership of the managed node passed to the caller.
   extra_nodes.push_back(managed_node.release());
 
   return extra_nodes.Pass();
 }
 
 std::string ChromeBookmarkClient::GetManagedBookmarksDomain() {
   policy::ProfilePolicyConnector* connector =
       policy::ProfilePolicyConnectorFactory::GetForProfile(profile_);
   if (connector->IsPolicyFromCloudPolicy(policy::key::kManagedBookmarks))
     return connector->GetManagementDomain();
   return std::string();
 }