Add NaCl proxies for Pepper Video Source and Destination resources.

Add NaCl proxies for Pepper Video Source and Destination resources.

I need to add whitelist testing to prevent arbitrary NaCl apps from using these resources.

BUG=230980
TEST=browser_tests, --gtest_filter="PPAPINaClPNaClTest.Video*"

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

[bbudge, 2013-06-03 23:48:33]

David, can you check that I'm creating the ImageData correctly in
video_source_resource.cc?

[dmichael (off chromium), 2013-06-04 17:10:49]

That chunk looks right to me. I think you may understand this stuff better at
this point than I do ;-)

I haven't looked at anything else yet.

https://codereview.chromium.org/16335018/diff/7001/ppapi/proxy/video_source_r...
File ppapi/proxy/video_source_resource.cc (right):

https://codereview.chromium.org/16335018/diff/7001/ppapi/proxy/video_source_r...
ppapi/proxy/video_source_resource.cc:111: frame->image_data = 0;
Do we do OS_ANDROID checks anywhere else in PPAPI? Do they even build ppapi? I
know it's not supported there. Might be okay to just not have this case.

[bbudge, 2013-06-04 23:46:28]

https://codereview.chromium.org/16335018/diff/7001/ppapi/proxy/video_source_r...
File ppapi/proxy/video_source_resource.cc (right):

https://codereview.chromium.org/16335018/diff/7001/ppapi/proxy/video_source_r...
ppapi/proxy/video_source_resource.cc:111: frame->image_data = 0;
On 2013/06/04 17:10:49, dmichael wrote:
> Do we do OS_ANDROID checks anywhere else in PPAPI? Do they even build ppapi? I
> know it's not supported there. Might be okay to just not have this case.

Done.

[dmichael (off chromium), 2013-06-05 01:54:42]

https://codereview.chromium.org/16335018/diff/2016/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_video_source_host.cc (right):

https://codereview.chromium.org/16335018/diff/2016/content/renderer/pepper/pe...
content/renderer/pepper/pepper_video_source_host.cc:201: int shmem_key =
You should be able to get rid of this and the associated parameter on the
message now, right?

https://codereview.chromium.org/16335018/diff/2016/ppapi/proxy/video_source_r...
File ppapi/proxy/video_source_resource.cc (right):

https://codereview.chromium.org/16335018/diff/2016/ppapi/proxy/video_source_r...
ppapi/proxy/video_source_resource.cc:110: #if defined(TOOLKIT_GTK)
What's this case for? Can you just use the SharedMemoryHandle exclusively now?

[bbudge, 2013-06-05 02:05:03]

https://codereview.chromium.org/16335018/diff/2016/ppapi/proxy/video_source_r...
File ppapi/proxy/video_source_resource.cc (right):

https://codereview.chromium.org/16335018/diff/2016/ppapi/proxy/video_source_r...
ppapi/proxy/video_source_resource.cc:110: #if defined(TOOLKIT_GTK)
The difficulty is that the ImageData constructor that takes a SharedMemoryHandle
is only available for NaCl. I could expose it but only for Linux since
ImageHandle and SharedMemoryHandle are the same type on other platforms, so this
would be ambiguous. That's probably the way to go, since this CL isn't passing
for Linux non-NaCl cases. WDYT?

On 2013/06/05 01:54:42, dmichael wrote:
> What's this case for? Can you just use the SharedMemoryHandle exclusively now?

[dmichael (off chromium), 2013-06-05 02:15:58]

On 2013/06/05 02:05:03, bbudge1 wrote:
>
https://codereview.chromium.org/16335018/diff/2016/ppapi/proxy/video_source_r...
> File ppapi/proxy/video_source_resource.cc (right):
> 
>
https://codereview.chromium.org/16335018/diff/2016/ppapi/proxy/video_source_r...
> ppapi/proxy/video_source_resource.cc:110: #if defined(TOOLKIT_GTK)
> The difficulty is that the ImageData constructor that takes a
SharedMemoryHandle
> is only available for NaCl. I could expose it but only for Linux since
> ImageHandle and SharedMemoryHandle are the same type on other platforms, so
this
> would be ambiguous. That's probably the way to go, since this CL isn't passing
> for Linux non-NaCl cases. WDYT?
> 
Yes, that sounds right. I think my intent with the ifdefs back then had more to
do with not allowing NaCl to do it any other way; preventing using the NaCl form
on other platforms was more for clarity I think.

Speaking of clarity, it might make sense to change some names to make them less
NaCl specific?

> On 2013/06/05 01:54:42, dmichael wrote:
> > What's this case for? Can you just use the SharedMemoryHandle exclusively
now?

[bbudge, 2013-06-05 19:18:52]

After looking at ppb_image_data_proxy.cc, I realized it would be non-trivial to
make the "NaCl style" and GTK style ImageData work at the same time. The
existing code uses #ifdefs to partition the behaviors, and I'd have to include
both code branches with some test to determine which style was intended. Another
possibility would be to derive a separate class for each style, but that's too
big a change I think.

I went back to sending a bool indicating if it's for a NaCl plugin.

https://codereview.chromium.org/16335018/diff/2016/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_video_source_host.cc (right):

