Fix use-after-free related to RenderFullScreen

Fix use-after-free related to RenderFullScreen

BUG=357452
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179436

[philipj_slow, 2014-07-25 23:13:47]

PTAL.

I went for a very localized fix in case this needs to be backported, but the
price of that is that setFullScreenRenderer depends on the early return to avoid
infinite recursion in a pretty non-obvious way. If you think I've identified the
real problem I'm happy to discuss a less ugly fix or future refactoring.

[esprehn, 2014-07-25 23:35:37]

What calls destroy() now, it looks like you're just leaking the renderers.

[philipj_slow, 2014-07-26 08:08:57]

Uh, right, I explained that away as "whatever unwrapRenderer() does" even though
I've changed what it does :/

The new deal is to note that setFullScreenRenderer(0) was equivalent to
m_fullScreenRenderer->destroy(), so first do that replacement. Now it becomes
straightforward to call unwrapRenderer() instead of destroy() in
setFullScreenRenderer(), and at least the test passes.

TBH, I'm not really clear on the ownership or the lifetime of these objects, but
it looks reasonable that unwrapRenderer() should destroy what wrapRenderer()
creates, and that they should be balanced. However, now unwrapRenderer() is
going to end up being called at the very end of wrapRenderer(). Another fix
which also seems to work is to call unwrapRenderer() on the old renderer (if
any) at the beginning of wrapRenderer(). Not sure if there are (untested) cases
where it makes a difference.

Is there a tool I can run to get some confidence that I haven't introduced
leaks?

[philipj_slow, 2014-07-29 14:54:36]

Ping falken and esprehn.

[falken, 2014-07-31 04:26:35]

Ack, sorry I missed this. I intend to look today or tomorrow. +esprehn as well
(he wasn't on reviewers list).

[esprehn, 2014-08-01 14:00:42]

lgtm, thanks for fixing this!

[philipj_slow, 2014-08-01 15:09:05]

Matt, wanna have a look before I land this?

[falken, 2014-08-02 04:06:57]

On 2014/08/01 15:09:05, philipj wrote:
> Matt, wanna have a look before I land this?

Elliott's lg is enough for me please go ahead and land. Thanks for fixing!

[philipj_slow, 2014-08-02 04:30:31]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-08-02 04:30:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/philipj@opera.com/414303006/20001

[commit-bot: I haz the power, 2014-08-02 04:33:43]

Change committed as 179436