Add a GUID to base::SharedMemoryHandle.

Add a GUID to base::SharedMemoryHandle.

This allows SharedMemoryHandle to be tracked across processes.
SharedMemoryHandles that point to the same region should have the same GUID.
This CL does not finish all of the GUID plumbing. The following still needs to
be done:
  * Passing GUID through Mojo. rockot@ has said that he will do this and TODOs
  have been left for him in the code.
  * Passing GUID for base::FieldTrial, which requires a SharedMemoryHandle
  shortly after proess launch, before IPC is set up.
  * Updating ArcGpuVideoDecodeAccelerator to use mojo ScopedHandles instead of
  fds.
  * Updating serialization of gfx::GpuMemoryBufferHandle to pass through shared
  buffer handles instead of generic handles.

BUG=713763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Review-Url: https://codereview.chromium.org/2859843002
Cr-Commit-Position: refs/heads/master@{#469877}
Committed: https://chromium.googlesource.com/chromium/src/+/145252018a78e2102e148f238970fd411e10a706

[erikchen, 2017-05-04 01:58:20]

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

[commit-bot: I haz the power, 2017-05-04 01:58:42]

Dry run: 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, 2017-05-04 02:07:40]

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

[commit-bot: I haz the power, 2017-05-04 02:07:41]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[erikchen, 2017-05-04 02:48:25]

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

[commit-bot: I haz the power, 2017-05-04 02:48:46]

Dry run: 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, 2017-05-04 02:57:20]

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

[commit-bot: I haz the power, 2017-05-04 02:57:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2017-05-04 03:01:03]

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

[commit-bot: I haz the power, 2017-05-04 03:01:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2017-05-04 03:01:48]

Description was changed from

==========
Add a GUID to base::SharedMemoryHandle.

This allows SharedMemoryHandle to be tracked across processes.

BUG=
==========

to

==========
Add a GUID to base::SharedMemoryHandle.

This allows SharedMemoryHandle to be tracked across processes.

BUG=713763
==========

[commit-bot: I haz the power, 2017-05-04 03:12:48]

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

[commit-bot: I haz the power, 2017-05-04 03:12:49]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2017-05-04 03:23:23]

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

[commit-bot: I haz the power, 2017-05-04 03:23:38]

Dry run: 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, 2017-05-04 03:31:59]

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

[commit-bot: I haz the power, 2017-05-04 03:32:00]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[erikchen, 2017-05-04 03:37:25]

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

[commit-bot: I haz the power, 2017-05-04 03:37:38]

Dry run: 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, 2017-05-04 03:59:15]

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

[commit-bot: I haz the power, 2017-05-04 03:59:16]

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

[erikchen, 2017-05-04 17:11:41]

Description was changed from

==========
Add a GUID to base::SharedMemoryHandle.

This allows SharedMemoryHandle to be tracked across processes.

BUG=713763
==========

to

==========
Add a GUID to base::SharedMemoryHandle.

This allows SharedMemoryHandle to be tracked across processes.

BUG=713763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[erikchen, 2017-05-04 17:12:45]

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

[commit-bot: I haz the power, 2017-05-04 17:13:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2017-05-04 17:18:40]

Description was changed from

==========
Add a GUID to base::SharedMemoryHandle.

This allows SharedMemoryHandle to be tracked across processes.

BUG=713763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Add a GUID to base::SharedMemoryHandle.

This allows SharedMemoryHandle to be tracked across processes.
SharedMemoryHandles that point to the same region should have the same GUID.
This CL does not finish all of the GUID plumbing. The following still needs to
be done:
  * Passing GUID through Mojo. rockot@ has said that he will do this and TODOs
  have been left for him in the code.
  * Passing GUID for base::FieldTrial, which requires a SharedMemoryHandle
  shortly after proess launch, before IPC is set up.
  * Getting rid of base::SharedMemory::CreateNamedDeprecated and replacing it
  with standard IPC. It's used in exacty one place:
  CloudPrintServiceProcessMain, and should be replaceable by standard IPC.

BUG=713763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[erikchen, 2017-05-04 17:33:42]

erikchen@chromium.org changed reviewers:
+ rockot@chromium.org, thakis@chromium.org

