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

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

[Joao da Silva, 2014-07-08 14:50:46]

Justin stumbled upon a couple of crashes with TestingProfiles after merging
trunk for iOS today; calling WaitForBookmarkModelToLoad() fixed those issues,
and I think they were caused by this use-after-free (from
https://codereview.chromium.org/370323002).

PTAL, thanks!

[gab, 2014-07-08 15:03:40]

lgtm, thanks for spotting this regression; the lifetime management in bookmark
code is ugly IMO (nobody's fault, just very old code I assume).

Also, back to you, ping on request for you on
https://codereview.chromium.org/372663002/

[Joao da Silva, 2014-07-08 15:07:29]

The CQ bit was checked by joaodasilva@chromium.org

[Joao da Silva, 2014-07-08 15:07:45]

On 2014/07/08 15:03:40, gab wrote:
> lgtm, thanks for spotting this regression; the lifetime management in bookmark
> code is ugly IMO (nobody's fault, just very old code I assume).
> 
> Also, back to you, ping on request for you on
> https://codereview.chromium.org/372663002/

Thanks Gab. Just posted a long reply on that one too :-)

[commit-bot: I haz the power, 2014-07-08 15:08:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/373153002/1

[Sigurður Ásgeirsson, 2014-07-08 15:14:51]

lgtm. Thanks for the fix.

[Joao da Silva, 2014-07-08 16:48:24]

sky@ to TBR

[commit-bot: I haz the power, 2014-07-08 21:09:58]

Change committed as 281830

[Cait (Slow), 2014-07-08 21:58:43]

A revert of this CL has been created in
https://codereview.chromium.org/379643002/ by caitkp@chromium.org.

The reason for reverting is: 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

.