Fix use-after-free in BrowserActionOverflowMenuController

Fix use-after-free in BrowserActionOverflowMenuController

When BrowserActionOverflowMenuController is deleted, it deletes its associated
IconUpdaters. When these go away, they remove themselves as the observer on the
associated BrowserActionViews. This led to the use-after-free if the
BrowserActionOverflowMenuController outlasts any of the BrowserActionViews.

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

[msw, 2014-07-09 16:56:27]

https://codereview.chromium.org/379483003/diff/1/chrome/browser/ui/views/tool...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/379483003/diff/1/chrome/browser/ui/views/tool...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:239: if
(overflow_menu_)
Can we simply close the overflow menu on DeleteBrowserActionViews?

[Devlin, 2014-07-09 19:23:28]

https://codereview.chromium.org/379483003/diff/1/chrome/browser/ui/views/tool...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/379483003/diff/1/chrome/browser/ui/views/tool...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:239: if
(overflow_menu_)
On 2014/07/09 16:56:27, msw wrote:
> Can we simply close the overflow menu on DeleteBrowserActionViews?

I'm not sure that we can safely - DeleteBrowserActionViews() can be called as a
result of moving browser actions, which can be triggered by the overflow menu. 
So it the flow would become:

OverflowMenu::OnDrop
  MoveBrowserAction()
  Finish()

MoveBrowserAction()
  DeleteBrowserActions()
  ...

DeleteBrowserActions()
  CloseOverflowMenu()
  Delete...

(All pseudocode, obviously).  The problem is that the menu would be canceled via
the call to close, but then would try to finish.

[msw, 2014-07-09 19:36:40]

It seems like IconUpdater (added in r253406 for Issue 319619) is really not the
right solution to this problem. We ought to delay showing the menu until the
icons are loaded, or just ignore the edge case defect that some icons might not
have been loaded yet and will be displayed blank. I'd like to hear what the
author or reviewers of that CL think. Otherwise, I guess this patch could be
okay.

[Devlin, 2014-07-09 20:25:56]

On 2014/07/09 19:36:40, msw wrote:
> It seems like IconUpdater (added in r253406 for Issue 319619) is really not
the
> right solution to this problem. We ought to delay showing the menu until the
> icons are loaded, or just ignore the edge case defect that some icons might
not
> have been loaded yet and will be displayed blank. I'd like to hear what the
> author or reviewers of that CL think. Otherwise, I guess this patch could be
> okay.

I'm not sure I love the idea of delaying opening the menu, and I personally
think the lack of icon looked weird enough in the original bug
(crbug.com/319619) that it warranted a fix.  But, that's just my two cents. :) 
I've added sky@ and finnur@ (original reviewers) for their opinions, but, while
we try to decide what the best approach is, can we land this one to fix the
immediate problem (the crash)?

(original review: https://codereview.chromium.org/148143004)

[msw, 2014-07-09 21:12:31]

On 2014/07/09 20:25:56, Devlin wrote:
> On 2014/07/09 19:36:40, msw wrote:
> > It seems like IconUpdater (added in r253406 for Issue 319619) is really not
> the
> > right solution to this problem. We ought to delay showing the menu until the
> > icons are loaded, or just ignore the edge case defect that some icons might
> not
> > have been loaded yet and will be displayed blank. I'd like to hear what the
> > author or reviewers of that CL think. Otherwise, I guess this patch could be
> > okay.
> 
> I'm not sure I love the idea of delaying opening the menu, and I personally
> think the lack of icon looked weird enough in the original bug
> (crbug.com/319619) that it warranted a fix.  But, that's just my two cents. :)


Then what about instead just showing a default icon in that rare edge case?
(newly installed extension, browser action pushed to overflow menu, quickly
invoking the overflow menu before we've loaded the icon)

> I've added sky@ and finnur@ (original reviewers) for their opinions, but,
while
> we try to decide what the best approach is, can we land this one to fix the
> immediate problem (the crash)?

I guess I can approve this if you dislike my alternatives and we don't hear back
soon.

[msw, 2014-07-09 21:13:45]

On 2014/07/09 21:12:31, msw wrote:
> On 2014/07/09 20:25:56, Devlin wrote:
> > On 2014/07/09 19:36:40, msw wrote:
> > > It seems like IconUpdater (added in r253406 for Issue 319619) is really
not
> > the
> > > right solution to this problem. We ought to delay showing the menu until
the
> > > icons are loaded, or just ignore the edge case defect that some icons
might
> > not
> > > have been loaded yet and will be displayed blank. I'd like to hear what
the
> > > author or reviewers of that CL think. Otherwise, I guess this patch could
be
> > > okay.
> > 
> > I'm not sure I love the idea of delaying opening the menu, and I personally
> > think the lack of icon looked weird enough in the original bug
> > (crbug.com/319619) that it warranted a fix.  But, that's just my two cents.
:)
> 
> 
> Then what about instead just showing a default icon in that rare edge case?
> (newly installed extension, browser action pushed to overflow menu, quickly
> invoking the overflow menu before we've loaded the icon)

Or, maybe omit entries from the overflow until their icons are loaded?

[Devlin, 2014-07-09 22:18:52]

On 2014/07/09 21:13:45, msw wrote:
> On 2014/07/09 21:12:31, msw wrote:
> > On 2014/07/09 20:25:56, Devlin wrote:
> > > On 2014/07/09 19:36:40, msw wrote:
> > > > It seems like IconUpdater (added in r253406 for Issue 319619) is really
> not
> > > the
> > > > right solution to this problem. We ought to delay showing the menu until
> the
> > > > icons are loaded, or just ignore the edge case defect that some icons
> might
> > > not
> > > > have been loaded yet and will be displayed blank. I'd like to hear what
> the
> > > > author or reviewers of that CL think. Otherwise, I guess this patch
could
> be
> > > > okay.
> > > 
> > > I'm not sure I love the idea of delaying opening the menu, and I
personally
> > > think the lack of icon looked weird enough in the original bug
> > > (crbug.com/319619) that it warranted a fix.  But, that's just my two
cents.
> :)
> > 
> > 
> > Then what about instead just showing a default icon in that rare edge case?
> > (newly installed extension, browser action pushed to overflow menu, quickly
> > invoking the overflow menu before we've loaded the icon)
> 
> Or, maybe omit entries from the overflow until their icons are loaded?

Experimenting with this, it actually looks like the problem isn't as much of an
edge-case as we thought.  On a fresh run of chrome (with browser actions already
collapsed), it looks like _none_ of the extensions' icons show up on the first
open of the overflow menu if we take out the IconUpdater. [1]  As such, omitting
or using the default icon really isn't a good enough solution. :/

[1] http://imgur.com/GIrZsvY

[msw, 2014-07-09 22:46:53]

okay, lgtm I guess.

[Devlin, 2014-07-09 23:04:25]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2014-07-09 23:05:48]

On 2014/07/09 22:46:53, msw wrote:
> okay, lgtm I guess.

Thanks :)
If we can think of a better way to implement this, I'm gonna be hanging around
the toolbar for awhile, so I'd be happy to add it to my list.  Feel free to
assign a bug to me if you can think of a better workaround. :)

[commit-bot: I haz the power, 2014-07-09 23:06:10]

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

[commit-bot: I haz the power, 2014-07-10 02:59:36]

Change committed as 282224