Fix BookmarkPermanentNode leak in ChromeBookmarkClient

Ownership of the bookmark node is temporarily passed to the task to avoid leaking it if the task never runs. This fixes a leak that occurs in Profile browser tests.

R=gab@chromium.org,joaodasilva@chromium.org
TBR=brettw@chromium.org
BUG=391508
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281836

[Sigurður Ásgeirsson, 2014-07-07 17:59:13]

PTAL - this is a diff from https://codereview.chromium.org/349263004/, as I
intend to land that first, if possible.

[gab, 2014-07-07 18:12:47]

https://codereview.chromium.org/372663002/diff/80001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/chrome_bookmark_client.cc (right):

https://codereview.chromium.org/372663002/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:168: // The ownership of
maanaged_node_ is in limbo until LoadExtraNodes runs,
s/maanaged/managed

https://codereview.chromium.org/372663002/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:171: managed_node_ =
managed.get();
So is the only purpose of storing this as a member for pointer comparison later?
If so, would it make sense to use a void* (or perhaps a new type typedef'ed to
void* for clarity)?

I don't like that there is a fully typed pointer in this class which no one
should ever deref...

[gab, 2014-07-07 18:13:38]

https://codereview.chromium.org/372663002/diff/80001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/chrome_bookmark_client.cc (right):

https://codereview.chromium.org/372663002/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:9: #include
"base/debug/leak_annotations.h"
Also, rm this include

[Sigurður Ásgeirsson, 2014-07-07 18:40:19]

Thanks - PTAL.

https://codereview.chromium.org/372663002/diff/80001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/chrome_bookmark_client.cc (right):

https://codereview.chromium.org/372663002/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:9: #include
"base/debug/leak_annotations.h"
On 2014/07/07 18:13:38, gab wrote:
> Also, rm this include

Done.

https://codereview.chromium.org/372663002/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:168: // The ownership of
maanaged_node_ is in limbo until LoadExtraNodes runs,
On 2014/07/07 18:12:47, gab wrote:
> s/maanaged/managed

Done.

https://codereview.chromium.org/372663002/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:171: managed_node_ =
managed.get();
On 2014/07/07 18:12:47, gab wrote:
> So is the only purpose of storing this as a member for pointer comparison
later?
> If so, would it make sense to use a void* (or perhaps a new type typedef'ed to
> void* for clarity)?
> 
> I don't like that there is a fully typed pointer in this class which no one
> should ever deref...

The member is used by ChromeBookmarkClient::BookmarkModelLoaded, but I don't
understand the contract, so can't say which thread that'll run on. I can't
naively change this even to a const pointer...

[gab, 2014-07-07 19:00:47]

