Fix crash in extension_popup_controller.mm appearing when devtools is closed

Fix crash in extension_popup_controller.mm appearing when devtools is closed
on an inspected popup.


BUG=196812
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193332

[Matt Perry, 2013-03-15 22:55:46]

lgtm

https://codereview.chromium.org/12572006/diff/1/chrome/browser/ui/cocoa/exten...
File chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm (right):

https://codereview.chromium.org/12572006/diff/1/chrome/browser/ui/cocoa/exten...
chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm:125:
RenderViewHost* rvh_;
nit: don't abbrev member vars

[not at google - send to devlin, 2013-03-15 23:49:53]

+thakis

https://codereview.chromium.org/12572006/diff/1/chrome/browser/ui/cocoa/exten...
File chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm (right):

https://codereview.chromium.org/12572006/diff/1/chrome/browser/ui/cocoa/exten...
chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm:125:
RenderViewHost* rvh_;
On 2013/03/15 22:55:46, Matt Perry wrote:
> nit: don't abbrev member vars

Done.

[Nico, 2013-03-15 23:50:53]

yoz touched this recently I think. yoz, does this make sense for you?

Is it possible to write a test for this?

[not at google - send to devlin, 2013-03-15 23:52:51]

yoz has been out for the day, and his comment about testing still applies I
imagine. It's a pretty small crasher though so probably ok to leave until the
dev channel build after this?

[Yoyo Zhou, 2013-03-18 18:45:07]

Should this have a TEST=?

[not at google - send to devlin, 2013-03-18 18:59:05]

On 2013/03/18 18:45:07, Yoyo Zhou wrote:
> Should this have a TEST=?

No, but only because I haven't reprod let alone actually run this, I don't have
a mac for development. Now that you're back it might be better if you take over
this CL, since you do have one.

[Yoyo Zhou, 2013-03-18 19:29:17]

LGTM. Seems reasonable, but I can't reproduce this.

[not at google - send to devlin, 2013-03-18 19:52:22]

thakis?

[Yoyo Zhou, 2013-03-18 20:20:53]

On 2013/03/18 19:29:17, Yoyo Zhou wrote:
> LGTM. Seems reasonable, but I can't reproduce this.

Actually, the change to -windowWillClose: reintroduces the crash in
crbug.com/161604.

[not at google - send to devlin, 2013-03-18 20:23:12]

> 
> Actually, the change to -windowWillClose: reintroduces the crash in
> crbug.com/161604.

Oops. Maybe I meant to move it back out of the branch again. See how this
goes...

