[Mac] Prevent using tab context menu after tab closed.

[Mac] Prevent using tab context menu after tab closed.

JavaScript can close windows while in the event loop processing a
right-click menu.  This change prevents sending messages to the tab
controller after it has been closed.

BUG=25462, 25465
TEST=See bug for test.html.
TEST=Run test.html, right-click tab, after close all items should be grayed out.
TEST=Run test.html, click tab's close button and hold until tab closes.  Should not crash on release.

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

[Scott Hess - ex-Googler, 2009-10-27 16:42:07]

I don't think this is the "right" fix, but I believe it addresses a lot of the
crashers in our biggest bin of browser crashs.

Windows cancels the menu when the tab closes.  I have code to do that, but it
felt like a poor experience to me.

I think the right fix is to do something like not pump certain IPC messages from
the renderer in certain cases.  For instance, animations seem reasonable while
the context menu is up, but things like navigation and closing might not be
because they change the meaning of the popped-up menu.

[pink (ping after 24hrs), 2009-10-27 16:59:23]

http://codereview.chromium.org/335048/diff/1/2
File chrome/browser/cocoa/tab_controller.mm (right):

http://codereview.chromium.org/335048/diff/1/2#newcode59
Line 59: for (NSMenuItem* item in [contextMenu_ itemArray]) {
how does windows handle this case? Is there a general pattern we should copy?
I'm sure it had to solve a similar type of problem.

[Scott Hess - ex-Googler, 2009-10-27 17:24:15]

http://codereview.chromium.org/335048/diff/1/2
File chrome/browser/cocoa/tab_controller.mm (right):

http://codereview.chromium.org/335048/diff/1/2#newcode59
Line 59: for (NSMenuItem* item in [contextMenu_ itemArray]) {
On 2009/10/27 16:59:23, pink wrote:
> how does windows handle this case? Is there a general pattern we should copy?
> I'm sure it had to solve a similar type of problem.

Windows dismisses the pop-up when the tab goes away.  This feels a bit jarring
to me.

Trung came over and complained about "Man, there should be a better way.  Or
something", and we poked at Windows.  I think that looking to Windows for the
uber-solution may not help us.  The context menu from the tab goes away when the
tab goes away, the pulldown from the back button stays up, but everything
unwinds when you select something (the tab goes away and your selection is
ignored or something).  On Windows the content context menu goes away when the
tab goes away, while on Mac it pins the tab until you dismiss the menu, so maybe
there's some argument for "Dismiss things when the tab goes away."

We could possibly wire up a generic way to dismiss right-click menus when the
visible tab on the window goes away.  But that doesn't handle right-click on a
tab which isn't visible :-).

[I suspect this is just going to be a long slog.]

[viettrungluu, 2009-10-27 17:27:11]

Of course I'd like a general solution, but I'd like to see how this affects the
crashes in the next dev channel.

So LG with the provisos that this is a temporary (tentative) solution to some of
our crashes and that the bug is left open.

http://codereview.chromium.org/335048/diff/1/2
File chrome/browser/cocoa/tab_controller.mm (right):

http://codereview.chromium.org/335048/diff/1/2#newcode52
Line 52: // is a problem for menu items with this object at target.
"object at target" -> "object as target"?

http://codereview.chromium.org/335048/diff/1/2#newcode59
Line 59: for (NSMenuItem* item in [contextMenu_ itemArray]) {
On 2009/10/27 16:59:23, pink wrote:
> how does windows handle this case? Is there a general pattern we should copy?
> I'm sure it had to solve a similar type of problem.

I suspect that this sort of problem is handled case-by-case (there's still a
crasher along these lines in WinChrome the last I looked -- crbug.com/25563).
IIRC, the exhibited behaviour also varies a bit.

[Scott Hess - ex-Googler, 2009-10-27 17:31:44]

BTW, AFAICT there are sort of two ways this kind of thing happens.  One is a
case like this where we leave a stale reference hanging around for an open-ended
period.  This case is handled by dismissing or disabling things.

But there also seems to be a sort of race-condition case, where disabling
doesn't work.  My suspicion is that the close is pumped in the call which
returns the event which initiates the action, and the enabled/disabled status of
the control has already been checked so the message is forwarded.  That's why my
code nils the target.  Unfortunately, I'm finding this case really hard to
provoke reliably.

[viettrungluu, 2009-10-27 17:34:18]

So the case I was thinking of (of different behaviour) was dragging a tab that
will disappear on Windows: if you release the tab before it disappears, it'll
disappear on schedule; if you hold onto it long enough, the closure is
cancelled. I'm not sure what mechanism is being used there....

[Mark Mentovai, 2009-10-27 17:38:05]

http://codereview.chromium.org/335048/diff/1/2
File chrome/browser/cocoa/tab_controller.mm (right):

http://codereview.chromium.org/335048/diff/1/2#newcode59
Line 59: for (NSMenuItem* item in [contextMenu_ itemArray]) {
shess wrote:
> Windows dismisses the pop-up when the tab goes away.  This feels a bit jarring
> to me.

Dunno, it sounds OK to me.  JS closing a tab can be jarring no matter what, but
if the context menu belongs to the tab...

How do other browsers handle it?

[viettrungluu, 2009-10-27 17:56:35]

http://codereview.chromium.org/335048/diff/1/2
File chrome/browser/cocoa/tab_controller.mm (right):

http://codereview.chromium.org/335048/diff/1/2#newcode59
Line 59: for (NSMenuItem* item in [contextMenu_ itemArray]) {
On 2009/10/27 17:38:05, Mark Mentovai wrote:
> Dunno, it sounds OK to me.  JS closing a tab can be jarring no matter what,
but
> if the context menu belongs to the tab...
> 
> How do other browsers handle it?

On Mac:

Safari blocks tab closure until the menu is dismissed (at which point the tab is
closed). In fact, it seems to block tab closure whenever a nested message loop
is running. (What happens afterwards varies a bit. For example, New Tab from the
tab context menu works, but navigation (e.g., via bookmarks or history) fails,
presumably because the closure happens before the navigation.

Opera seems to behave similarly.

test.html doesn't seem to work on Firefox and Camino.

[Scott Hess - ex-Googler, 2009-10-27 18:01:03]

Aha!  I found another repro which seems more likely to explain a lot
of our crashers - click-hold the tab's close button until the tab goes
away, then release.  The close button is wired directly to the tab
controller.  I'll see if I can roll that fix into this CL.

-scott


On Tue, Oct 27, 2009 at 10:31 AM,  <shess@chromium.org> wrote:
> BTW, AFAICT there are sort of two ways this kind of thing happens. =A0One=
 is a
> case like this where we leave a stale reference hanging around for an
> open-ended
> period. =A0This case is handled by dismissing or disabling things.
>
> But there also seems to be a sort of race-condition case, where disabling
> doesn't work. =A0My suspicion is that the close is pumped in the call whi=
ch
> returns the event which initiates the action, and the enabled/disabled
> status of
> the control has already been checked so the message is forwarded. =A0That=
's
> why my
> code nils the target. =A0Unfortunately, I'm finding this case really hard=
 to
> provoke reliably.
>
> http://codereview.chromium.org/335048
>

[Scott Hess - ex-Googler, 2009-10-27 18:03:56]

On Tue, Oct 27, 2009 at 10:56 AM,  <viettrungluu@chromium.org> wrote:
> test.html doesn't seem to work on Firefox and Camino.

Awhile back, I found a rash of crashers relating to popup windows and
action-after-closed.  test.html is specially crafted to let me close a
regular page, so I wouldn't expect it to necessarily work on Firefox.

> http://codereview.chromium.org/335048

[Scott Hess - ex-Googler, 2009-10-27 18:13:44]

On 2009/10/27 18:01:03, shess wrote:
> Aha!  I found another repro which seems more likely to explain a lot
> of our crashers - click-hold the tab's close button until the tab goes
> away, then release.  The close button is wired directly to the tab
> controller.  I'll see if I can roll that fix into this CL.

Now handles closeButton_ also.

[Scott Hess - ex-Googler, 2009-10-27 19:27:35]

On 2009/10/27 18:13:44, shess wrote:
> On 2009/10/27 18:01:03, shess wrote:
> > Aha!  I found another repro which seems more likely to explain a lot
> > of our crashers - click-hold the tab's close button until the tab goes
> > away, then release.  The close button is wired directly to the tab
> > controller.  I'll see if I can roll that fix into this CL.
> 
> Now handles closeButton_ also.

Q for the peanut gallery: This is a safe thing to do, right?  AFAICT, the view
which this is NSViewController for has the objects I'm messing with as
sub-views, so they should still all be live when I reach -dealloc.  I had a
couple minutes of doubt before I convinced myself of that.

[pink (ping after 24hrs), 2009-10-27 22:03:50]

lgtm

i still think we should add a TODO that this is a temporary spot-fix and that
other context menus and closeboxes in the browser window should be audited for a
more general solution.

[Scott Hess - ex-Googler, 2009-10-28 16:11:25]

http://crbug.com/25968 : Controls can outlive their targets when
JavaScript closes tabs.

I'll list the cases I've got a repro for there.


On Tue, Oct 27, 2009 at 3:03 PM,  <pinkerton@chromium.org> wrote:
> lgtm
>
> i still think we should add a TODO that this is a temporary spot-fix and
> that
> other context menus and closeboxes in the browser window should be audited
> for a
> more general solution.
>
> http://codereview.chromium.org/335048
>