https://codereview.chromium.org/16335018/diff/2016/content/renderer/pepper/pe...
content/renderer/pepper/pepper_video_source_host.cc:201: int shmem_key =
It's still needed on Linux for trusted plugins.
On 2013/06/05 01:54:42, dmichael wrote:
> You should be able to get rid of this and the associated parameter on the
> message now, right?

https://codereview.chromium.org/16335018/diff/38001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/16335018/diff/38001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1560: bool /* is_nacl_plugin */)
This is the only change to the messages aside from exposing them to NaCl builds.

[bbudge, 2013-06-09 00:14:17]

This includes the ImageData refactoring. Once that's done, this will be ready
for review

[bbudge, 2013-06-19 00:09:08]

David for PPAPI stuff.
Mustafa for IPC security review - ppapi_messages.h, and overall whitelist
checking in renderer process.

[bbudge, 2013-06-19 00:21:30]

On 2013/06/19 00:09:08, bbudge1 wrote:
> David for PPAPI stuff.
> Mustafa for IPC security review - ppapi_messages.h, and overall whitelist
> checking in renderer process.

Try's against LKGR fail, need more recent revision.

[bbudge, 2013-06-19 00:22:32]

https://codereview.chromium.org/16335018/diff/2016/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_video_source_host.cc (right):

https://codereview.chromium.org/16335018/diff/2016/content/renderer/pepper/pe...
content/renderer/pepper/pepper_video_source_host.cc:201: int shmem_key =
On 2013/06/05 19:18:52, bbudge1 wrote:
> It's still needed on Linux for trusted plugins.
> On 2013/06/05 01:54:42, dmichael wrote:
> > You should be able to get rid of this and the associated parameter on the
> > message now, right?
> 

Done.

https://codereview.chromium.org/16335018/diff/2016/ppapi/proxy/video_source_r...
File ppapi/proxy/video_source_resource.cc (right):

https://codereview.chromium.org/16335018/diff/2016/ppapi/proxy/video_source_r...
ppapi/proxy/video_source_resource.cc:110: #if defined(TOOLKIT_GTK)
This issue was solved by the recent ImageData refactoring. Done.

[meacer, 2013-06-19 02:05:11]

https://codereview.chromium.org/16335018/diff/67001/ppapi/proxy/video_source_...
File ppapi/proxy/video_source_resource.cc (right):

https://codereview.chromium.org/16335018/diff/67001/ppapi/proxy/video_source_...
ppapi/proxy/video_source_resource.cc:96: PP_VideoFrame_Private* frame,
It seems possible that frame->image_data is not initialized after this method.
Is that okay? Just asking.

[bbudge, 2013-06-19 16:55:00]

https://codereview.chromium.org/16335018/diff/67001/ppapi/proxy/video_source_...
File ppapi/proxy/video_source_resource.cc (right):

https://codereview.chromium.org/16335018/diff/67001/ppapi/proxy/video_source_...
ppapi/proxy/video_source_resource.cc:96: PP_VideoFrame_Private* frame,
Yes, the API contract is that the frame isn't written to unless the function
succeeds. I will add some documentation to that effect in the .idl file for this
API.

[dmichael (off chromium), 2013-06-19 17:50:23]

This looks great, thank you for going through the trouble of fixing ImageData so
this could be so clean. I think it will pay off more later, too...  I think
we'll kill off the "Platform" version eventually.

Anyway, lgtm

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/16335018/80001

[meacer, 2013-06-19 17:53:38]

On 2013/06/19 16:55:00, bbudge1 wrote:
>
https://codereview.chromium.org/16335018/diff/67001/ppapi/proxy/video_source_...
> File ppapi/proxy/video_source_resource.cc (right):
> 
>
https://codereview.chromium.org/16335018/diff/67001/ppapi/proxy/video_source_...
> ppapi/proxy/video_source_resource.cc:96: PP_VideoFrame_Private* frame,
> Yes, the API contract is that the frame isn't written to unless the function
> succeeds. I will add some documentation to that effect in the .idl file for
this
> API.

Thanks, lgtm.

[commit-bot: I haz the power, 2013-06-19 18:06:40]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[bbudge, 2013-06-19 18:14:38]

+Raymes for content/renderer/pepper

[bbudge, 2013-06-19 18:16:19]

+Chris for IPC OWNERS, meacer has reviewed messages

[raymes, 2013-06-19 18:40:49]

lgtm


On Wed, Jun 19, 2013 at 11:14 AM, <bbudge@chromium.org> wrote:

> +Raymes for content/renderer/pepper
>
>
https://codereview.chromium.**org/16335018/<https://codereview.chromium.org/1...
>

[palmer, 2013-06-19 18:57:49]

https://codereview.chromium.org/16335018/diff/80001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/16335018/diff/80001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1651: std::string /* stream_url */)
Shouldn't this, and the |stream_url| on line 1661, be a GURL?

[bbudge, 2013-06-19 19:33:12]

https://codereview.chromium.org/16335018/diff/80001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/16335018/diff/80001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1651: std::string /* stream_url */)
GURL isn't available on the Plugin side of the proxy for NaCl. We do validate
the URLs before using them on the host side. See pepper_video_source_host.cc and
pepper_video_destination_host.cc.

[palmer, 2013-06-19 20:27:09]

lgtm

[commit-bot: I haz the power, 2013-06-20 01:43:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/16335018/80001

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

Change committed as 207348