[Mac] Do not pump nested tasks in -[NSApp sendEvent:].

[Closing in favor of http://codereview.chromium.org/362013 ]

[Mac] Do not pump nested tasks in -[NSApp sendEvent:].

Renderers can cause UI state changes which can badly break modal event loops.  For instance, if a tab closes while the user is selecting a context-menu item relating to that tab, the app can crash.  When the tab-content right-click menu is up, nested tasks are not pumped.  This change causes modal event loops in other areas of the UI to work the same way.

The downside of this change is that Chrome events are no longer pumped in modal event loops.  This means that if a context menu is up, animations and other updates will not happen, in any tab in any window.  This is sad, but not as sad as sometimes crashing all tabs in all windows.  This includes plug-ins, so video will pause while in a context menu (this was already true of tab content context menus).

Examples of problem cases:

 - Last tab in a window closes while dragging the window.
 - Last tab in a window closes while bookmark bar context menu visible.
 - Last tab in a window closes while download shelf context menu visible.
 - Tab closes while dragging a link over the tab.
 - Tab closes while dragging a link from the tab.
 - Tab closes while back/forward context menu is open.
 - Tab closes while click-hold in the tab's close button.
 - Tab closes while closing info bar.
 - Tab closes while tab context menu is visible.
 - Probably more I'm not aware of.

Many of these cases also have poor interactions with alert() panels breaking into the modal loop.  For instance, if you were in a context menu and an alert() pops up on a different window.

This CL removes previous fix for issues 25462 and 25465.

BUG=25463, 25462, 25465, 25556, 26135, 26136, 25467, 25463
TEST=See bugs for repro cases.

[Scott Hess - ex-Googler, 2009-11-02 21:24:41]

Feel free to rope anyone else into this that seems worth involving.

How does Windows handle these?  So far, it's a grab-bag.  The tab context-menu
cancels with the tab.  The history context menu pins the tab until dismissed. 
The bookmark popup menu crashes when the last tab closes out from under it. 
Dragging a closing tab breaks the close so that the tab stays alive.

http://codereview.chromium.org/348047/diff/1/2
File chrome/browser/chrome_application_mac.mm (right):

http://codereview.chromium.org/348047/diff/1/2#newcode276
Line 276: ![keyWindow shortcircuitEvent:event]) {
This looks even nicer if I define -shortcircuitEvent: in an NSWindow @category.

[Mark Mentovai, 2009-11-02 21:30:59]

LGTM.

Can you enumerate some of the things that we're losing by doing this in the CL
description too?

http://codereview.chromium.org/348047/diff/1/2
File chrome/browser/chrome_application_mac.mm (right):

http://codereview.chromium.org/348047/diff/1/2#newcode264
Line 264: bool was_enabled = MessageLoop::current()->NestableTasksAllowed();
Use wasEnabled because you're in an Objective-C method.

http://codereview.chromium.org/348047/diff/1/2#newcode271
Line 271: // sending them to the menu, and we never want them to the kev
Who's Kev?

[Scott Hess - ex-Googler, 2009-11-02 21:33:29]

Additionally, want to emphasize the alert() aspect of this.  I have a collection
of CLs which handle context-menu disabling on a piecemeal basis, which worked
fine for close().  But late in the week I realized that alert() had much the
same issues, except worse in some ways.  For instance, you could end up having
alert() rip you to a different tab in the middle of navigating your context
menu, so now you aren't even looking at the same stuff.

[Scott Hess - ex-Googler, 2009-11-02 21:45:28]

CL message updated, let me know if it needs more love.

http://codereview.chromium.org/348047/diff/1/2
File chrome/browser/chrome_application_mac.mm (right):

http://codereview.chromium.org/348047/diff/1/2#newcode264
Line 264: bool was_enabled = MessageLoop::current()->NestableTasksAllowed();
On 2009/11/02 21:30:59, Mark Mentovai wrote:
> Use wasEnabled because you're in an Objective-C method.

Done.

I had originally written a ScopedNestableTasksEnabler helper, because it was
cool, and because I figured that it would be useful for the inevitable cases
where someone wants to invert this.  But I couldn't figure out a way to make
that work with @try...@finally, and that seemed really really important, here (I
seem to remember that we've had a variety of fun jank down to the nestable tasks
setting getting turned off forever).

http://codereview.chromium.org/348047/diff/1/2#newcode271
Line 271: // sending them to the menu, and we never want them to the kev
On 2009/11/02 21:30:59, Mark Mentovai wrote:
> Who's Kev?

I have no idea, wasn't my comment.  I assume it means "key view loop", will
change.

[Mark Mentovai, 2009-11-02 22:06:14]

dmac might want a look too, but LGTM.

http://codereview.chromium.org/348047/diff/5001/5002
File chrome/browser/chrome_application_mac.mm (left):

http://codereview.chromium.org/348047/diff/5001/5002#oldcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
In description, model->modal

http://codereview.chromium.org/348047/diff/5001/5002
File chrome/browser/chrome_application_mac.mm (right):

http://codereview.chromium.org/348047/diff/5001/5002#newcode264
Line 264: bool wasEnabled = MessageLoop::current()->NestableTasksAllowed();
I agree about the stack setter.  I was gonna suggest that too, but then I saw
the @finally and said "forget it."

http://codereview.chromium.org/348047/diff/5001/5002#newcode271
Line 271: // sending them to the menu, and we never want them to the key
to go to the key view loop, I guess.

Sorry to harp on not-yours.  I thought you'd see that too when I asked about
Kev.

[Scott Hess - ex-Googler, 2009-11-02 22:28:14]

I have to keep catching model.  Grrr.  Someone up there is
pattern-matching too much.

Something I just added to the description - this will pause things
like video.  It doesn't pause audio, because that goes via different
channels (haha).  Does not pause video on Windows Chrome - Windows
also seems to pump during the right-click tab-contents menu.  Mac
Firefox manages to keep video going when in a context menu, too.

I don't know, this seems kinda killer for this CL, to me.  I can cause
some of the alert() jank in Windows, so maybe it's reasonable to go
back to addressing these cases piecemeal, and ignore the alert()
variants for now.

Probably the right long-term fix is to enumerate a set of messages
which will not be processed until we next reach the main event loop.
The renderer presumably cannot expect continued progress while waiting
for the results of an alert() anyhow.

-scott


On Mon, Nov 2, 2009 at 3:06 PM,  <mark@chromium.org> wrote:
> dmac might want a look too, but LGTM.
>
>
> http://codereview.chromium.org/348047/diff/5001/5002
> File chrome/browser/chrome_application_mac.mm (left):
>
> http://codereview.chromium.org/348047/diff/5001/5002#oldcode1
> Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
> In description, model->modal
>
> http://codereview.chromium.org/348047/diff/5001/5002
> File chrome/browser/chrome_application_mac.mm (right):
>
> http://codereview.chromium.org/348047/diff/5001/5002#newcode264
> Line 264: bool wasEnabled =3D
> MessageLoop::current()->NestableTasksAllowed();
> I agree about the stack setter. =A0I was gonna suggest that too, but then
> I saw the @finally and said "forget it."
>
> http://codereview.chromium.org/348047/diff/5001/5002#newcode271
> Line 271: // sending them to the menu, and we never want them to the key
> to go to the key view loop, I guess.
>
> Sorry to harp on not-yours. =A0I thought you'd see that too when I asked
> about Kev.
>
> http://codereview.chromium.org/348047
>

[dmac, 2009-11-02 22:29:05]

http://codereview.chromium.org/348047/diff/6/8
File chrome/browser/cocoa/tab_controller.mm (left):

http://codereview.chromium.org/348047/diff/6/8#oldcode55
Line 55: [contextMenu_ setAutoenablesItems:NO];
does it make sense just to call [contextMenu_ cancelTracking] instead of leaving
the menu open?

[dmac, 2009-11-02 22:30:49]

On 2009/11/02 22:28:14, shess wrote:
> I have to keep catching model.  Grrr.  Someone up there is
> pattern-matching too much.
> 
> Something I just added to the description - this will pause things
> like video.  It doesn't pause audio, because that goes via different
> channels (haha).  Does not pause video on Windows Chrome - Windows
> also seems to pump during the right-click tab-contents menu.  Mac
> Firefox manages to keep video going when in a context menu, too.
> 
> I don't know, this seems kinda killer for this CL, to me.  I can cause
> some of the alert() jank in Windows, so maybe it's reasonable to go
> back to addressing these cases piecemeal, and ignore the alert()
> variants for now.
> 
> Probably the right long-term fix is to enumerate a set of messages
> which will not be processed until we next reach the main event loop.
> The renderer presumably cannot expect continued progress while waiting
> for the results of an alert() anyhow.
> 

I think http://code.google.com/p/chromium/issues/detail?id=26519 would probably
solve most of these issues wouldn't it?

[Scott Hess - ex-Googler, 2009-11-02 22:35:15]

On 2009/11/02 22:30:49, dmac wrote:
> On 2009/11/02 22:28:14, shess wrote:
> > I have to keep catching model.  Grrr.  Someone up there is
> > pattern-matching too much.
> > 
> > Something I just added to the description - this will pause things
> > like video.  It doesn't pause audio, because that goes via different
> > channels (haha).  Does not pause video on Windows Chrome - Windows
> > also seems to pump during the right-click tab-contents menu.  Mac
> > Firefox manages to keep video going when in a context menu, too.
> > 
> > I don't know, this seems kinda killer for this CL, to me.  I can cause
> > some of the alert() jank in Windows, so maybe it's reasonable to go
> > back to addressing these cases piecemeal, and ignore the alert()
> > variants for now.
> > 
> > Probably the right long-term fix is to enumerate a set of messages
> > which will not be processed until we next reach the main event loop.
> > The renderer presumably cannot expect continued progress while waiting
> > for the results of an alert() anyhow.
> > 
> 
> I think http://code.google.com/p/chromium/issues/detail?id=26519 would
probably
> solve most of these issues wouldn't it?

Sure would, but that sounds like kind of a substantial change.  This CL is
intentially kind of a bandaid.

http://codereview.chromium.org/348047/diff/6/8
File chrome/browser/cocoa/tab_controller.mm (left):

http://codereview.chromium.org/348047/diff/6/8#oldcode55
Line 55: [contextMenu_ setAutoenablesItems:NO];
On 2009/11/02 22:29:05, dmac wrote:
> does it make sense just to call [contextMenu_ cancelTracking] instead of
leaving
> the menu open?

I had variable success with -cancelTracking.  I was finding cases where it
didn't seem to dismiss the menu.  I do not know why.