Bandaid a memory corruption bug by converting it to a leak.

Bandaid a memory corruption bug by converting it to a leak.

InfoBarService doesn't delete InfoBarDelegates directly (on shutdown or at any
other time), but rather relies on a listening InfoBarContainer to delete the
corresponding InfoBars, which in turn would delete the InfoBarDelegates.

If InfoBarContainer was destroyed before InfoBarService, it could still be
holding infobars, which would then leak, because there would be no container to
do the deletions at InfoBarService destruction time.  To prevent this leak,
InfoBarContainer was calling CloseSoon() and then Hide() on all InfoBars on
destruction, which would force them to be deleted and delete their delegates.

However, this (always!) caused a write-to-freed-memory when InfoBarService shut
down, because in this case it would still be holding pointers to all the
corresponding (deleted) InfoBarDelegates, and would attempt to access them
(specifically, to call clear_owner()) when removing them.  (The clear_owner()
call is sort of a hack; really, InfoBar and InfoBarDelegate should not have
separate members pointing to the owning InfoBarService.  Unfortunately this is
necessary to avoid some other bugs in the current system.)

The core problem was that CloseSoon() tells the InfoBar it's unowned, but in
this case the InfoBar isn't actually unowned yet.  Removing the CloseSoon() call
from InfoBarContainer fixes this, at the cost of re-introducing the leak
described above.

The correct fix is for InfoBarService to talk to InfoBars instead of
InfoBarDelegates, so that regardless of the destruction order of InfoBarService
versus InfoBarContainer, InfoBars will stay alive until they're both un-owned
and hidden, then delete themselves.  In this world we also don't need
InfoBarDelegates to have a pointer back to an "owning" InfoBarService; they're
owned by long-lived InfoBars.

If this sounds familiar, that's because it's a precise description of the new
ownership model that I've written (years ago) but still not yet landed.  (Though
I'm getting close; I'm now only blocked by some Android work.)  Until this model
is landed to fix the problem correctly, we have little choice but to accept the
potential leak in this scenario.

BUG=290976
TEST=none
R=erg@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223405

[Peter Kasting, 2013-09-14 01:03:07]

This effectively reverts part of http://codereview.chromium.org/7294006 .

I like how my CL description of the bug is ten times longer than the actual fix.

[Elliot Glaysher, 2013-09-16 06:32:54]

This does *not* LGTM, but I guess you should commit it anyway. :-/

[Elliot Glaysher, 2013-09-16 20:41:20]

On 2013/09/16 06:32:54, Elliot Glaysher wrote:
> This does *not* LGTM, but I guess you should commit it anyway. :-/

(For the record, this is a real LGTM.)

[Peter Kasting, 2013-09-16 20:42:34]

Committed patchset #1 manually as r223405 (presubmit successful).