[commit-bot: I haz the power, 2013-03-22 00:33:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/12572006/13001

[commit-bot: I haz the power, 2013-03-22 00:33:48]

Presubmit check for 12572006-13001 failed and returned exit status 1.

INFO:root:Found 1 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm

[Yoyo Zhou, 2013-03-29 01:25:36]

thakis: I was able to verify that this fixes the crash reported in
crbug.com/224544. (kalman is on vacation.) Could you review this for OWNERS?

[Nico, 2013-03-29 02:48:29]

yoz: I don't understand this change well. If the extension host can go
away, why is it safe to keep a pointer to its render_view_host?
(mpcomplete: Maybe you can comment, ExtensionHost has a TODO on you about
validity of the rvh)

yoz: Neither chrome/browser/ui/views/extensions/extension_popup.cc
nor chrome/browser/ui/gtk/extensions/extension_popup_gtk.cc seem to need
this. Why is it needed here?


On Thu, Mar 28, 2013 at 6:25 PM, <yoz@chromium.org> wrote:

> thakis: I was able to verify that this fixes the crash reported in
> crbug.com/224544. (kalman is on vacation.) Could you review this for
> OWNERS?
>
>
https://codereview.chromium.**org/12572006/<https://codereview.chromium.org/1...
>

[Yoyo Zhou, 2013-03-29 05:12:30]

On 2013/03/29 02:48:29, Nico wrote:
> yoz: I don't understand this change well. If the extension host can go
> away, why is it safe to keep a pointer to its render_view_host?
> (mpcomplete: Maybe you can comment, ExtensionHost has a TODO on you about
> validity of the rvh)

I think it's the ExtensionPopupController that might go away. This is related to
your next question...

> yoz: Neither chrome/browser/ui/views/extensions/extension_popup.cc
> nor chrome/browser/ui/gtk/extensions/extension_popup_gtk.cc seem to need
> this. Why is it needed here?

The GTK and Views implementations don't have these extra layer(s) of indirection
(the popup itself, which owns the ExtensionHost, listens to the devtools
notifications).

[Nico, 2013-03-29 05:23:27]

On 2013/03/29 05:12:30, Yoyo Zhou wrote:
> On 2013/03/29 02:48:29, Nico wrote:
> > yoz: I don't understand this change well. If the extension host can go
> > away, why is it safe to keep a pointer to its render_view_host?
> > (mpcomplete: Maybe you can comment, ExtensionHost has a TODO on you about
> > validity of the rvh)
> 
> I think it's the ExtensionPopupController that might go away. This is related
to
> your next question...
> 
> > yoz: Neither chrome/browser/ui/views/extensions/extension_popup.cc
> > nor chrome/browser/ui/gtk/extensions/extension_popup_gtk.cc seem to need
> > this. Why is it needed here?
> 
> The GTK and Views implementations don't have these extra layer(s) of
indirection
> (the popup itself, which owns the ExtensionHost, listens to the devtools
> notifications).

Right, but as long ownership arrows point in the same direction the extra layer
shouldn't matter, right? i.e. if it's popup --owns-> ExtensionHost in views and
gtk and it's popup --owns-> controller -->owns->ExtensionHost in cocoa, things
should be fine. Are the arrows the wrong way round?

In general, I think it's good to keep the code in the various uis as similar as
possible.

[Matt Perry, 2013-03-29 22:25:22]

I think the key here is that the render_view_host_ is not dereferenced. The
extension host (and its RVH) might have been deleted, so it's safer to keep a
pointer to the RVH since we don't need to dereference it.

[Nico, 2013-03-29 22:28:05]

On Fri, Mar 29, 2013 at 3:25 PM, <mpcomplete@chromium.org> wrote:

> I think the key here is that the render_view_host_ is not dereferenced. The
> extension host (and its RVH) might have been deleted, so it's safer to
> keep a
> pointer to the RVH since we don't need to dereference it.
>

That's pretty subtle though, and it's not needed on views/gtk.

But if you're happy with this patch, I'm happy with it too (with a "do not
dereference, might be invalid" comment at the member)


>
>
https://codereview.chromium.**org/12572006/<https://codereview.chromium.org/1...
>

[not at google - send to devlin, 2013-04-02 03:43:37]

> 
> That's pretty subtle though, and it's not needed on views/gtk.

I presume the complexity here is due to the Bridge classes that are necessary in
cocoa.

> 
> But if you're happy with this patch, I'm happy with it too (with a "do not
> dereference, might be invalid" comment at the member)

Such a comment would be misleading; the RVH outlives that bridge, but not
necessarily the controller that was holding onto it weakly.

[Nico, 2013-04-02 04:26:32]

On Mon, Apr 1, 2013 at 8:43 PM, <kalman@chromium.org> wrote:

>
>  That's pretty subtle though, and it's not needed on views/gtk.
>>
>
> I presume the complexity here is due to the Bridge classes that are
> necessary in
> cocoa.


As I said to yoz upthread, I don't see why that's true as long as the
ownership arrows point in the same direction.


>
>
>
>  But if you're happy with this patch, I'm happy with it too (with a "do not
>> dereference, might be invalid" comment at the member)
>>
>
> Such a comment would be misleading; the RVH outlives that bridge, but not
> necessarily the controller that was holding onto it weakly.
>
>
https://codereview.chromium.**org/12572006/<https://codereview.chromium.org/1...
>

[not at google - send to devlin, 2013-04-02 04:36:20]

> 
> As I said to yoz upthread, I don't see why that's true as long as the
> ownership arrows point in the same direction.
> 

Ok - I went back and read upthread (I've been on vacation) -

The extension host can't go away. It's the controller that goes away. This
change just hangs onto the deepest node in the tree (the RVH) but we could
equally hold onto the extension host like GTK and views do (but, I would also
point out that each of these already have divergent logic and structure, and it
seems much of a muchness whether I go and change the way this patch is written).

[Yoyo Zhou, 2013-04-09 20:09:11]

On 2013/04/02 04:36:20, kalman wrote:
> > 
> > As I said to yoz upthread, I don't see why that's true as long as the
> > ownership arrows point in the same direction.
> > 
> 
> Ok - I went back and read upthread (I've been on vacation) -
> 
> The extension host can't go away. It's the controller that goes away. This
> change just hangs onto the deepest node in the tree (the RVH) but we could
> equally hold onto the extension host like GTK and views do (but, I would also
> point out that each of these already have divergent logic and structure, and
it
> seems much of a muchness whether I go and change the way this patch is
written).

ping - Matt/Nico?

[Matt Perry, 2013-04-09 20:11:19]

I'm OK with holding onto either the ExtensionHost or RenderViewHost. LGTM either
way.

[Nico, 2013-04-09 20:14:35]

As said above, this feels wrong to me, but if this lgtmp, it also lgtm.

(There's currently no chrome/browser/ui/cocoa/extensions/OWNERS btw. There
should be.)

[commit-bot: I haz the power, 2013-04-09 22:36:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/12572006/13001

[commit-bot: I haz the power, 2013-04-10 06:30:35]

Change committed as 193332