lgtm since you're only making lifetime management better without making the
overall model worse, but the current code is very confusing pointer management
IMO (at least there should be a big comment on |managed_node_| explaining why
it's kept as a member variable and how it should be used....

This code is asking for race conditions... 

CC joaodasilva who wrote this it looks like
(https://codereview.chromium.org/319543003): Joao, can you please fix the
lifetime management and usage of |managed_node_|? As-is it's kept as a member
variable and used on threads other than the one on which it is initially set
without any sort of memory barriers.

We temporarily suppressed the memory leak caused by this code and are now trying
to mitigate it without a leak-annotation, but really this shouldn't be that way
to begin with (or if the current code is sane as-is, as a reader I'm not
convinced of that, can you add a comment on the member variable to convince
me?).

Thanks!
Gab

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

On 2014/07/07 19:00:47, gab wrote:
> lgtm since you're only making lifetime management better without making the
> overall model worse, but the current code is very confusing pointer management
> IMO (at least there should be a big comment on |managed_node_| explaining why
> it's kept as a member variable and how it should be used....
> 
> This code is asking for race conditions... 
> 
> CC joaodasilva who wrote this it looks like
> (https://codereview.chromium.org/319543003): Joao, can you please fix the
> lifetime management and usage of |managed_node_|? As-is it's kept as a member
> variable and used on threads other than the one on which it is initially set
> without any sort of memory barriers.
> 
> We temporarily suppressed the memory leak caused by this code and are now
trying
> to mitigate it without a leak-annotation, but really this shouldn't be that
way
> to begin with (or if the current code is sane as-is, as a reader I'm not
> convinced of that, can you add a comment on the member variable to convince
> me?).
> 
> Thanks!
> Gab

This change lgtm, thanks for fixing.

I agree that it isn't obvious that the lifetime of managed_node_ is correct; the
code is a bit convoluted because we need to retain a handle to the node in
ChromeBookmarkClient, but the node needs to be loaded with the rest of the
bookmark model in a background thread (from bookmark_storage.cc).

This code is correct because:

- BookmarkModelFactory depends on ChromeBookmarkClientFactory;

- so the managed_node only gets created when the BookmarkModel is loaded;

- if shutdown happens during that load, then the node is now deleted by the
background task since it's owned by the callback (fixed in this CL)

- if the load is successful then the node is owned by the BookmarkModel;

- the BookmarkModel always outlives the ChromeBookmarkClient, so the
managed_node_ is always valid.

Regarding races: all the nodes of the BookmarkModel live in the UI thread. Only
LoadExtraNodes() gets called in a background thread when the BookmarkModel gets
loaded. But that task is synchronized with the BookmarkModel, so the node won't
be used until the model is loaded.

After all that said, I think this can be improved: the LoadExtraNode() callback
can post back to the ChromeBookmarkClient and tell it the ID of the managed
node; and then ChromeBookmarkClient::BookmarkModelLoaded() can get the handle by
looking it up by ID. Then the managed_node_ handle isn't set until the model is
fully loaded. WDYT?

[Sigurður Ásgeirsson, 2014-07-08 15:38:47]

The CQ bit was checked by siggi@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/siggi@chromium.org/372663002/100001

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

On 2014/07/08 15:07:04, Joao da Silva wrote:
> On 2014/07/07 19:00:47, gab wrote:
> > lgtm since you're only making lifetime management better without making the
> > overall model worse, but the current code is very confusing pointer
management
> > IMO (at least there should be a big comment on |managed_node_| explaining
why
> > it's kept as a member variable and how it should be used....
> > 
> > This code is asking for race conditions... 
> > 
> > CC joaodasilva who wrote this it looks like
> > (https://codereview.chromium.org/319543003): Joao, can you please fix the
> > lifetime management and usage of |managed_node_|? As-is it's kept as a
member
> > variable and used on threads other than the one on which it is initially set
> > without any sort of memory barriers.
> > 
> > We temporarily suppressed the memory leak caused by this code and are now
> trying
> > to mitigate it without a leak-annotation, but really this shouldn't be that
> way
> > to begin with (or if the current code is sane as-is, as a reader I'm not
> > convinced of that, can you add a comment on the member variable to convince
> > me?).
> > 
> > Thanks!
> > Gab
> 
> This change lgtm, thanks for fixing.
> 
> I agree that it isn't obvious that the lifetime of managed_node_ is correct;
the
> code is a bit convoluted because we need to retain a handle to the node in
> ChromeBookmarkClient, but the node needs to be loaded with the rest of the
> bookmark model in a background thread (from bookmark_storage.cc).
> 
> This code is correct because:
> 
> - BookmarkModelFactory depends on ChromeBookmarkClientFactory;
> 
> - so the managed_node only gets created when the BookmarkModel is loaded;
> 
> - if shutdown happens during that load, then the node is now deleted by the
> background task since it's owned by the callback (fixed in this CL)
> 
> - if the load is successful then the node is owned by the BookmarkModel;
> 
> - the BookmarkModel always outlives the ChromeBookmarkClient, so the
> managed_node_ is always valid.
> 
> Regarding races: all the nodes of the BookmarkModel live in the UI thread.
Only
> LoadExtraNodes() gets called in a background thread when the BookmarkModel
gets
> loaded. But that task is synchronized with the BookmarkModel, so the node
won't
> be used until the model is loaded.
> 
> After all that said, I think this can be improved: the LoadExtraNode()
callback
> can post back to the ChromeBookmarkClient and tell it the ID of the managed
> node; and then ChromeBookmarkClient::BookmarkModelLoaded() can get the handle
by
> looking it up by ID. Then the managed_node_ handle isn't set until the model
is
> fully loaded. WDYT?

Thanks, submitting as-is, as I don't have time to work on this further for now.

[gab, 2014-07-08 15:39:17]

On 2014/07/08 15:07:04, Joao da Silva wrote:
> On 2014/07/07 19:00:47, gab wrote:
> > lgtm since you're only making lifetime management better without making the
> > overall model worse, but the current code is very confusing pointer
management
> > IMO (at least there should be a big comment on |managed_node_| explaining
why
> > it's kept as a member variable and how it should be used....
> > 
> > This code is asking for race conditions... 
> > 
> > CC joaodasilva who wrote this it looks like
> > (https://codereview.chromium.org/319543003): Joao, can you please fix the
> > lifetime management and usage of |managed_node_|? As-is it's kept as a
member
> > variable and used on threads other than the one on which it is initially set
> > without any sort of memory barriers.
> > 
> > We temporarily suppressed the memory leak caused by this code and are now
> trying
> > to mitigate it without a leak-annotation, but really this shouldn't be that
> way
> > to begin with (or if the current code is sane as-is, as a reader I'm not
> > convinced of that, can you add a comment on the member variable to convince
> > me?).
> > 
> > Thanks!
> > Gab
> 
> This change lgtm, thanks for fixing.
> 
> I agree that it isn't obvious that the lifetime of managed_node_ is correct;
the
> code is a bit convoluted because we need to retain a handle to the node in
> ChromeBookmarkClient, but the node needs to be loaded with the rest of the
> bookmark model in a background thread (from bookmark_storage.cc).
> 
> This code is correct because:
> 
> - BookmarkModelFactory depends on ChromeBookmarkClientFactory;
> 
> - so the managed_node only gets created when the BookmarkModel is loaded;
> 
> - if shutdown happens during that load, then the node is now deleted by the
> background task since it's owned by the callback (fixed in this CL)
> 
> - if the load is successful then the node is owned by the BookmarkModel;
> 
> - the BookmarkModel always outlives the ChromeBookmarkClient, so the
> managed_node_ is always valid.
> 
> Regarding races: all the nodes of the BookmarkModel live in the UI thread.
Only
> LoadExtraNodes() gets called in a background thread when the BookmarkModel
gets
> loaded. But that task is synchronized with the BookmarkModel, so the node
won't
> be used until the model is loaded.
> 
> After all that said, I think this can be improved: the LoadExtraNode()
callback
> can post back to the ChromeBookmarkClient and tell it the ID of the managed
> node; and then ChromeBookmarkClient::BookmarkModelLoaded() can get the handle
by
> looking it up by ID. Then the managed_node_ handle isn't set until the model
is
> fully loaded. WDYT?

Yes, looking it up by ID (posted back from LoadExtraNode()) feels a LOT safer;
keeping this raw pointer may be provably "correct" for now, but I much prefer
safety by design than by proof :-)!

Thanks!
Gab

[gab, 2014-07-08 15:44:00]

> Thanks, submitting as-is, as I don't have time to work on this further for
now.

Yes, sgtm, Joao can you handle improving the design as discussed as a follow-up?

Thanks,
Gab

[commit-bot: I haz the power, 2014-07-08 17:37:00]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)

[commit-bot: I haz the power, 2014-07-08 17:40:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-08 17:40:53]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Sigurður Ásgeirsson, 2014-07-08 18:09:53]

The CQ bit was checked by siggi@chromium.org

[commit-bot: I haz the power, 2014-07-08 18:11:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/siggi@chromium.org/372663002/100001

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

Change committed as 281836