Pepper: add validity checks in RendererPpapiHostImpl::GetDocumentURL

Pepper: add validity checks in RendererPpapiHostImpl::GetDocumentURL

BUG=465765
Committed: https://crrev.com/0e359d8acccc7204ede3cfe8c4e8be5067c1c59d
Cr-Commit-Position: refs/heads/master@{#320827}

[dmichael (off chromium), 2015-03-16 21:39:23]

dmichael@chromium.org changed reviewers:
+ bbudge@chromium.org

[bbudge, 2015-03-16 21:50:33]

https://codereview.chromium.org/1004283003/diff/1/content/renderer/pepper/ren...
File content/renderer/pepper/renderer_ppapi_host_impl.cc (right):

https://codereview.chromium.org/1004283003/diff/1/content/renderer/pepper/ren...
content/renderer/pepper/renderer_ppapi_host_impl.cc:268: return GURL();
You could replace the lines above with a single call to GetContainerForInstance
above.

https://codereview.chromium.org/1004283003/diff/1/content/renderer/pepper/ren...
content/renderer/pepper/renderer_ppapi_host_impl.cc:276: return GURL();
Are you sure we need these checks? There are a bunch of other places where we
don't check these for null:
https://code.google.com/p/chromium/codesearch#search/&q=%22container-%3Eeleme...

[dmichael (off chromium), 2015-03-16 21:57:16]

https://codereview.chromium.org/1004283003/diff/1/content/renderer/pepper/ren...
File content/renderer/pepper/renderer_ppapi_host_impl.cc (right):

https://codereview.chromium.org/1004283003/diff/1/content/renderer/pepper/ren...
content/renderer/pepper/renderer_ppapi_host_impl.cc:268: return GURL();
On 2015/03/16 21:50:33, bbudge wrote:
> You could replace the lines above with a single call to
GetContainerForInstance
> above.
Good call, done.

https://codereview.chromium.org/1004283003/diff/1/content/renderer/pepper/ren...
content/renderer/pepper/renderer_ppapi_host_impl.cc:276: return GURL();
On 2015/03/16 21:50:33, bbudge wrote:
> Are you sure we need these checks? There are a bunch of other places where we
> don't check these for null:
>
https://code.google.com/p/chromium/codesearch#search/&q=%22container-%3Eeleme...
Absolutely not sure. I don't know how we can get in this state :-(. Please see
the last comment in the bug. This is mostly speculative.
https://code.google.com/p/chromium/issues/detail?id=465765#c37

[bbudge, 2015-03-16 22:11:30]

lgtm

https://codereview.chromium.org/1004283003/diff/1/content/renderer/pepper/ren...
File content/renderer/pepper/renderer_ppapi_host_impl.cc (right):

https://codereview.chromium.org/1004283003/diff/1/content/renderer/pepper/ren...
content/renderer/pepper/renderer_ppapi_host_impl.cc:276: return GURL();
On 2015/03/16 21:57:16, dmichael wrote:
> On 2015/03/16 21:50:33, bbudge wrote:
> > Are you sure we need these checks? There are a bunch of other places where
we
> > don't check these for null:
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=%22container-%3Eeleme...
> Absolutely not sure. I don't know how we can get in this state :-(. Please see
> the last comment in the bug. This is mostly speculative.
> https://code.google.com/p/chromium/issues/detail?id=465765#c37

Looking at other usages, it seems unlikely, but OK.

[dmichael (off chromium), 2015-03-16 22:15:46]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2015-03-16 22:16:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1004283003/20001

[dmichael (off chromium), 2015-03-16 22:16:49]

Thanks, Bill.

https://codereview.chromium.org/1004283003/diff/1/content/renderer/pepper/ren...
File content/renderer/pepper/renderer_ppapi_host_impl.cc (right):

https://codereview.chromium.org/1004283003/diff/1/content/renderer/pepper/ren...
content/renderer/pepper/renderer_ppapi_host_impl.cc:276: return GURL();
On 2015/03/16 22:11:29, bbudge wrote:
> On 2015/03/16 21:57:16, dmichael wrote:
> > On 2015/03/16 21:50:33, bbudge wrote:
> > > Are you sure we need these checks? There are a bunch of other places where
> we
> > > don't check these for null:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=%22container-%3Eeleme...
> > Absolutely not sure. I don't know how we can get in this state :-(. Please
see
> > the last comment in the bug. This is mostly speculative.
> > https://code.google.com/p/chromium/issues/detail?id=465765#c37
> 
> Looking at other usages, it seems unlikely, but OK.
I agree with you. I'm partly doing this to get more information...  if the crash
persists, it will rule out that these are Null.

[commit-bot: I haz the power, 2015-03-17 00:08:29]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-17 00:09:32]

Patchset 2 (id:??) landed as
https://crrev.com/0e359d8acccc7204ede3cfe8c4e8be5067c1c59d
Cr-Commit-Position: refs/heads/master@{#320827}