[erikchen, 2017-05-04 17:33:44]

thakis, rockot: Please review.

[erikchen, 2017-05-04 17:34:05]

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

[erikchen, 2017-05-04 17:34:06]

wfh: Please review sandbox/win

[Nico, 2017-05-04 17:42:13]

Adding a GUID to SMH feels like SMH is now maybe handling too many things now.
Have you considered using composition instead? (`class CrossProcessSHM {
SharedMemoryHandle handle; GUID guid; }`). Or putting this in SharedMemory? I
don't know if that's feasible, but:

// SharedMemoryHandle is a platform specific type which represents
// the underlying OS handle to a shared memory segment.

That's not really true any more after this change.

https://codereview.chromium.org/2859843002/diff/140001/base/memory/shared_mem...
File base/memory/shared_memory_handle.h (right):

https://codereview.chromium.org/2859843002/diff/140001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:65: base::UnguessableToken GetGUID() const;
add comment saying what this is good for

https://codereview.chromium.org/2859843002/diff/140001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:81: // "consumes" the fd.
mention what the guid is for (can people who don't want to share their fd pass
some default empty guid?)

https://codereview.chromium.org/2859843002/diff/140001/base/memory/shared_mem...
File base/memory/shared_memory_handle_win.cc (right):

https://codereview.chromium.org/2859843002/diff/140001/base/memory/shared_mem...
base/memory/shared_memory_handle_win.cc:35: base::SharedMemoryHandle
handle(duped_handle, GetGUID());
So several handles can have the same guid? Document that in header somehow.

[Will Harris, 2017-05-04 17:50:51]

https://codereview.chromium.org/2859843002/diff/140001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/2859843002/diff/140001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:694: ParamTraits<base::UnguessableToken>::Write(m,
p.GetGUID());
should there be a DCHECK here that guid is not empty, to satisfy the comment in
unguessable_token.h "It is illegal to send empty UnguessableTokens across
processes"

[Ken Rockot(use gerrit already), 2017-05-04 17:54:49]

rockot@chromium.org changed reviewers:
- wfh@chromium.org

[Ken Rockot(use gerrit already), 2017-05-04 17:54:51]

https://codereview.chromium.org/2859843002/diff/140001/chrome/gpu/arc_gpu_vid...
File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2859843002/diff/140001/chrome/gpu/arc_gpu_vid...
chrome/gpu/arc_gpu_video_decode_accelerator.cc:299: // TODO(rockot): Pass GUIDs
through Mojo. https://crbug.com/713763.
not sure what Mojo has to do with this bit of code really. Looks like the FD
comes from explicitly duping the FD of another SharedMemoryhandle?

https://codereview.chromium.org/2859843002/diff/140001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/2859843002/diff/140001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:715: if (!ParamTraits<base::UnguessableToken>::Read(m,
iter, &guid))
We have templated helper functions for this in ipc_message_util.h. You should be
able to e.g.:

 GetParamSize(size, p.GetGUID())
 ReadParam(m, iter, &guid)
 WriteParam(m, p.GetGUID())

with no need for explicit template arguments.

https://codereview.chromium.org/2859843002/diff/140001/ui/gfx/mojo/buffer_typ...
File ui/gfx/mojo/buffer_types_struct_traits.cc (right):

https://codereview.chromium.org/2859843002/diff/140001/ui/gfx/mojo/buffer_typ...
ui/gfx/mojo/buffer_types_struct_traits.cc:132: base::UnguessableToken guid =
base::UnguessableToken::Create();
I don't think this is correct. Note that this deserialization path corresponds
to the serialization path at line 78. That is, these traits don't even serialize
shared memory on Linux using Mojo's shared buffer serialization path, but they
explicitly decompose the shared memory handle into a file descriptor and
serialize that. Mojo internals would have no way of knowing that there's
supposed to be a shm guid associated with the FD in this case, so if you want to
support this now you'd have to add an optional UnguessableToken field to
GpuMemoryBufferHandle which serves as a companion to shared_memory_handle when
used on Linux.

