Fix Mac crash when page goes away while a pop-up menu is active.

Fix Mac crash when page goes away while a pop-up menu is active.
This may also fix the older related bug 31716.
BUG=33250
TEST=see steps to reproduce in the bug report

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

[Jens Alfke, 2010-01-28 18:50:05]

I believe this should also fix 31716, unless there's some other very different
scenario that leads to the same backtrace.

[viettrungluu, 2010-01-28 19:00:26]

On 2010/01/28 18:50:05, Jens Alfke wrote:
> I believe this should also fix 31716, unless there's some other very different
> scenario that leads to the same backtrace.

LGTM. Could you file a bug about dismissing the menu (or somesuch) and put a
TODO in a comment somewhere in here?

[Scott Hess - ex-Googler, 2010-01-28 19:02:13]

NOT LGTM!

http://codereview.chromium.org/558021/diff/1/2
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/558021/diff/1/2#newcode402
chrome/browser/renderer_host/render_widget_host_view_mac.mm:402: ([cocoa_view_
retain]);
None of the code in question references cocoa_view_, I think you would want to
retain parent_view_.

http://codereview.chromium.org/558021/diff/1/2#newcode423
chrome/browser/renderer_host/render_widget_host_view_mac.mm:423: if
(!render_widget_host_) {
It is not safe to access any member variables at this point.  The instance could
have been reallocated.  As I mentioned on the bug, I think you should use a
scoped method factory, which is designed for exactly this type of thing.

[Scott Hess - ex-Googler, 2010-01-28 19:02:55]

On 2010/01/28 19:00:26, viettrungluu wrote:
> On 2010/01/28 18:50:05, Jens Alfke wrote:
> > I believe this should also fix 31716, unless there's some other very
different
> > scenario that leads to the same backtrace.
> 
> LGTM. Could you file a bug about dismissing the menu (or somesuch) and put a
> TODO in a comment somewhere in here?

Cocoa menus cannot be reliably dismissed.  No, really :-).

-scott

[viettrungluu, 2010-01-28 19:04:54]

LGTM withdrawn, I guess.

On 2010/01/28 19:02:55, shess wrote:
> On 2010/01/28 19:00:26, viettrungluu wrote:
> > On 2010/01/28 18:50:05, Jens Alfke wrote:
> > > I believe this should also fix 31716, unless there's some other very
> different
> > > scenario that leads to the same backtrace.
> > 
> > LGTM. Could you file a bug about dismissing the menu (or somesuch) and put a
> > TODO in a comment somewhere in here?
> 
> Cocoa menus cannot be reliably dismissed.  No, really :-).

I thought that we successfully dismiss context menus on navigation, or at least
it looks that way ... or am I missing something?

[Scott Hess - ex-Googler, 2010-01-28 19:09:27]

On Thu, Jan 28, 2010 at 11:04 AM,  <viettrungluu@chromium.org> wrote:
>> Cocoa menus cannot be reliably dismissed.  No, really :-).
>
> I thought that we successfully dismiss context menus on navigation, or at
> least
> it looks that way ... or am I missing something?

Could you describe the case you're thinking of more fully?  Menus drop
down when you select an item or click somewhere else.  You can also
call -[NSMenu cancelTracking], but it only works in certain cases,
which I don't recall, but I think it was something like it no longer
works once the user starts interacting with the menu or something like
that.  [The only ref to -cancelTracking in cocoa/ is a comment by you
in the DelayedMenuButtonTest.MenuAssign saying that -cancelTracking
doesn't get rid of the menu, and you don't know why :-).]

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

[Jens Alfke, 2010-01-28 19:10:53]

On 2010/01/28 19:02:13, shess wrote:
> None of the code in question references cocoa_view_, I think you would want to
retain parent_view_.

