Plugin Power Saver: Add missing RenderFrame null-checks in placeholders

Plugin Power Saver: Add missing RenderFrame null-checks in placeholders

PluginPlaceholders are RenderFrameObservers, and render_frames can
go away before the object is actually destructed (during shutdown).

This CL adds the missing null checks needed to prevent trying to use
null pointers.

BUG=693120
R=raymes@chromium.org

Review-Url: https://codereview.chromium.org/2845413002 .
Cr-Commit-Position: refs/heads/master@{#468451}
Committed: https://chromium.googlesource.com/chromium/src/+/254b5995602a9d0d912d30cc4eb77ab2cb36b2ff

[tommycli, 2017-04-28 17:09:48]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[tommycli, 2017-04-28 17:10:23]

tommycli@chromium.org changed reviewers:
+ raymes@chromium.org

[tommycli, 2017-04-28 17:10:24]

raymes: PTAL, this fixes a crash by adding some discipline about our usage of
render_frame().

This is explained by a comment here:
https://cs.chromium.org/chromium/src/content/public/renderer/render_frame_obs...

[commit-bot: I haz the power, 2017-04-28 17:10:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 18:28:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-28 18:28:39]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-04-30 23:34:33]

lg - just one question. Thanks!

https://codereview.chromium.org/2845413002/diff/1/chrome/renderer/plugins/chr...
File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):

https://codereview.chromium.org/2845413002/diff/1/chrome/renderer/plugins/chr...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:238:
render_frame()->Send(new ChromeViewHostMsg_GetPluginInfo(
Do we need to check on this occasion too?

[tommycli, 2017-05-01 15:37:19]

raymes: Thanks, PTAL again

https://codereview.chromium.org/2845413002/diff/1/chrome/renderer/plugins/chr...
File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):

https://codereview.chromium.org/2845413002/diff/1/chrome/renderer/plugins/chr...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:238:
render_frame()->Send(new ChromeViewHostMsg_GetPluginInfo(
On 2017/04/30 23:34:33, raymes wrote:
> Do we need to check on this occasion too?

Hi. GetFrame() is an equivalent check. It returns a variable which is also
cleared by RenderFrameObserver::OnDestruct.

If you prefer, I'm happy to add a render_frame() nullcheck too, it's just
strictly speaking unnecessary. Or I can add a comment.

[raymes, 2017-05-01 22:10:26]

lgtm

https://codereview.chromium.org/2845413002/diff/1/chrome/renderer/plugins/chr...
File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):

https://codereview.chromium.org/2845413002/diff/1/chrome/renderer/plugins/chr...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:238:
render_frame()->Send(new ChromeViewHostMsg_GetPluginInfo(
On 2017/05/01 15:37:18, tommycli wrote:
> On 2017/04/30 23:34:33, raymes wrote:
> > Do we need to check on this occasion too?
> 
> Hi. GetFrame() is an equivalent check. It returns a variable which is also
> cleared by RenderFrameObserver::OnDestruct.
> 
> If you prefer, I'm happy to add a render_frame() nullcheck too, it's just
> strictly speaking unnecessary. Or I can add a comment.

Ah that makes sense. Thanks :) Maybe add a comment?

[tommycli, 2017-05-01 22:15:52]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[tommycli, 2017-05-01 22:15:56]

https://codereview.chromium.org/2845413002/diff/1/chrome/renderer/plugins/chr...
File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):

https://codereview.chromium.org/2845413002/diff/1/chrome/renderer/plugins/chr...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:238:
render_frame()->Send(new ChromeViewHostMsg_GetPluginInfo(
On 2017/05/01 22:10:26, raymes wrote:
> On 2017/05/01 15:37:18, tommycli wrote:
> > On 2017/04/30 23:34:33, raymes wrote:
> > > Do we need to check on this occasion too?
> > 
> > Hi. GetFrame() is an equivalent check. It returns a variable which is also
> > cleared by RenderFrameObserver::OnDestruct.
> > 
> > If you prefer, I'm happy to add a render_frame() nullcheck too, it's just
> > strictly speaking unnecessary. Or I can add a comment.
> 
> Ah that makes sense. Thanks :) Maybe add a comment? 

Done. Thanks. I added a comment and DCHECK on the following line. I'm going to
send this in bypassing CQ, since ChromeOS TPM needs this crash fix in by 4PM,
and we have high confidence in it.

[commit-bot: I haz the power, 2017-05-01 22:16:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommycli, 2017-05-01 22:16:55]

Description was changed from

==========
Plugin Power Saver: Add missing RenderFrame null-checks in placeholders

PluginPlaceholders are RenderFrameObservers, and render_frames can
go away before the object is actually destructed (during shutdown).

This CL adds the missing null checks needed to prevent trying to use
null pointers.

BUG=693120
==========

to

==========
Plugin Power Saver: Add missing RenderFrame null-checks in placeholders

PluginPlaceholders are RenderFrameObservers, and render_frames can
go away before the object is actually destructed (during shutdown).

This CL adds the missing null checks needed to prevent trying to use
null pointers.

BUG=693120
R=raymes@chromium.org

Review-Url: https://codereview.chromium.org/2845413002 .
Cr-Commit-Position: refs/heads/master@{#468451}
Committed:
https://chromium.googlesource.com/chromium/src/+/254b5995602a9d0d912d30cc4eb7...
==========

[tommycli, 2017-05-01 22:16:57]

Committed patchset #2 (id:20001) manually as
254b5995602a9d0d912d30cc4eb77ab2cb36b2ff (presubmit successful).