[Mac] Prevent ppapi fullscreen window from dealloc while on stack.

[Mac] Prevent ppapi fullscreen window from dealloc while on stack.

If the rwhvm is being destroyed in response to a command accelerator,
the window being deallocated may still be executing code higher on the
stack.

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

[Scott Hess - ex-Googler, 2012-11-16 18:01:14]

Another option would be to override something in the window subclass and grab a
ref while forwarding, but I don't see that as productive.

[Robert Sesek, 2012-11-16 18:07:42]

https://codereview.chromium.org/11416040/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/11416040/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_mac.mm:731:
pepper_fullscreen_window_.autorelease();
Ooh. When did this get born?

https://codereview.chromium.org/11416040/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_mac.mm:731:
pepper_fullscreen_window_.autorelease();
Does this window have a delegate that the window could now outlive?

[Scott Hess - ex-Googler, 2012-11-16 18:17:36]

Sailesh, I realize you're busy elsewhere, just wondering if 10s of thought can
provide any thoughts about refs out from the pepper fullscreen window which
might need to be broken.  I can't find any.

https://codereview.chromium.org/11416040/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/11416040/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_mac.mm:731:
pepper_fullscreen_window_.autorelease();
On 2012/11/16 18:07:43, rsesek wrote:
> Ooh. When did this get born?

Pretty recent.  I had rewritten a piece of code three ways and thought "Maybe
this is too much freedom."

https://codereview.chromium.org/11416040/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_mac.mm:731:
pepper_fullscreen_window_.autorelease();
On 2012/11/16 18:07:43, rsesek wrote:
> Does this window have a delegate that the window could now outlive?

AFAICT, no.  fullscreen_window_manager_ may have a reference to this window, but
the window doesn't have any controller or delegate or anything.  Which makes
sense, as it only exists to cover the screen, you can't interact with it.

[sail, 2012-11-16 18:31:05]

On 2012/11/16 18:17:36, shess wrote:
> Sailesh, I realize you're busy elsewhere, just wondering if 10s of thought can
> provide any thoughts about refs out from the pepper fullscreen window which
> might need to be broken.  I can't find any.

There's no delegate so you're definitely in the clear.

[Robert Sesek, 2012-11-16 19:31:54]

LGTM

[sail, 2012-11-16 19:59:02]

https://codereview.chromium.org/11416040/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/11416040/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_mac.mm:731:
pepper_fullscreen_window_.autorelease();
Could you add a comment here saying that on shutdown this object is still being
referenced earlier in the stack.

[Scott Hess - ex-Googler, 2012-11-16 21:35:20]

Nico for the OWNERS stamp, please?

[Nico, 2012-11-16 21:38:40]

lgtm

lbtm with test, but this is probably hard to test.

[Scott Hess - ex-Googler, 2012-11-16 22:37:28]

Sailesh, you're probably most able to say whether the test is insane, as I based
it on your ESC test.

[sail, 2012-11-16 22:45:55]

test looks awesome!

[commit-bot: I haz the power, 2012-11-16 22:47:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/11416040/3003

[commit-bot: I haz the power, 2012-11-16 23:36:31]

Retried try job too often for step(s) content_unittests

[commit-bot: I haz the power, 2012-11-17 20:56:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/11416040/3003

[commit-bot: I haz the power, 2012-11-17 22:13:37]

Change committed as 168463