Update pepper to not assume that SharedMemoryHandle is an int.

Update pepper to not assume that SharedMemoryHandle is an int.

This CL is a refactor. This CL contains no intended behavior changes.

Pepper code assumes that SharedMemoryHandle is backed by a PlatformFile, and
that the relevant HANDLE or fd can be cast to an int. These assumptions will no
longer be true once SharedMemory is backed by Mach primitives on Mac.

This CL adds the method ShareSharedMemoryHandleWithRemote() to
ProxyChannel::Delegate. This method is used in place of ShareHandleWithRemote()
when a SharedMemory object is being shared between processes. This CL updates
the type of all SharedMemory handles to be SharedMemoryHandle.

BUG=466437
Committed: https://crrev.com/4fc32d5dd347edbc0dfac470e7fa9b78b0923b72
Cr-Commit-Position: refs/heads/master@{#332325}

[erikchen, 2015-05-28 20:27:39]

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

[commit-bot: I haz the power, 2015-05-28 20:28:24]

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

[commit-bot: I haz the power, 2015-05-28 20:53:57]

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

[commit-bot: I haz the power, 2015-05-28 20:53:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2015-05-29 00:31:13]

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

[commit-bot: I haz the power, 2015-05-29 00:32:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154613006/40001

[commit-bot: I haz the power, 2015-05-29 01:37:50]

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

[commit-bot: I haz the power, 2015-05-29 01:37:51]

Dry run: This issue passed the CQ dry run.

[erikchen, 2015-05-29 21:50:38]

erikchen@chromium.org changed reviewers:
+ piman@chromium.org

[erikchen, 2015-05-29 21:50:39]

piman: Please review.

The changes to base/memory are being reviewed in a separate CL:
https://codereview.chromium.org/1164473003/. They are only here for reference.

There is currently duplication of code in the implementations of
ShareSharedMemoryHandleWithRemote(). There will be a follow up CL to remove this
duplication using a new sandbox method I'm introducing at:
https://codereview.chromium.org/1166473003/

[erikchen, 2015-06-01 19:54:40]

On 2015/05/29 21:50:39, erikchen wrote:
> piman: Please review.
> 
> The changes to base/memory are being reviewed in a separate CL:
> https://codereview.chromium.org/1164473003/. They are only here for reference.
> 
> There is currently duplication of code in the implementations of
> ShareSharedMemoryHandleWithRemote(). There will be a follow up CL to remove
this
> duplication using a new sandbox method I'm introducing at:
> https://codereview.chromium.org/1166473003/

The changes to base/memory have landed, so I've removed the changes from patch
set 4.

[piman, 2015-06-01 22:16:08]

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
File content/renderer/pepper/audio_helper.cc (right):

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
content/renderer/pepper/audio_helper.cc:34: *shm_handle =
base::SharedMemory::ShallowCopyHandle(
Do we need ShallowCopyHandle at all? Can we pass the handle directly?

Actually, I think we could even pass back the SharedMemory directly. The
"trusted API" isn't actually exposed to plugins, so we can keep using Chrome
types all the way.

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
File content/renderer/pepper/pepper_compositor_host.cc (right):

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:137:
base::SharedMemory::DeepCopyHandle(handle, false);
Can you do this in both OS_WIN and !OS_WIN paths?

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
File content/renderer/pepper/pepper_proxy_channel_delegate_impl.cc (right):

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
content/renderer/pepper/pepper_proxy_channel_delegate_impl.cc:42:
reinterpret_cast<HANDLE>(static_cast<intptr_t>(handle));
not sure why we need those 2 casts. handle is already a HANDLE.

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
File content/renderer/pepper/ppb_buffer_impl.cc (right):

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
content/renderer/pepper/ppb_buffer_impl.cc:82: *shm_handle =
base::SharedMemory::ShallowCopyHandle(shared_memory_->handle());
Same here, do we need ShallowCopy, or can we just pass shared_memory_ back
directly?

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
File content/renderer/pepper/ppb_image_data_impl.cc (right):

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
content/renderer/pepper/ppb_image_data_impl.cc:195: *handle =
base::SharedMemory::ShallowCopyHandle(dib_->handle());
ditto.

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
File content/renderer/pepper/renderer_ppapi_host_impl.cc (right):

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
content/renderer/pepper/renderer_ppapi_host_impl.cc:241:
reinterpret_cast<HANDLE>(static_cast<intptr_t>(handle));
ditto, no need for casts.

https://codereview.chromium.org/1154613006/diff/60001/ppapi/proxy/ppapi_proxy...
File ppapi/proxy/ppapi_proxy_test.cc (right):

https://codereview.chromium.org/1154613006/diff/60001/ppapi/proxy/ppapi_proxy...
ppapi/proxy/ppapi_proxy_test.cc:267:
reinterpret_cast<HANDLE>(static_cast<intptr_t>(handle)), remote_pid,
ditto, no need for casts.

https://codereview.chromium.org/1154613006/diff/60001/ppapi/proxy/ppapi_proxy...
ppapi/proxy/ppapi_proxy_test.cc:515:
reinterpret_cast<HANDLE>(static_cast<intptr_t>(handle)), remote_pid,
ditto

https://codereview.chromium.org/1154613006/diff/60001/ppapi/proxy/proxy_chann...
File ppapi/proxy/proxy_channel.cc (right):

https://codereview.chromium.org/1154613006/diff/60001/ppapi/proxy/proxy_chann...
ppapi/proxy/proxy_channel.cc:83:
reinterpret_cast<HANDLE>(static_cast<intptr_t>(handle)), false);
ditto wrt casts

[erikchen, 2015-06-01 23:44:36]

Patchset #5 (id:80001) has been deleted

[erikchen, 2015-06-01 23:48:53]

piman: PTAL

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
File content/renderer/pepper/audio_helper.cc (right):

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
content/renderer/pepper/audio_helper.cc:34: *shm_handle =
base::SharedMemory::ShallowCopyHandle(
On 2015/06/01 22:16:08, piman (Very slow to review) wrote:
> Do we need ShallowCopyHandle at all? Can we pass the handle directly?
> 
> Actually, I think we could even pass back the SharedMemory directly. The
> "trusted API" isn't actually exposed to plugins, so we can keep using Chrome
> types all the way.

Good suggestion. Done.

We should also be able to remove shm_size, since that's a field on
base::SharedMemory. That being said, I am not doing so in this CL, and I do not
plan to write a followup CL to do so. Let me know if you think this is an
important task.

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
File content/renderer/pepper/pepper_compositor_host.cc (right):

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:137:
base::SharedMemory::DeepCopyHandle(handle, false);
On 2015/06/01 22:16:08, piman (Very slow to review) wrote:
> Can you do this in both OS_WIN and !OS_WIN paths?

Yes. I will do so in a follow up CL. DeepCopyHandle() isn't defined on OS_WIN
yet, so I'll need to add that to base/memory first.

I removed an unnecessary cast to base::SharedMemoryHandle.

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
File content/renderer/pepper/pepper_proxy_channel_delegate_impl.cc (right):

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
content/renderer/pepper/pepper_proxy_channel_delegate_impl.cc:42:
reinterpret_cast<HANDLE>(static_cast<intptr_t>(handle));
On 2015/06/01 22:16:08, piman (Very slow to review) wrote:
> not sure why we need those 2 casts. handle is already a HANDLE.

You're right. Fixed.

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
File content/renderer/pepper/ppb_buffer_impl.cc (right):

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
content/renderer/pepper/ppb_buffer_impl.cc:82: *shm_handle =
base::SharedMemory::ShallowCopyHandle(shared_memory_->handle());
On 2015/06/01 22:16:08, piman (Very slow to review) wrote:
> Same here, do we need ShallowCopy, or can we just pass shared_memory_ back
> directly?

Changed the interface to pass a base::SharedMemory directly.

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
File content/renderer/pepper/ppb_image_data_impl.cc (right):

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
content/renderer/pepper/ppb_image_data_impl.cc:195: *handle =
base::SharedMemory::ShallowCopyHandle(dib_->handle());
On 2015/06/01 22:16:08, piman (Very slow to review) wrote:
> ditto.

We can't change the signature of this method without also changing the interface
of TransportDIB. TransportDIB is backed by a base::SharedMemory object, but that
object isn't exposed, only handles to the object. Furthermore, dib_->handle()
already implicitly makes a copy of the handle, so the additional call to
ShallowCopyHandle() doesn't accomplish much.

I've removed the ShallowCopyHandle(), and I'm going to talk to the OWNERs of
TransportDIB to see if I can change the interface, and if I succeed there, I
will also come back and update the code here.

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
File content/renderer/pepper/renderer_ppapi_host_impl.cc (right):

https://codereview.chromium.org/1154613006/diff/60001/content/renderer/pepper...
content/renderer/pepper/renderer_ppapi_host_impl.cc:241:
reinterpret_cast<HANDLE>(static_cast<intptr_t>(handle));
On 2015/06/01 22:16:08, piman (Very slow to review) wrote:
> ditto, no need for casts.

Done.

https://codereview.chromium.org/1154613006/diff/60001/ppapi/proxy/ppapi_proxy...
File ppapi/proxy/ppapi_proxy_test.cc (right):

https://codereview.chromium.org/1154613006/diff/60001/ppapi/proxy/ppapi_proxy...
ppapi/proxy/ppapi_proxy_test.cc:267:
reinterpret_cast<HANDLE>(static_cast<intptr_t>(handle)), remote_pid,
On 2015/06/01 22:16:08, piman (Very slow to review) wrote:
> ditto, no need for casts.

Done.

https://codereview.chromium.org/1154613006/diff/60001/ppapi/proxy/ppapi_proxy...
ppapi/proxy/ppapi_proxy_test.cc:515:
reinterpret_cast<HANDLE>(static_cast<intptr_t>(handle)), remote_pid,
On 2015/06/01 22:16:08, piman (Very slow to review) wrote:
> ditto

Done.

https://codereview.chromium.org/1154613006/diff/60001/ppapi/proxy/proxy_chann...
File ppapi/proxy/proxy_channel.cc (right):

https://codereview.chromium.org/1154613006/diff/60001/ppapi/proxy/proxy_chann...
ppapi/proxy/proxy_channel.cc:83:
reinterpret_cast<HANDLE>(static_cast<intptr_t>(handle)), false);
On 2015/06/01 22:16:08, piman (Very slow to review) wrote:
> ditto wrt casts

Done.

[erikchen, 2015-06-01 23:57:14]

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

[piman, 2015-06-02 00:00:15]

https://codereview.chromium.org/1154613006/diff/100001/content/renderer/peppe...
File content/renderer/pepper/audio_helper.cc (right):

https://codereview.chromium.org/1154613006/diff/100001/content/renderer/peppe...
content/renderer/pepper/audio_helper.cc:34: shm =
shared_memory_for_create_callback_.get();
That won't do anything. This only modifies a local variable...

You either need to make shm a base::SharedMemory** and assign *shm, or maybe
have GetSharedMemoryImpl return the SharedMemory*.

https://codereview.chromium.org/1154613006/diff/100001/content/renderer/peppe...
File content/renderer/pepper/ppb_buffer_impl.cc (right):

https://codereview.chromium.org/1154613006/diff/100001/content/renderer/peppe...
content/renderer/pepper/ppb_buffer_impl.cc:82: shm = shared_memory_.get();
Same here

[erikchen, 2015-06-02 00:01:47]

https://codereview.chromium.org/1154613006/diff/100001/content/renderer/peppe...
File content/renderer/pepper/audio_helper.cc (right):

https://codereview.chromium.org/1154613006/diff/100001/content/renderer/peppe...
content/renderer/pepper/audio_helper.cc:34: shm =
shared_memory_for_create_callback_.get();
On 2015/06/02 00:00:15, piman (Very slow to review) wrote:
> That won't do anything. This only modifies a local variable...
> 
> You either need to make shm a base::SharedMemory** and assign *shm, or maybe
> have GetSharedMemoryImpl return the SharedMemory*.

Uhh, not sure what portions of my brain failed to function there. Fixing now.

[commit-bot: I haz the power, 2015-06-02 00:06:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154613006/100001

[erikchen, 2015-06-02 00:10:07]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2015-06-02 00:12:24]

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

[commit-bot: I haz the power, 2015-06-02 00:18:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154613006/120001

[piman, 2015-06-02 00:22:31]

lgtm

[erikchen, 2015-06-02 00:28:38]

https://codereview.chromium.org/1154613006/diff/100001/content/renderer/peppe...
File content/renderer/pepper/ppb_buffer_impl.cc (right):

https://codereview.chromium.org/1154613006/diff/100001/content/renderer/peppe...
content/renderer/pepper/ppb_buffer_impl.cc:82: shm = shared_memory_.get();
On 2015/06/02 00:00:15, piman (Very slow to review) wrote:
> Same here

Fixed.

https://codereview.chromium.org/1154613006/diff/140001/content/ppapi_plugin/p...
File content/ppapi_plugin/ppapi_thread.cc (right):

https://codereview.chromium.org/1154613006/diff/140001/content/ppapi_plugin/p...
content/ppapi_plugin/ppapi_thread.cc:209:
reinterpret_cast<HANDLE>(static_cast<intptr_t>(handle));
Did another pass for nits. Removed an unnecessary reinterpret_cast<> as well as
a pointless local variable.

[erikchen, 2015-06-02 00:28:45]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-06-02 00:28:46]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1154613006/#ps160001
(title: "Another nits pass.")

[commit-bot: I haz the power, 2015-06-02 00:32:44]

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

[commit-bot: I haz the power, 2015-06-02 00:52:10]

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

[commit-bot: I haz the power, 2015-06-02 00:52:11]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[erikchen, 2015-06-02 00:55:34]

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

[erikchen, 2015-06-02 00:55:35]

bbudge: Looking for an OWNER review of  
chrome/renderer/pepper/*
ppapi/nacl_irt/*
ppapi/thunk/*

[bbudge, 2015-06-02 01:02:04]

chrome/renderer/pepper/*
ppapi/nacl_irt/*
ppapi/thunk/*

LGTM w/ a question

https://codereview.chromium.org/1154613006/diff/160001/ppapi/thunk/ppb_image_...
File ppapi/thunk/ppb_image_data_api.h (right):

https://codereview.chromium.org/1154613006/diff/160001/ppapi/thunk/ppb_image_...
ppapi/thunk/ppb_image_data_api.h:8: #include "base/memory/shared_memory.h"
Can you forward declare this as in ppb_audio_api.h?

[erikchen, 2015-06-02 01:05:25]

https://codereview.chromium.org/1154613006/diff/160001/ppapi/thunk/ppb_image_...
File ppapi/thunk/ppb_image_data_api.h (right):

https://codereview.chromium.org/1154613006/diff/160001/ppapi/thunk/ppb_image_...
ppapi/thunk/ppb_image_data_api.h:8: #include "base/memory/shared_memory.h"
On 2015/06/02 01:02:03, bbudge wrote:
> Can you forward declare this as in ppb_audio_api.h?

Not in a clean way. 

Right now, base::SharedMemoryHandle is typedefed to HANDLE (int) on windows, and
FileDescriptor on POSIX. It will soon become its own class on OSX.

[erikchen, 2015-06-02 01:07:20]

On 2015/06/02 01:05:25, erikchen wrote:
>
https://codereview.chromium.org/1154613006/diff/160001/ppapi/thunk/ppb_image_...
> File ppapi/thunk/ppb_image_data_api.h (right):
> 
>
https://codereview.chromium.org/1154613006/diff/160001/ppapi/thunk/ppb_image_...
> ppapi/thunk/ppb_image_data_api.h:8: #include "base/memory/shared_memory.h"
> On 2015/06/02 01:02:03, bbudge wrote:
> > Can you forward declare this as in ppb_audio_api.h?
> 
> Not in a clean way. 
> 
> Right now, base::SharedMemoryHandle is typedefed to HANDLE (int) on windows,
and
> FileDescriptor on POSIX. It will soon become its own class on OSX.

I already have a followup CL in progress that changes the type to
base::SharedMemory**, which will allow the class to be forward declared:
https://codereview.chromium.org/1145893009/

[erikchen, 2015-06-02 01:46:34]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-06-02 01:48:07]

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

[commit-bot: I haz the power, 2015-06-02 02:16:47]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-06-02 02:17:31]

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