From 047a44d2056d0a9af8dfc0e8c3708b33ba5e274f Mon Sep 17 00:00:00 2001 From: "caitkp@chromium.org" Date: Tue, 8 Jul 2014 22:02:46 +0000 Subject: [PATCH] Revert of Fixed use-after-free in LoadCallback in bookmark_storage.cc (https://codereview.chromium.org/373153002/) Reason for revert: Causing memory leaks: Indirect leak of 328 byte(s) in 1 object(s) allocated from: #0 0x515feb in operator new(unsigned long) /usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:55 #1 0xc334cfb in BookmarkModel::CreatePermanentNode(BookmarkNode::Type) components/bookmarks/browser/bookmark_model.cc:910 #2 0xc326772 in BookmarkModel::CreateLoadDetails(std::string const&) components/bookmarks/browser/bookmark_model.cc:1006 #3 0xc326553 in BookmarkModel::Load(PrefService*, std::string const&, base::FilePath const&, scoped_refptr\u003Cbase::SequencedTaskRunner> const&, scoped_refptr\u003Cbase::SequencedTaskRunner> const&) components/bookmarks/browser/bookmark_model.cc:151 #4 0x232a4d7 in BookmarkModelFactory::BuildServiceInstanceFor(content::BrowserContext*) const chrome/browser/bookmarks/bookmark_model_factory.cc:65 #5 0xc0bc85f in BrowserContextKeyedServiceFactory::GetServiceForBrowserContext(content::BrowserContext*, bool) components/keyed_service/content/browser_context_keyed_service_factory.cc:91 #6 0x260de89 in ProfileImpl::DoFinalInit() chrome/browser/profiles/profile_impl.cc:657 #7 0x260beb5 in ProfileImpl::OnPrefsLoaded(bool) chrome/browser/profiles/profile_impl.cc:881 #8 0x260ae3e in ProfileImpl::ProfileImpl(base::FilePath const&, Profile::Delegate*, Profile::CreateMode, base::SequencedTaskRunner*) chrome/browser/profiles/profile_impl.cc:492 #9 0x2608ddb in Profile::CreateProfile(base::FilePath const&, Profile::Delegate*, Profile::CreateMode) chrome/browser/profiles/profile_impl.cc:297 #10 0x18de165 in ProfileBrowserTest::CreateProfile(base::FilePath const&, Profile::Delegate*, Profile::CreateMode) chrome/browser/profiles/profile_browsertest.cc:106 #11 0x18df8f8 in ProfileBrowserTest_CreateOldProfileSynchronous_Test::RunTestOnMainThread() chrome/browser/profiles/profile_browsertest.cc:153 #12 0x33f5434 in InProcessBrowserTest::RunTestOnMainThreadLoop() chrome/test/base/in_process_browser_test.cc:427 #13 0x2e6ca18 in Run base/callback.h:401 #14 0x2e6ca18 in ChromeBrowserMainParts::PreMainMessageLoopRunImpl() chrome/browser/chrome_browser_main.cc:1548 #15 0x2e69666 in ChromeBrowserMainParts::PreMainMessageLoopRun() chrome/browser/chrome_browser_main.cc:975 #16 0x59381d7 in content::BrowserMainLoop::PreMainMessageLoopRun() content/browser/browser_main_loop.cc:694 #17 0x5c8f207 in Run base/callback.h:401 #18 0x5c8f207 in content::StartupTaskRunner::RunAllTasksNow() content/browser/startup_task_runner.cc:45 #19 0x59342e0 in content::BrowserMainLoop::CreateStartupTasks() content/browser/browser_main_loop.cc:594 #20 0x5e8a153 in content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) content/browser/browser_main_runner.cc:106 #21 0xf0228a0 in content::BrowserMain(content::MainFunctionParams const&) content/browser/browser_main.cc:22 #22 0xef6cbc4 in content::ContentMainRunnerImpl::Run() content/app/content_main_runner.cc:763 #23 0xef69d3f in content::ContentMain(content::ContentMainParams const&) content/app/content_main.cc:19 #24 0xcf5b57f in content::BrowserTestBase::SetUp() content/public/test/browser_test_base.cc:253 #25 0x33f2563 in InProcessBrowserTest::SetUp() chrome/test/base/in_process_browser_test.cc:210 #26 0x3e81941 in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2045 #27 0x3e81941 in testing::Test::Run() testing/gtest/src/gtest.cc:2057 #28 0x3e83cd9 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2237 #29 0x3e84a66 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2344 #30 0x3e97b7a in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4065 #31 0x3e971b0 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2045 #32 0x3e971b0 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697 #33 0x359557c in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2231 #34 0x359557c in base::TestSuite::Run() base/test/test_suite.cc:227 Original issue's description: > 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 > > Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281830 TBR=siggi@chromium.org,gab@chromium.org,sky@chromium.org,joaodasilva@chromium.org NOTREECHECKS=true NOTRY=true BUG=165760 Review URL: https://codereview.chromium.org/379643002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@281843 0039d316-1c4b-4281-b951-d872f2087c98 --- .../bookmarks/browser/bookmark_storage.cc | 22 ++++++++++--------- .../bookmarks/browser/bookmark_storage.h | 5 ++++- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/components/bookmarks/browser/bookmark_storage.cc b/components/bookmarks/browser/bookmark_storage.cc index 50e066920be9f..f2f1f1bc58880 100644 --- a/components/bookmarks/browser/bookmark_storage.cc +++ b/components/bookmarks/browser/bookmark_storage.cc @@ -49,7 +49,7 @@ void AddBookmarksToIndex(BookmarkLoadDetails* details, void LoadCallback(const base::FilePath& path, const base::WeakPtr& storage, - scoped_ptr details, + BookmarkLoadDetails* details, base::SequencedTaskRunner* task_runner) { startup_metric_utils::ScopedSlowStartupUMA scoped_timer("Startup.SlowStartupBookmarksLoad"); @@ -96,18 +96,17 @@ void LoadCallback(const base::FilePath& path, if (load_index) { TimeTicks start_time = TimeTicks::Now(); - AddBookmarksToIndex(details.get(), details->bb_node()); - AddBookmarksToIndex(details.get(), details->other_folder_node()); - AddBookmarksToIndex(details.get(), details->mobile_folder_node()); + AddBookmarksToIndex(details, details->bb_node()); + AddBookmarksToIndex(details, details->other_folder_node()); + AddBookmarksToIndex(details, details->mobile_folder_node()); for (size_t i = 0; i < extra_nodes.size(); ++i) - AddBookmarksToIndex(details.get(), extra_nodes[i]); + AddBookmarksToIndex(details, extra_nodes[i]); UMA_HISTOGRAM_TIMES("Bookmarks.CreateBookmarkIndexTime", TimeTicks::Now() - start_time); } task_runner->PostTask(FROM_HERE, - base::Bind(&BookmarkStorage::OnLoadFinished, storage, - base::Passed(&details))); + base::Bind(&BookmarkStorage::OnLoadFinished, storage)); } } // namespace @@ -163,11 +162,14 @@ BookmarkStorage::~BookmarkStorage() { void BookmarkStorage::LoadBookmarks( scoped_ptr details, const scoped_refptr& task_runner) { + DCHECK(!details_.get()); + DCHECK(details); + details_ = details.Pass(); sequenced_task_runner_->PostTask(FROM_HERE, base::Bind(&LoadCallback, writer_.path(), weak_factory_.GetWeakPtr(), - base::Passed(&details), + details_.get(), task_runner)); } @@ -191,11 +193,11 @@ bool BookmarkStorage::SerializeData(std::string* output) { return serializer.Serialize(*(value.get())); } -void BookmarkStorage::OnLoadFinished(scoped_ptr details) { +void BookmarkStorage::OnLoadFinished() { if (!model_) return; - model_->DoneLoading(details.Pass()); + model_->DoneLoading(details_.Pass()); } bool BookmarkStorage::SaveNow() { diff --git a/components/bookmarks/browser/bookmark_storage.h b/components/bookmarks/browser/bookmark_storage.h index 39639fa30cee1..f7eee8cb5a93a 100644 --- a/components/bookmarks/browser/bookmark_storage.h +++ b/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(scoped_ptr details); + void OnLoadFinished(); // ImportantFileWriter::DataSerializer implementation. virtual bool SerializeData(std::string* output) OVERRIDE; @@ -175,6 +175,9 @@ 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 details_; + // Sequenced task runner where file I/O operations will be performed at. scoped_refptr sequenced_task_runner_;