HOWEVER, it seems to me that what you should do instead is change the mojom to
use handle<shared_buffer> for the shared_memory_handle field (so that the
bindings only accept an explicit shared buffer handle rather than a generic Mojo
handle), and then change the Linux de/serialization path to use
WrapSharedMemoryHandle/UnwrapSharedMemoryHandle like Mac does.

I don't know why the code wasn't written that way in the first place, but maybe
the author can explain.

[commit-bot: I haz the power, 2017-05-04 17:56:35]

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

[commit-bot: I haz the power, 2017-05-04 17:56:36]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[erikchen, 2017-05-04 18:22:20]

On 2017/05/04 17:42:13, Nico wrote:
> Adding a GUID to SMH feels like SMH is now maybe handling too many things now.
> Have you considered using composition instead? (`class CrossProcessSHM {
> SharedMemoryHandle handle; GUID guid; }`). Or putting this in SharedMemory? I
> don't know if that's feasible, but:

Almost all SharedMemoryHandles are passed across processes - otherwise we
wouldn't need it to be shared memory. The primitive that is passed across IPC is
SharedMemoryHandle, so unless we want to touch every IPC message that used
shared memory, we need to change SharedMemoryHandle.

[erikchen, 2017-05-04 19:20:22]

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

[commit-bot: I haz the power, 2017-05-04 19:20:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2017-05-04 19:21:11]

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

[erikchen, 2017-05-04 19:21:13]

thakis, rockot, wfh: PTAL

https://codereview.chromium.org/2859843002/diff/140001/base/memory/shared_mem...
File base/memory/shared_memory_handle.h (right):

https://codereview.chromium.org/2859843002/diff/140001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:65: base::UnguessableToken GetGUID() const;
On 2017/05/04 17:42:13, Nico wrote:
> add comment saying what this is good for

Done.

https://codereview.chromium.org/2859843002/diff/140001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:81: // "consumes" the fd.
On 2017/05/04 17:42:13, Nico wrote:
> mention what the guid is for (can people who don't want to share their fd pass
> some default empty guid?)

Done.

https://codereview.chromium.org/2859843002/diff/140001/base/memory/shared_mem...
File base/memory/shared_memory_handle_win.cc (right):

https://codereview.chromium.org/2859843002/diff/140001/base/memory/shared_mem...
base/memory/shared_memory_handle_win.cc:35: base::SharedMemoryHandle
handle(duped_handle, GetGUID());
On 2017/05/04 17:42:13, Nico wrote:
> So several handles can have the same guid? Document that in header somehow.

Done.

https://codereview.chromium.org/2859843002/diff/140001/chrome/gpu/arc_gpu_vid...
File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2859843002/diff/140001/chrome/gpu/arc_gpu_vid...
chrome/gpu/arc_gpu_video_decode_accelerator.cc:299: // TODO(rockot): Pass GUIDs
through Mojo. https://crbug.com/713763.
On 2017/05/04 17:54:50, Ken Rockot wrote:
> not sure what Mojo has to do with this bit of code really. Looks like the FD
> comes from explicitly duping the FD of another SharedMemoryhandle?

This comes from:
GpuArcVideoService::BindSharedMemory
https://cs.chromium.org/chromium/src/chrome/gpu/gpu_arc_video_service.cc?type...

which gets the FD from a mojo handle: 
"""
base::ScopedFD fd = UnwrapFdFromMojoHandle(std::move(ashmem_handle));
"""

I've added a TODO to myself to pass through the mojo handle, but I've left the
TODO for you as well.

https://codereview.chromium.org/2859843002/diff/140001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/2859843002/diff/140001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:694: ParamTraits<base::UnguessableToken>::Write(m,
p.GetGUID());
On 2017/05/04 17:50:51, Will Harris wrote:
> should there be a DCHECK here that guid is not empty, to satisfy the comment
in
> unguessable_token.h "It is illegal to send empty UnguessableTokens across
> processes"

Yes, done.

https://codereview.chromium.org/2859843002/diff/140001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:715: if (!ParamTraits<base::UnguessableToken>::Read(m,
iter, &guid))
On 2017/05/04 17:54:50, Ken Rockot wrote:
> We have templated helper functions for this in ipc_message_util.h. You should
be
> able to e.g.:
> 
>  GetParamSize(size, p.GetGUID())
>  ReadParam(m, iter, &guid)
>  WriteParam(m, p.GetGUID())
> 
> with no need for explicit template arguments.

