Views: fix a crash where in the browser actions container....

Views: fix a crash where in the browser actions container.

This crash didn't actually affect linux/views (for some reason the RunContextMenu() call seems to never return).

BUG=38964
TEST=crash an extension while the context menu for it is showing.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=42703

[Evan Stade, 2010-03-25 01:31:16]

Do you guys know how I can make a change to native_menu_win that is equivalent
to the change I made to native_menu_gtk?

[Evan Stade, 2010-03-25 01:35:03]

btw, here is some background on the windows crash:

"If the button is destroyed in the nested message loop created by
RunContextMenuAt(), then SetButtonNotPushed() will cause an access violation."

[dmazzoni, 2010-03-25 02:22:46]

If I understand correctly, the user right-clicks on a browser action button, and
then while the context menu is open, the browser action button itself is
destroyed.

I suppose something similar could be done on Windows, but did you look into
whether this could be fixed by deferring deleting the button while the menu is
open? Actually I think it makes sense to just close the menu, because if the
button is gone, then none of the menu items make sense anyway, right?

What if the call to delete the browser action button first closed any open
pop-up menus and then posted a task to delete the button (so it'd run in the
main loop, rather than the nested one)?

[Evan Stade, 2010-03-25 02:33:13]

On 2010/03/25 02:22:46, Dominic Mazzoni wrote:
> If I understand correctly, the user right-clicks on a browser action button,
and
> then while the context menu is open, the browser action button itself is
> destroyed.
> 
> I suppose something similar could be done on Windows, but did you look into
> whether this could be fixed by deferring deleting the button while the menu is
> open?
> Actually I think it makes sense to just close the menu, because if the
> button is gone, then none of the menu items make sense anyway, right?
> 
> What if the call to delete the browser action button first closed any open
> pop-up menus and then posted a task to delete the button (so it'd run in the
> main loop, rather than the nested one)?

that is one possibility, and I've had to do something similar on gtk in a number
of places, but it seems less solid as a general approach because the crash is
likely to come up in the future for similar situations (and each time, won't be
noticed right away), whereas establishing the pattern of connecting to the
notification seems more futureproof. Assuming there is some easy place to stick
the "closing" callback, are there any drawbacks you are worried about with this
approach?

[dmazzoni, 2010-03-25 02:50:03]

I guess two things bother me:

One is that after context_menu_menu_->RunContextMenuAt(point) returns, |this|
could point to invalid memory. If someone were to add another innocent-looking
statement after RunContextMenuAt, it could reintroduce the bug. I think it's
better programming practice to never end up in a situation where |this| is
invalid.

Two is that I don't think the menu should remain open if the button disappears.
What happens if the user selects an item from the menu but the button is gone?

One solution would be to take care of all of these cases when the button is
destroyed - make it an error to just delete the button without calling
Destroy(), and have Destroy() handle the scenario of safely cleaning up if
necessary.

[sky, 2010-03-25 15:36:30]

I agree with Dominic here. Callers should close the menu if it no longer makes
sense to be open (especially if it would cause crashes).

http://codereview.chromium.org/1237004/diff/8001/9001
File views/controls/menu/menu_wrapper.h (right):

http://codereview.chromium.org/1237004/diff/8001/9001#newcode23
views/controls/menu/menu_wrapper.h:23: virtual void OnMenuOpened() {};
no ; here and on 26.

http://codereview.chromium.org/1237004/diff/8001/9003
File views/controls/menu/native_menu_gtk.h (right):

http://codereview.chromium.org/1237004/diff/8001/9003#newcode125
views/controls/menu/native_menu_gtk.h:125: // Vector of listeners to receive
callbacks when the menu opens.
update comment

[Evan Stade, 2010-03-25 18:56:54]

new patch uploaded

> One is that after context_menu_menu_->RunContextMenuAt(point) returns, |this|
> could point to invalid memory. If someone were to add another innocent-looking
> statement after RunContextMenuAt, it could reintroduce the bug. I think it's
> better programming practice to never end up in a situation where |this| is
> invalid.

I'm not particularly against this, it would be very easy. It just seems from my
experience on the linux side that if we rely on people realizing that this
border case exists, and using DeleteSoon to fix it, we will end up hitting this
problem again and again.

(Somewhere higher up in the OnMousePressed stack I remember seeing a comment
about avoiding touching member vars because |this| might be invalid.)

> 
> Two is that I don't think the menu should remain open if the button
disappears.
> What happens if the user selects an item from the menu but the button is gone?

It doesn't remain open. It is closed when the button is destroyed (i.e. *during*
the button's destructor). It's just that this function doesn't continue until
*after* the button's destructor.

> 
> One solution would be to take care of all of these cases when the button is
> destroyed - make it an error to just delete the button without calling
> Destroy(), and have Destroy() handle the scenario of safely cleaning up if
> necessary.

[sky, 2010-03-25 19:15:00]

LGTM with the following change.

http://codereview.chromium.org/1237004/diff/23001/24001
File chrome/browser/views/browser_actions_container.h (right):

http://codereview.chromium.org/1237004/diff/23001/24001#newcode103
chrome/browser/views/browser_actions_container.h:103: virtual
~BrowserActionButton();
Move implementation in .cc so that position matches that of header.

[dmazzoni, 2010-03-25 19:18:10]

LGTM

I'm assuming this fixes the problem on Windows too now?

Like you said, we'll probably keep hitting this problem again, but I think
that's true no matter how we solve it.

[Evan Stade, 2010-03-25 21:16:45]

yes, it works on windows. Please note change to native_menu_gtk to get it to
work on linux/views.