Fix a crash in pdf::PepperPDFHost::OnHostMsgHasUnsupportedFeature().

Fix a crash in pdf::PepperPDFHost::OnHostMsgHasUnsupportedFeature().

BUG=627814
Committed: https://crrev.com/d72a1999af854cec09a4b20a69ff5795b8c84c9f
Cr-Commit-Position: refs/heads/master@{#407323}

[Lei Zhang, 2016-07-22 20:33:06]

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

[commit-bot: I haz the power, 2016-07-22 20:33:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2016-07-22 20:35:37]

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

[Lei Zhang, 2016-07-22 20:35:37]

https://codereview.chromium.org/2174963002/diff/1/components/pdf/renderer/pep...
File components/pdf/renderer/pepper_pdf_host.cc (left):

https://codereview.chromium.org/2174963002/diff/1/components/pdf/renderer/pep...
components/pdf/renderer/pepper_pdf_host.cc:148:
CHECK(instance->GetContainer()->document().frame());
I found this CHECK() failed, but then I looked below and found a better way to
get the render view.

https://codereview.chromium.org/2174963002/diff/1/components/pdf/renderer/pep...
components/pdf/renderer/pepper_pdf_host.cc:170: content::RenderView* render_view
= instance->GetRenderView();
This can also return a nullptr and crash later down, but it's a much rarer
crash.

[tommycli, 2016-07-22 20:42:15]

lgtm, but I want to confirm:

Sometimes the RenderView may legitimately be null right? So it shouldn't crash
right?

In other words: it can happen for some reason other than programmer error right?

[Lei Zhang, 2016-07-22 21:00:31]

On 2016/07/22 20:42:15, tommycli wrote:
> lgtm, but I want to confirm:
> 
> Sometimes the RenderView may legitimately be null right? So it shouldn't crash
> right?
> 
> In other words: it can happen for some reason other than programmer error
right?

Not 100% sure, but it seems wrong to crash when we can handle the error. I
actually found yet more crashes in related functions, so there will be a new CL
shortly.

[tommycli, 2016-07-22 21:04:20]

On 2016/07/22 20:42:15, tommycli wrote:
> lgtm, but I want to confirm:
> 
> Sometimes the RenderView may legitimately be null right? So it shouldn't crash
> right?
> 
> In other words: it can happen for some reason other than programmer error
right?

I dug into PepperPluginInstanceImpl a bit. It seems that YES, it can return a
nullptr RenderView() if its render_frame_ is null.

Internally, it has "if (render_frame_)" clauses, and render_frame_ can be null
if it has been destroyed and it calls the "OnDestruct" RenderFrameObserver
method.

In short: I think these changes are doing the right thing.

[commit-bot: I haz the power, 2016-07-22 21:17:38]

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

[commit-bot: I haz the power, 2016-07-22 21:17:39]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-07-22 21:41:11]

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

[commit-bot: I haz the power, 2016-07-22 21:41:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2016-07-22 21:41:49]

Patch set 2 has more of the same checks all over the file.

[tommycli, 2016-07-22 21:47:37]

some more suggestions

https://codereview.chromium.org/2174963002/diff/20001/components/pdf/renderer...
File components/pdf/renderer/pepper_pdf_host.cc (right):

https://codereview.chromium.org/2174963002/diff/20001/components/pdf/renderer...
components/pdf/renderer/pepper_pdf_host.cc:108: render_view->DidStartLoading();
Since this pattern is now repeated in many methods, can we make a helper
function that looks like GetRenderView() which returns nullptr if either
instance or instance->GetRenderView is null?

https://codereview.chromium.org/2174963002/diff/20001/components/pdf/renderer...
File components/pdf/renderer/pepper_pdf_host.h (right):

https://codereview.chromium.org/2174963002/diff/20001/components/pdf/renderer...
components/pdf/renderer/pepper_pdf_host.h:38: }  // ppapi
}  // namespace ppapi

Also... since there's a \n after namespace ppapi, maybe there should be a \n
before this too

[Lei Zhang, 2016-07-22 22:18:27]

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

[commit-bot: I haz the power, 2016-07-22 22:18:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2016-07-22 22:20:21]

https://codereview.chromium.org/2174963002/diff/20001/components/pdf/renderer...
File components/pdf/renderer/pepper_pdf_host.cc (right):

https://codereview.chromium.org/2174963002/diff/20001/components/pdf/renderer...
components/pdf/renderer/pepper_pdf_host.cc:108: render_view->DidStartLoading();
On 2016/07/22 21:47:37, tommycli wrote:
> Since this pattern is now repeated in many methods, can we make a helper
> function that looks like GetRenderView() which returns nullptr if either
> instance or instance->GetRenderView is null?

Done.

https://codereview.chromium.org/2174963002/diff/20001/components/pdf/renderer...
File components/pdf/renderer/pepper_pdf_host.h (right):

https://codereview.chromium.org/2174963002/diff/20001/components/pdf/renderer...
components/pdf/renderer/pepper_pdf_host.h:38: }  // ppapi
On 2016/07/22 21:47:37, tommycli wrote:
> }  // namespace ppapi
> 
> Also... since there's a \n after namespace ppapi, maybe there should be a \n
> before this too

Compacted.

[tommycli, 2016-07-22 22:29:11]

lgtm sans nit.

https://codereview.chromium.org/2174963002/diff/20001/components/pdf/renderer...
File components/pdf/renderer/pepper_pdf_host.h (right):

https://codereview.chromium.org/2174963002/diff/20001/components/pdf/renderer...
components/pdf/renderer/pepper_pdf_host.h:38: }  // ppapi
On 2016/07/22 22:20:21, Lei Zhang wrote:
> On 2016/07/22 21:47:37, tommycli wrote:
> > }  // namespace ppapi
> > 
> > Also... since there's a \n after namespace ppapi, maybe there should be a \n
> > before this too
> 
> Compacted.

Hey I think it should probably still be // namespace ppapi. At least that seems
much more common in the codebase than plain // ppapi. Same with 'host'

https://cs.chromium.org/search/?q=%22%7D++//+namespace+ppapi%22&sq=package:ch...

[Lei Zhang, 2016-07-22 22:50:13]

On 2016/07/22 22:29:11, tommycli wrote:
> Hey I think it should probably still be // namespace ppapi. At least that
seems
> much more common in the codebase than plain // ppapi. Same with 'host'
> 
>
https://cs.chromium.org/search/?q=%22%7D++//+namespace+ppapi%22&sq=package:ch...

Oh ya, I'm being sloppy.

[Lei Zhang, 2016-07-22 22:51:34]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2016-07-22 22:51:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/2174963002/#ps60001
(title: "niiiiiiiits!")

[commit-bot: I haz the power, 2016-07-22 22:52:06]

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, 2016-07-23 00:58:36]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-23 01:00:23]

Description was changed from

==========
Fix a crash in pdf::PepperPDFHost::OnHostMsgHasUnsupportedFeature().

BUG=627814
==========

to

==========
Fix a crash in pdf::PepperPDFHost::OnHostMsgHasUnsupportedFeature().

BUG=627814

Committed: https://crrev.com/d72a1999af854cec09a4b20a69ff5795b8c84c9f
Cr-Commit-Position: refs/heads/master@{#407323}
==========

[commit-bot: I haz the power, 2016-07-23 01:00:23]

Patchset 4 (id:??) landed as
https://crrev.com/d72a1999af854cec09a4b20a69ff5795b8c84c9f
Cr-Commit-Position: refs/heads/master@{#407323}