Make sure RenderWidgetHostViewCocoa doesn't disappear inside of -keyEvent:

Make sure RenderWidgetHostViewCocoa doesn't disappear inside of -keyEvent:

BUG=25857
TEST=see bug 25857 comment 10

[Mark Mentovai, 2009-10-28 16:46:22]

Take a look at the bug too.  I'm not terribly familiar with the ownership model
in here, and I don't know if this is ultimately the best way to go.

[Nico, 2009-10-28 16:59:27]

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

http://codereview.chromium.org/341022/diff/1/2#newcode586
Line 586: widgetHost->ForwardKeyboardEvent(event);
After r30032 , we can be destroyed after this line. Both windows and linux don't
do anything after this call, so they should be fine.

Wouldn't it be better to set some flag in `dealloc` and then return after this
lien? That way interpretKeyEvents: is not called (it _should_ be save, because
cmd-w etc shouldn't trigger an IME key anyway, but it feels more explicit)

[Mark Mentovai, 2009-10-28 17:02:02]

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

http://codereview.chromium.org/341022/diff/1/2#newcode586
Line 586: widgetHost->ForwardKeyboardEvent(event);
Nico wrote:
> After r30032 , we can be destroyed after this line. Both windows and linux
don't
> do anything after this call, so they should be fine.
> 
> Wouldn't it be better to set some flag in `dealloc` and then return after this
> lien? That way interpretKeyEvents: is not called (it _should_ be save, because
> cmd-w etc shouldn't trigger an IME key anyway, but it feels more explicit)

How do you propose we do this?  Set a member variable flag in this object as
we're destroying it?  Ain't gonna work.

[Nico, 2009-10-28 17:03:50]

Give this object a pointer that we make point to a variable on the
stack at the beginning of keyEvent:. During dealloc, the pointer would
still be valid, and back in the method, the stack variable is still
valid.

On Wed, Oct 28, 2009 at 10:02 AM,  <mark@chromium.org> wrote:
>
> http://codereview.chromium.org/341022/diff/1/2
> File chrome/browser/renderer_host/render_widget_host_view_mac.mm
> (right):
>
> http://codereview.chromium.org/341022/diff/1/2#newcode586
> Line 586: widgetHost->ForwardKeyboardEvent(event);
> Nico wrote:
>>
>> After r30032 , we can be destroyed after this line. Both windows and
>
> linux don't
>>
>> do anything after this call, so they should be fine.
>
>> Wouldn't it be better to set some flag in `dealloc` and then return
>
> after this
>>
>> lien? That way interpretKeyEvents: is not called (it _should_ be save,
>
> because
>>
>> cmd-w etc shouldn't trigger an IME key anyway, but it feels more
>
> explicit)
>
> How do you propose we do this? =A0Set a member variable flag in this
> object as we're destroying it? =A0Ain't gonna work.
>
> http://codereview.chromium.org/341022
>

[dmac, 2009-10-28 17:08:07]

On 2009/10/28 17:03:50, Nico wrote:
> Give this object a pointer that we make point to a variable on the
> stack at the beginning of keyEvent:. During dealloc, the pointer would
> still be valid, and back in the method, the stack variable is still
> valid.
> 
> On Wed, Oct 28, 2009 at 10:02 AM,  <mailto:mark@chromium.org> wrote:
> >
> > http://codereview.chromium.org/341022/diff/1/2
> > File chrome/browser/renderer_host/render_widget_host_view_mac.mm
> > (right):
> >
> > http://codereview.chromium.org/341022/diff/1/2#newcode586
> > Line 586: widgetHost->ForwardKeyboardEvent(event);
> > Nico wrote:
> >>
> >> After r30032 , we can be destroyed after this line. Both windows and
> >
> > linux don't
> >>
> >> do anything after this call, so they should be fine.
> >
> >> Wouldn't it be better to set some flag in `dealloc` and then return
> >
> > after this
> >>
> >> lien? That way interpretKeyEvents: is not called (it _should_ be save,
> >
> > because
> >>
> >> cmd-w etc shouldn't trigger an IME key anyway, but it feels more
> >
> > explicit)
> >
> > How do you propose we do this? =A0Set a member variable flag in this
> > object as we're destroying it? =A0Ain't gonna work.
> >
> > http://codereview.chromium.org/341022
> >

Mark, this is what I wanted to talk to you about runloops for. I think we can
avoid all of this (and the somewhat equivalent hack in the browser window) if we
got rid of the autorelease pool in our CFRunLoopSource.... but I needed more
data on why that was there. I assume we can cover this when we chat.

[Mark Mentovai, 2009-10-28 18:01:50]

Nico wrote:
> Give this object a pointer that we make point to a variable on the
> stack at the beginning of keyEvent:. During dealloc, the pointer would
> still be valid, and back in the method, the stack variable is still
> valid.

I see what you're driving at.  I think that the stack boolean thing is a little
hokey, but I've come up with an alternative.  Do you like this better?

[Nico, 2009-10-28 18:04:50]

I do. LGTM, but please add a comment that mentions somewhere that the current
tab / browser might be gone after |widgetHost->ForwardKeyboardEvent(event);|

[dmac, 2009-10-28 18:10:11]

On 2009/10/28 18:04:50, Nico wrote:
> I do. LGTM, but please add a comment that mentions somewhere that the current
> tab / browser might be gone after |widgetHost->ForwardKeyboardEvent(event);|

Mark... can you still hold off on this until our meeting? I think I may have a
better solution, and I'm afraid you are putting a bandage on a bigger problem.
Maybe add Nico to the meeting if he's interested?

[Mark Mentovai, 2009-10-28 18:13:16]

Right, I wasn't planning on checking this in until we spoke.

Mark

dmaclach@chromium.org wrote:
> On 2009/10/28 18:04:50, Nico wrote:
>>
>> I do. LGTM, but please add a comment that mentions somewhere that the
>> current
>> tab / browser might be gone after
>> |widgetHost->ForwardKeyboardEvent(event);|
>
> Mark... can you still hold off on this until our meeting? I think I may have
> a
> better solution, and I'm afraid you are putting a bandage on a bigger
> problem.
> Maybe add Nico to the meeting if he's interested?
>
>
> http://codereview.chromium.org/341022
>

[pink (ping after 24hrs), 2009-10-28 19:56:40]

drive-by.

After the meeting, what have we decided?

http://codereview.chromium.org/341022/diff/4003/4004
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/341022/diff/4003/4004#newcode557
Line 557: scoped_nsobject<RenderWidgetHostViewCocoa> keepSelfAlive([self
retain]);
in mozilla these were generally named kungFuDeathGrip with nsCOMPtr :=)

[Mark Mentovai, 2009-10-29 01:56:23]

I'm closing this without checking it in.  It shouldn't be needed once we get
http://codereview.chromium.org/343024.