Done.

https://codereview.chromium.org/2859843002/diff/140001/ui/gfx/mojo/buffer_typ...
File ui/gfx/mojo/buffer_types_struct_traits.cc (right):

https://codereview.chromium.org/2859843002/diff/140001/ui/gfx/mojo/buffer_typ...
ui/gfx/mojo/buffer_types_struct_traits.cc:132: base::UnguessableToken guid =
base::UnguessableToken::Create();
On 2017/05/04 17:54:50, Ken Rockot wrote:
> I don't think this is correct. Note that this deserialization path corresponds
> to the serialization path at line 78. That is, these traits don't even
serialize
> shared memory on Linux using Mojo's shared buffer serialization path, but they
> explicitly decompose the shared memory handle into a file descriptor and
> serialize that. Mojo internals would have no way of knowing that there's
> supposed to be a shm guid associated with the FD in this case, so if you want
to
> support this now you'd have to add an optional UnguessableToken field to
> GpuMemoryBufferHandle which serves as a companion to shared_memory_handle when
> used on Linux.
> 
> HOWEVER, it seems to me that what you should do instead is change the mojom to
> use handle<shared_buffer> for the shared_memory_handle field (so that the
> bindings only accept an explicit shared buffer handle rather than a generic
Mojo
> handle), and then change the Linux de/serialization path to use
> WrapSharedMemoryHandle/UnwrapSharedMemoryHandle like Mac does.
> 
> I don't know why the code wasn't written that way in the first place, but
maybe
> the author can explain.

I've left a TODO for myself to update the serialization path, but left the TODO
for you as well.

[erikchen, 2017-05-04 19:23:28]

Description was changed from

==========
Add a GUID to base::SharedMemoryHandle.

This allows SharedMemoryHandle to be tracked across processes.
SharedMemoryHandles that point to the same region should have the same GUID.
This CL does not finish all of the GUID plumbing. The following still needs to
be done:
  * Passing GUID through Mojo. rockot@ has said that he will do this and TODOs
  have been left for him in the code.
  * Passing GUID for base::FieldTrial, which requires a SharedMemoryHandle
  shortly after proess launch, before IPC is set up.
  * Getting rid of base::SharedMemory::CreateNamedDeprecated and replacing it
  with standard IPC. It's used in exacty one place:
  CloudPrintServiceProcessMain, and should be replaceable by standard IPC.

BUG=713763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Add a GUID to base::SharedMemoryHandle.

This allows SharedMemoryHandle to be tracked across processes.
SharedMemoryHandles that point to the same region should have the same GUID.
This CL does not finish all of the GUID plumbing. The following still needs to
be done:
  * Passing GUID through Mojo. rockot@ has said that he will do this and TODOs
  have been left for him in the code.
  * Passing GUID for base::FieldTrial, which requires a SharedMemoryHandle
  shortly after proess launch, before IPC is set up.
  * Getting rid of base::SharedMemory::CreateNamedDeprecated and replacing it
  with standard IPC. It's used in exacty one place:
  CloudPrintServiceProcessMain, and should be replaceable by standard IPC.
  * Updating ArcGpuVideoDecodeAccelerator to use mojo ScopedHandles instead of
  fds.
  * Updating serialization of gfx::GpuMemoryBufferHandle to pass through shared
  buffer handles instead of generic handles.

BUG=713763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[Ken Rockot(use gerrit already), 2017-05-04 19:31:55]

lgtm

[Nico, 2017-05-04 19:32:04]

On Thu, May 4, 2017 at 2:22 PM, <erikchen@chromium.org> wrote:

> On 2017/05/04 17:42:13, Nico wrote:
> > Adding a GUID to SMH feels like SMH is now maybe handling too many
> things now.
> > Have you considered using composition instead? (`class CrossProcessSHM {
> > SharedMemoryHandle handle; GUID guid; }`). Or putting this in
> SharedMemory? I
> > don't know if that's feasible, but:
>
> Almost all SharedMemoryHandles are passed across processes - otherwise we
> wouldn't need it to be shared memory. The primitive that is passed across
> IPC is
> SharedMemoryHandle, so unless we want to touch every IPC message that used
> shared memory, we need to change SharedMemoryHandle.
>

