Allow renderer to create pepper ResourceHosts in the browser

Allow renderer to create pepper ResourceHosts in the browser

This CL allows pending ResourceHosts to be created in the browser from the renderer. The functionality is currently exposed through the RendererPpapiHost. The creation call is asynchronous - a callback will be called in the renderer with a pending host ID when the host has been created.

BUG=246396
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204954

[teravest, 2013-05-29 15:19:36]

https://codereview.chromium.org/15947004/diff/2001/content/public/renderer/re...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/15947004/diff/2001/content/public/renderer/re...
content/public/renderer/renderer_ppapi_host.h:146: // returned upon error.
small nit: Should this be "sent to the callback" instead of returned?

https://codereview.chromium.org/15947004/diff/2001/content/renderer/pepper/re...
File content/renderer/pepper/renderer_ppapi_host_impl.h (right):

https://codereview.chromium.org/15947004/diff/2001/content/renderer/pepper/re...
content/renderer/pepper/renderer_ppapi_host_impl.h:111:
base::Callback<void(int32)> callback) const OVERRIDE;
nit: You've got int32 here, and int in the implementation.

https://codereview.chromium.org/15947004/diff/2001/ppapi/host/ppapi_host.cc
File ppapi/host/ppapi_host.cc (right):

https://codereview.chromium.org/15947004/diff/2001/ppapi/host/ppapi_host.cc#n...
ppapi/host/ppapi_host.cc:227: return;
What should callers of AttachToPendingHost() do to detect this case? Since the
information remains in pending_resource_hosts_, it looks like you're allowing
the caller to retry on failure, but I don't know how it would be detected.

[raymes, 2013-05-29 15:53:03]

https://codereview.chromium.org/15947004/diff/2001/content/public/renderer/re...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/15947004/diff/2001/content/public/renderer/re...
content/public/renderer/renderer_ppapi_host.h:146: // returned upon error.
On 2013/05/29 15:19:36, teravest wrote:
> small nit: Should this be "sent to the callback" instead of returned?

Done.

https://codereview.chromium.org/15947004/diff/2001/content/renderer/pepper/re...
File content/renderer/pepper/renderer_ppapi_host_impl.h (right):

https://codereview.chromium.org/15947004/diff/2001/content/renderer/pepper/re...
content/renderer/pepper/renderer_ppapi_host_impl.h:111:
base::Callback<void(int32)> callback) const OVERRIDE;
good catch thanks

https://codereview.chromium.org/15947004/diff/2001/ppapi/host/ppapi_host.cc
File ppapi/host/ppapi_host.cc (right):

https://codereview.chromium.org/15947004/diff/2001/ppapi/host/ppapi_host.cc#n...
ppapi/host/ppapi_host.cc:227: return;
Oops this shouldn't be here. I already added the checks in the 2 places where we
add things to the map.

[teravest, 2013-05-29 16:40:09]

lgtm

[teravest, 2013-05-29 16:57:13]

https://codereview.chromium.org/15947004/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right):

https://codereview.chromium.org/15947004/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_renderer_connection.cc:63:
NOTREACHED();
One late comment, from
https://sites.google.com/a/chromium.org/dev/Home/chromium-security/security-t...:

Avoid browser side DCHECKs and WebKit ASSERTs that fail with invalid input from
the renderer. Such conditions should be explicitly checked instead. Otherwise
the macros lead to false sense of security since they aren't compiled into
release builds.

[raymes, 2013-05-29 17:10:30]

On 2013/05/29 16:57:13, teravest wrote:
>
https://codereview.chromium.org/15947004/diff/9001/content/browser/renderer_h...
> File content/browser/renderer_host/pepper/pepper_renderer_connection.cc
(right):
> 
>
https://codereview.chromium.org/15947004/diff/9001/content/browser/renderer_h...
> content/browser/renderer_host/pepper/pepper_renderer_connection.cc:63:
> NOTREACHED();
> One late comment, from
>
https://sites.google.com/a/chromium.org/dev/Home/chromium-security/security-t...:
> 
> Avoid browser side DCHECKs and WebKit ASSERTs that fail with invalid input
from
> the renderer. Such conditions should be explicitly checked instead. Otherwise
> the macros lead to false sense of security since they aren't compiled into
> release builds.

