Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Commit

Permalink
Revert of Fixed use-after-free in LoadCallback in bookmark_storage.cc (…
Browse files Browse the repository at this point in the history 
…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.
> 
> [email protected]
> BUG=165760
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281830

[email protected],[email protected],[email protected],[email protected]
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
  • Loading branch information
[email protected] committed Jul 8, 2014
1 parent addd4ba commit 047a44d
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 11 deletions.
22 changes: 12 additions & 10 deletions components/bookmarks/browser/bookmark_storage.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ void AddBookmarksToIndex(BookmarkLoadDetails* details,

void LoadCallback(const base::FilePath& path,
const base::WeakPtr<BookmarkStorage>& storage,
scoped_ptr<BookmarkLoadDetails> details,
BookmarkLoadDetails* details,
base::SequencedTaskRunner* task_runner) {
startup_metric_utils::ScopedSlowStartupUMA
scoped_timer("Startup.SlowStartupBookmarksLoad");
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -163,11 +162,14 @@ 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(),
base::Passed(&details),
details_.get(),
task_runner));
}

Expand All @@ -191,11 +193,11 @@ bool BookmarkStorage::SerializeData(std::string* output) {
return serializer.Serialize(*(value.get()));
}

void BookmarkStorage::OnLoadFinished(scoped_ptr<BookmarkLoadDetails> details) {
void BookmarkStorage::OnLoadFinished() {
if (!model_)
return;

model_->DoneLoading(details.Pass());
model_->DoneLoading(details_.Pass());
}

bool BookmarkStorage::SaveNow() {
Expand Down
5 changes: 4 additions & 1 deletion components/bookmarks/browser/bookmark_storage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ class BookmarkStorage : public base::ImportantFileWriter::DataSerializer {
void BookmarkModelDeleted();

// Callback from backend after loading the bookmark file.
void OnLoadFinished(scoped_ptr<BookmarkLoadDetails> details);
void OnLoadFinished();

// ImportantFileWriter::DataSerializer implementation.
virtual bool SerializeData(std::string* output) OVERRIDE;
Expand All @@ -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<BookmarkLoadDetails> details_;

// Sequenced task runner where file I/O operations will be performed at.
scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_;

Expand Down

0 comments on commit 047a44d

Please sign in to comment.