What's the difference between SharedMemoryHandle and SharedMemory for you?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Will Harris, 2017-05-04 19:39:36]

lgtm on sandbox/win/src/process_mitigations_win32k_dispatcher.cc but other than
my comment above (that was addressed, thanks!) I'm happy to defer the review to
base/ owners

[erikchen, 2017-05-04 19:49:27]

On 2017/05/04 19:32:04, Nico wrote:
> On Thu, May 4, 2017 at 2:22 PM, <mailto:erikchen@chromium.org> wrote:
> 
> > On 2017/05/04 17:42:13, Nico wrote:
> > > Adding a GUID to SMH feels like SMH is now maybe handling too many
> > things now.
> > > Have you considered using composition instead? (`class CrossProcessSHM {
> > > SharedMemoryHandle handle; GUID guid; }`). Or putting this in
> > SharedMemory? I
> > > don't know if that's feasible, but:
> >
> > Almost all SharedMemoryHandles are passed across processes - otherwise we
> > wouldn't need it to be shared memory. The primitive that is passed across
> > IPC is
> > SharedMemoryHandle, so unless we want to touch every IPC message that used
> > shared memory, we need to change SharedMemoryHandle.
> >
> 
> What's the difference between SharedMemoryHandle and SharedMemory for you?

Are you asking about a platonic ideal? In an ideal world, we probably wouldn't
expose SharedMemoryHandle in the public API of SharedMemory. A process might
create some SharedMemory, map it multiple times into its own address space, and
then pass the SharedMemory to another process and/or keep a copy to itself.

In the actual Chrome code-base, we have a large number of IPCs that pass a
SharedMemoryHandle type, which is passed by copy everywhere. We also have a
large number of consumers that use public methods on base::SharedMemory, which
is not copyable. So here's my vision of the distinction:

SharedMemoryHandle is the smallest possible IPC-transportable "reference" to a
shared memory OS resource. A "reference" can be consumed exactly once [by
base::SharedMemory] to map the shared memory OS resource into the virtual
address space of the current process.
base::SharedMemory represents a mapped shared memory OS resource.

[Nico, 2017-05-04 19:51:18]

On Thu, May 4, 2017 at 3:49 PM, <erikchen@chromium.org> wrote:

> On 2017/05/04 19:32:04, Nico wrote:
> > On Thu, May 4, 2017 at 2:22 PM, <mailto:erikchen@chromium.org> wrote:
> >
> > > On 2017/05/04 17:42:13, Nico wrote:
> > > > Adding a GUID to SMH feels like SMH is now maybe handling too many
> > > things now.
> > > > Have you considered using composition instead? (`class
> CrossProcessSHM {
> > > > SharedMemoryHandle handle; GUID guid; }`). Or putting this in
> > > SharedMemory? I
> > > > don't know if that's feasible, but:
> > >
> > > Almost all SharedMemoryHandles are passed across processes - otherwise
> we
> > > wouldn't need it to be shared memory. The primitive that is passed
> across
> > > IPC is
> > > SharedMemoryHandle, so unless we want to touch every IPC message that
> used
> > > shared memory, we need to change SharedMemoryHandle.
> > >
> >
> > What's the difference between SharedMemoryHandle and SharedMemory for
> you?
>
> Are you asking about a platonic ideal? In an ideal world, we probably
> wouldn't
> expose SharedMemoryHandle in the public API of SharedMemory. A process
> might
> create some SharedMemory, map it multiple times into its own address
> space, and
> then pass the SharedMemory to another process and/or keep a copy to itself.
>
> In the actual Chrome code-base, we have a large number of IPCs that pass a
> SharedMemoryHandle type, which is passed by copy everywhere. We also have a
> large number of consumers that use public methods on base::SharedMemory,
> which
> is not copyable. So here's my vision of the distinction:
>
> SharedMemoryHandle is the smallest possible IPC-transportable "reference"
> to a
> shared memory OS resource. A "reference" can be consumed exactly once [by
> base::SharedMemory] to map the shared memory OS resource into the virtual
> address space of the current process.
> base::SharedMemory represents a mapped shared memory OS resource.
>