This is why I have a separate branch here. If a malicious renderer sends a bad
id, I return a pending host id of 0, which is a safe failure. The NOTREACHED
helps to catch this case in debug mode (i.e. if it's the programmers fault).

[teravest, 2013-05-29 17:13:16]

lgtm

On 2013/05/29 17:10:30, raymes wrote:
> On 2013/05/29 16:57:13, teravest wrote:
> >
>
https://codereview.chromium.org/15947004/diff/9001/content/browser/renderer_h...
> > File content/browser/renderer_host/pepper/pepper_renderer_connection.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/15947004/diff/9001/content/browser/renderer_h...
> > content/browser/renderer_host/pepper/pepper_renderer_connection.cc:63:
> > NOTREACHED();
> > One late comment, from
> >
>
https://sites.google.com/a/chromium.org/dev/Home/chromium-security/security-t...:
> > 
> > Avoid browser side DCHECKs and WebKit ASSERTs that fail with invalid input
> from
> > the renderer. Such conditions should be explicitly checked instead.
Otherwise
> > the macros lead to false sense of security since they aren't compiled into
> > release builds.
> 
> This is why I have a separate branch here. If a malicious renderer sends a bad
> id, I return a pending host id of 0, which is a safe failure. The NOTREACHED
> helps to catch this case in debug mode (i.e. if it's the programmers fault).

Right. I think the suggestion is to not bother with NOTREACHED() at all. Then
it's more likely that the branch where you return a pending host id of 0 will
get coverage.

I don't feel too strongly about this, though.

[yzshen1, 2013-05-29 17:57:21]

It is a little bit unfortunate that it gets more and more complex. But maybe
that is unavoidable.

https://codereview.chromium.org/15947004/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right):

https://codereview.chromium.org/15947004/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_renderer_connection.cc:56: return;
Do you mean 'break'?

https://codereview.chromium.org/15947004/diff/9001/content/public/renderer/re...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/15947004/diff/9001/content/public/renderer/re...
content/public/renderer/renderer_ppapi_host.h:147: virtual void
CreateBrowserResourceHost(
[I need to double check the code.]
If the resource host at the browser side is created and the renderer dies before
it receive the response, will it leak the resource host?

https://codereview.chromium.org/15947004/diff/9001/content/public/renderer/re...
content/public/renderer/renderer_ppapi_host.h:150: base::Callback<void(int)>
callback) const = 0;
const &?

https://codereview.chromium.org/15947004/diff/9001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_browser_connection.h (right):

https://codereview.chromium.org/15947004/diff/9001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_browser_connection.h:38:
PendingResourceIDCallback callback);
const &?

https://codereview.chromium.org/15947004/diff/9001/webkit/plugins/ppapi/plugi...
File webkit/plugins/ppapi/plugin_module.h (right):

https://codereview.chromium.org/15947004/diff/9001/webkit/plugins/ppapi/plugi...
webkit/plugins/ppapi/plugin_module.h:133: // of process. Returns 0 otherwise.
This is the ID that the browser process
two 'of'.

[raymes, 2013-06-04 04:11:10]

https://codereview.chromium.org/15947004/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right):

https://codereview.chromium.org/15947004/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_renderer_connection.cc:56: return;
On 2013/05/29 17:57:21, yzshen1 wrote:
> Do you mean 'break'?

Done.

https://codereview.chromium.org/15947004/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_renderer_connection.cc:63:
NOTREACHED();
On 2013/05/29 16:57:13, teravest wrote:
> One late comment, from
>
https://sites.google.com/a/chromium.org/dev/Home/chromium-security/security-t...:
> 
> Avoid browser side DCHECKs and WebKit ASSERTs that fail with invalid input
from
> the renderer. Such conditions should be explicitly checked instead. Otherwise
> the macros lead to false sense of security since they aren't compiled into
> release builds.

Done.

https://codereview.chromium.org/15947004/diff/9001/content/public/renderer/re...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/15947004/diff/9001/content/public/renderer/re...
content/public/renderer/renderer_ppapi_host.h:147: virtual void
CreateBrowserResourceHost(
This doesn't happen right now. I will fix it in a separate CL.

https://codereview.chromium.org/15947004/diff/9001/content/public/renderer/re...
content/public/renderer/renderer_ppapi_host.h:150: base::Callback<void(int)>
callback) const = 0;
On 2013/05/29 17:57:21, yzshen1 wrote:
> const &?

Done.

https://codereview.chromium.org/15947004/diff/9001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_browser_connection.h (right):

https://codereview.chromium.org/15947004/diff/9001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_browser_connection.h:38:
PendingResourceIDCallback callback);
On 2013/05/29 17:57:21, yzshen1 wrote:
> const &?

Done.

https://codereview.chromium.org/15947004/diff/9001/webkit/plugins/ppapi/plugi...
File webkit/plugins/ppapi/plugin_module.h (right):

https://codereview.chromium.org/15947004/diff/9001/webkit/plugins/ppapi/plugi...
webkit/plugins/ppapi/plugin_module.h:133: // of process. Returns 0 otherwise.
This is the ID that the browser process
On 2013/05/29 17:57:21, yzshen1 wrote:
> two 'of'.

Done.

[yzshen1, 2013-06-04 16:21:32]

lgtm

[raymes, 2013-06-04 16:44:40]

+brettw

[brettw, 2013-06-06 20:06:18]

Owners LGTM.

We discussed verbally:
- Make sure the messages come in on the I/O thread in the browser process
- See if the observer can be moved to the helper rather than on the RVimpl.

[raymes, 2013-06-06 21:20:02]

+jschuh for ppapi_messages.h

Note that the renderer could lie about the plugin process ID. In this case it is
safe, because all it can do is create pending resource hosts for plugins and it
can't actually do anything with them (only the plugin can). As we discussed, I
still need to make a change to use a plugin GUID globally.

[jschuh, 2013-06-06 21:23:40]

On 2013/06/06 21:20:02, raymes wrote:
> +jschuh for ppapi_messages.h
> 
> Note that the renderer could lie about the plugin process ID. In this case it
is
> safe, because all it can do is create pending resource hosts for plugins and
it
> can't actually do anything with them (only the plugin can). As we discussed, I
> still need to make a change to use a plugin GUID globally.

lgtm based on previous discussion.

[commit-bot: I haz the power, 2013-06-07 17:34:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/15947004/51001

[commit-bot: I haz the power, 2013-06-07 22:15:26]

Change committed as 204954