As the comment says, it's the dealloc of cocoa_view_ that triggers deletion of
'this'. And that dealloc happens because of the autorelease in the Destroy()
method. Retaining cocoa_view_ defers this. The cocoa_view_ must be the sole
owner of the RWHVM object because its renderWidgetHostView_ ivar never gets
reset to nil anywhere, so it will always delete its RWHVM on dealloc.

> It is not safe to access any member variables at this point.  The instance
could have been reallocated.

No, because its owning NSView is still alive thanks to having been retained.

[viettrungluu, 2010-01-28 19:21:13]

On 2010/01/28 19:09:27, shess wrote:
> On Thu, Jan 28, 2010 at 11:04 AM,  <mailto:viettrungluu@chromium.org> wrote:
> >> Cocoa menus cannot be reliably dismissed. &nbsp;No, really :-).
> >
> > I thought that we successfully dismiss context menus on navigation, or at
> > least
> > it looks that way ... or am I missing something?
> 
> Could you describe the case you're thinking of more fully?  Menus drop
> down when you select an item or click somewhere else.  You can also
> call -[NSMenu cancelTracking], but it only works in certain cases,
> which I don't recall, but I think it was something like it no longer
> works once the user starts interacting with the menu or something like
> that.  [The only ref to -cancelTracking in cocoa/ is a comment by you
> in the DelayedMenuButtonTest.MenuAssign saying that -cancelTracking
> doesn't get rid of the menu, and you don't know why :-).]

D'oh. I thought it dismissed context menus, but I appear to be wrong. :-(

[Scott Hess - ex-Googler, 2010-01-28 19:28:42]

OK.  I think I see where I went wrong - Destroy() is reflecting
through the cocoa code to the destructor, but clearing
render_widget_host_.  Brrr.

I think now I'm LGTM.

You'll have a merge with my fix, which is now checked-in.  I _think_
that both sets of code should be left in, because having the tab go
away (and thus possibly the window) while in the popup is jarring.  If
your change would hold the tab up until after the popup runs, then get
back to me?

-scott


On Thu, Jan 28, 2010 at 11:10 AM,  <snej@chromium.org> wrote:
> On 2010/01/28 19:02:13, shess wrote:
>>
>> None of the code in question references cocoa_view_, I think you would
>> want to
>
> retain parent_view_.
>
> As the comment says, it's the dealloc of cocoa_view_ that triggers deletion
> of
> 'this'. And that dealloc happens because of the autorelease in the Destroy()
> method. Retaining cocoa_view_ defers this. The cocoa_view_ must be the sole
> owner of the RWHVM object because its renderWidgetHostView_ ivar never gets
> reset to nil anywhere, so it will always delete its RWHVM on dealloc.
>
>> It is not safe to access any member variables at this point.  The instance
>
> could have been reallocated.
>
> No, because its owning NSView is still alive thanks to having been retained.
>
> http://codereview.chromium.org/558021
>

[viettrungluu, 2010-01-28 20:25:26]

I'll also re-instate my LGTM then. :-)

[Scott Hess - ex-Googler, 2010-01-28 20:37:47]

BTW, it does occur to me that this might be fixing a symptom, rather
than a more general issue.

I think the problem is that when I added ScopedNestableTaskAllower, it
enabled desirable things like video updates, but also enabled other
things like close and navigation to be pushed.  It's kind of
surprising that these can be pushed during a synchronous operation, I
think - not just at the browser level, but even down in the renderer.
I don't mean that there's a real bug, it just feels wrong.  I still
don't have any light to shed on this, but it is something to keep in
mind.

-scott


On Thu, Jan 28, 2010 at 12:25 PM,  <viettrungluu@chromium.org> wrote:
> I'll also re-instate my LGTM then. :-)
>
> http://codereview.chromium.org/558021
>

[Jens Alfke, 2010-01-28 21:05:11]

On 2010/01/28 19:28:42, shess wrote:
>  If your change would hold the tab up until after the popup runs, then get
> back to me?

No, my change doesn't affect the tab, only the pop-up. I'll merge, test and
commit, then. Thanks.