Ok, that sounds reasonable to me. Could you update the class comments of
SharedMemory and SharedMemoryHandle to say something like that?


>
> https://codereview.chromium.org/2859843002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[erikchen, 2017-05-04 20:01:35]

> 
> Ok, that sounds reasonable to me. Could you update the class comments of
> SharedMemory and SharedMemoryHandle to say something like that?
done

[erikchen, 2017-05-04 20:02:30]

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

[commit-bot: I haz the power, 2017-05-04 20:03:05]

Dry run: 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, 2017-05-04 20:43:20]

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

[commit-bot: I haz the power, 2017-05-04 20:43:21]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[erikchen, 2017-05-04 20:53:12]

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

[commit-bot: I haz the power, 2017-05-04 20:53:38]

Dry run: 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, 2017-05-04 22:33:48]

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

[commit-bot: I haz the power, 2017-05-04 22:33:49]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[erikchen, 2017-05-04 23:26:28]

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

[commit-bot: I haz the power, 2017-05-04 23:27:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2017-05-04 23:28:06]

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

[erikchen, 2017-05-04 23:28:07]

bbudge: Please review ppapi/

[erikchen, 2017-05-04 23:42:11]

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

[commit-bot: I haz the power, 2017-05-04 23:43:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bbudge, 2017-05-05 00:35:39]

ppapi lgtm w/comment

https://codereview.chromium.org/2859843002/diff/300001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/2859843002/diff/300001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:624: // nacl_ipc_adapater.cc:WriteHandle().
This comment seems like it belongs on the SharedMemoryHandle specialization.

[commit-bot: I haz the power, 2017-05-05 00:49:25]

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

[commit-bot: I haz the power, 2017-05-05 00:49:26]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[erikchen, 2017-05-05 00:57:00]

https://codereview.chromium.org/2859843002/diff/300001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/2859843002/diff/300001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:624: // nacl_ipc_adapater.cc:WriteHandle().
On 2017/05/05 00:35:39, bbudge wrote:
> This comment seems like it belongs on the SharedMemoryHandle specialization.

base::FileDescriptor *also* has to be kept in sync with the method in question
for

[erikchen, 2017-05-05 01:03:45]

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

[commit-bot: I haz the power, 2017-05-05 01:04:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2017-05-05 01:33:20]

Description was changed from

==========
Add a GUID to base::SharedMemoryHandle.

This allows SharedMemoryHandle to be tracked across processes.
SharedMemoryHandles that point to the same region should have the same GUID.
This CL does not finish all of the GUID plumbing. The following still needs to
be done:
  * Passing GUID through Mojo. rockot@ has said that he will do this and TODOs
  have been left for him in the code.
  * Passing GUID for base::FieldTrial, which requires a SharedMemoryHandle
  shortly after proess launch, before IPC is set up.
  * Getting rid of base::SharedMemory::CreateNamedDeprecated and replacing it
  with standard IPC. It's used in exacty one place:
  CloudPrintServiceProcessMain, and should be replaceable by standard IPC.
  * Updating ArcGpuVideoDecodeAccelerator to use mojo ScopedHandles instead of
  fds.
  * Updating serialization of gfx::GpuMemoryBufferHandle to pass through shared
  buffer handles instead of generic handles.

BUG=713763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Add a GUID to base::SharedMemoryHandle.

This allows SharedMemoryHandle to be tracked across processes.
SharedMemoryHandles that point to the same region should have the same GUID.
This CL does not finish all of the GUID plumbing. The following still needs to
be done:
  * Passing GUID through Mojo. rockot@ has said that he will do this and TODOs
  have been left for him in the code.
  * Passing GUID for base::FieldTrial, which requires a SharedMemoryHandle
  shortly after proess launch, before IPC is set up.
  * Updating ArcGpuVideoDecodeAccelerator to use mojo ScopedHandles instead of
  fds.
  * Updating serialization of gfx::GpuMemoryBufferHandle to pass through shared
  buffer handles instead of generic handles.

BUG=713763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[erikchen, 2017-05-05 01:36:41]

On 2017/05/05 00:57:00, erikchen wrote:
>
https://codereview.chromium.org/2859843002/diff/300001/ipc/ipc_message_utils.cc
> File ipc/ipc_message_utils.cc (right):
> 
>
https://codereview.chromium.org/2859843002/diff/300001/ipc/ipc_message_utils....
> ipc/ipc_message_utils.cc:624: // nacl_ipc_adapater.cc:WriteHandle().
> On 2017/05/05 00:35:39, bbudge wrote:
> > This comment seems like it belongs on the SharedMemoryHandle specialization.
> 
> base::FileDescriptor *also* has to be kept in sync with the method in question
> for

thakis: I updated the CL description to remove the line about getting rid of
CreateNamedDeprecated. I looked into it - it's currently used to communicate
between two different versions of the same service [cloud print] very early in
their life cycle. I don't see any way to do this without a very large refactor.
The other alternative would be to rip out named shared memory from
base::SharedMemory and have a small, custom SharedMemory implementation used
only by Cloud Print. Either way, I don't think it's high priority, since we
can't keep track of shared memory across different versions of a service to
begin with.

[commit-bot: I haz the power, 2017-05-05 03:02:52]

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

[commit-bot: I haz the power, 2017-05-05 03:02:54]

Dry run: This issue passed the CQ dry run.

[Nico, 2017-05-05 18:34:37]

gltm

https://codereview.chromium.org/2859843002/diff/320001/base/memory/shared_mem...
File base/memory/shared_memory_mac_unittest.cc (right):

https://codereview.chromium.org/2859843002/diff/320001/base/memory/shared_mem...
base/memory/shared_memory_mac_unittest.cc:365: // Tests that the read-only flag
works.
do you want to test somewhere in here that the rw and the read-only shm have the
same guid?

https://codereview.chromium.org/2859843002/diff/320001/base/memory/shared_mem...
File base/memory/shared_memory_posix.cc (right):

https://codereview.chromium.org/2859843002/diff/320001/base/memory/shared_mem...
base/memory/shared_memory_posix.cc:218:
base::FileDescriptor(readonly_mapped_file, false), shm_.GetGUID());
For posix too I suppose, not just mac.

https://codereview.chromium.org/2859843002/diff/320001/base/memory/shared_mem...
base/memory/shared_memory_posix.cc:257: // region. Instead, we should just
remove all instances of this.
Your last comment sounds like you no longer think this is easily possible, maybe
update this comment. (Nice example of "deprecation comments don't work" too.)

https://codereview.chromium.org/2859843002/diff/320001/base/memory/shared_mem...
File base/memory/shared_memory_win.cc (right):

https://codereview.chromium.org/2859843002/diff/320001/base/memory/shared_mem...
base/memory/shared_memory_win.cc:287: // region. Instead, we should just remove
all instances of this.
Likewise?

What's the failure case if someone calls this function? Just shm memory being
double-counted? Is there anything that prevents new callers of this? Are you
going to work on removing calls to this function – sounds like this isn't really
possible?

[Nico, 2017-05-05 18:36:24]

> gltm

lgtm

[erikchen, 2017-05-05 19:29:33]

https://codereview.chromium.org/2859843002/diff/320001/base/memory/shared_mem...
File base/memory/shared_memory_mac_unittest.cc (right):

https://codereview.chromium.org/2859843002/diff/320001/base/memory/shared_mem...
base/memory/shared_memory_mac_unittest.cc:365: // Tests that the read-only flag
works.
On 2017/05/05 18:34:36, Nico wrote:
> do you want to test somewhere in here that the rw and the read-only shm have
the
> same guid?

Done.

https://codereview.chromium.org/2859843002/diff/320001/base/memory/shared_mem...
File base/memory/shared_memory_posix.cc (right):

https://codereview.chromium.org/2859843002/diff/320001/base/memory/shared_mem...
base/memory/shared_memory_posix.cc:218:
base::FileDescriptor(readonly_mapped_file, false), shm_.GetGUID());
On 2017/05/05 18:34:36, Nico wrote:
> For posix too I suppose, not just mac.

Done.

https://codereview.chromium.org/2859843002/diff/320001/base/memory/shared_mem...
base/memory/shared_memory_posix.cc:257: // region. Instead, we should just
remove all instances of this.
On 2017/05/05 18:34:36, Nico wrote:
> Your last comment sounds like you no longer think this is easily possible,
maybe
> update this comment. (Nice example of "deprecation comments don't work" too.)

Done.

https://codereview.chromium.org/2859843002/diff/320001/base/memory/shared_mem...
File base/memory/shared_memory_win.cc (right):

https://codereview.chromium.org/2859843002/diff/320001/base/memory/shared_mem...
base/memory/shared_memory_win.cc:287: // region. Instead, we should just remove
all instances of this.
On 2017/05/05 18:34:36, Nico wrote:
> Likewise?
> 
> What's the failure case if someone calls this function? Just shm memory being
> double-counted? Is there anything that prevents new callers of this? Are you
> going to work on removing calls to this function – sounds like this isn't
really
> possible?

Done.

[erikchen, 2017-05-05 19:29:38]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-05-05 19:29:39]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
wfh@chromium.org, bbudge@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2859843002/#ps340001
(title: "comments form thakis and fix bootstrap.py")

[commit-bot: I haz the power, 2017-05-05 19:30:24]

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, 2017-05-05 22:29:26]

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

[commit-bot: I haz the power, 2017-05-05 22:29:27]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[erikchen, 2017-05-05 22:39:39]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-05-05 22:39:40]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
bbudge@chromium.org, thakis@chromium.org, wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/2859843002/#ps360001
(title: "fix guid on android.")

[commit-bot: I haz the power, 2017-05-05 22:41:07]

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, 2017-05-05 23:40:13]

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

[commit-bot: I haz the power, 2017-05-05 23:40:14]

Try jobs failed on following builders:
  win10_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win10_chromium_x6...)

[erikchen, 2017-05-06 00:17:08]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2017-05-06 00:17:43]

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, 2017-05-06 19:16:54]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1494029828565790,
"parent_rev": "469d0548dfb4c3304c03571ba3f0f1a9bc58e2a4", "commit_rev":
"145252018a78e2102e148f238970fd411e10a706"}

[commit-bot: I haz the power, 2017-05-06 19:17:03]

Description was changed from

==========
Add a GUID to base::SharedMemoryHandle.

This allows SharedMemoryHandle to be tracked across processes.
SharedMemoryHandles that point to the same region should have the same GUID.
This CL does not finish all of the GUID plumbing. The following still needs to
be done:
  * Passing GUID through Mojo. rockot@ has said that he will do this and TODOs
  have been left for him in the code.
  * Passing GUID for base::FieldTrial, which requires a SharedMemoryHandle
  shortly after proess launch, before IPC is set up.
  * Updating ArcGpuVideoDecodeAccelerator to use mojo ScopedHandles instead of
  fds.
  * Updating serialization of gfx::GpuMemoryBufferHandle to pass through shared
  buffer handles instead of generic handles.

BUG=713763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Add a GUID to base::SharedMemoryHandle.

This allows SharedMemoryHandle to be tracked across processes.
SharedMemoryHandles that point to the same region should have the same GUID.
This CL does not finish all of the GUID plumbing. The following still needs to
be done:
  * Passing GUID through Mojo. rockot@ has said that he will do this and TODOs
  have been left for him in the code.
  * Passing GUID for base::FieldTrial, which requires a SharedMemoryHandle
  shortly after proess launch, before IPC is set up.
  * Updating ArcGpuVideoDecodeAccelerator to use mojo ScopedHandles instead of
  fds.
  * Updating serialization of gfx::GpuMemoryBufferHandle to pass through shared
  buffer handles instead of generic handles.

BUG=713763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Review-Url: https://codereview.chromium.org/2859843002
Cr-Commit-Position: refs/heads/master@{#469877}
Committed:
https://chromium.googlesource.com/chromium/src/+/145252018a78e2102e148f238970...
==========

[commit-bot: I haz the power, 2017-05-06 19:17:05]

Committed patchset #19 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/145252018a78e2102e148f238970...