New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 872778 link

Starred by 4 users
Status: Fixed
Owner:
mattcary@chromium.org
Closed: Sep 7
Cc:
rsesek@chromium.org
█ roc...@chromium.org
alexilin@chromium.org
mattcary@chromium.org
dcheng@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 797470



Sign in to add a comment

Add brokered mojo shared memory region create methods

Project Member Reported by mattcary@chromium.org, Aug 9 
mojo::SharedMemoryBuffer::Create is used for shared memory in processes which don't have the privileges to create shared memory region themselves. Under the hood, a broker is used in a privileged process to create a region which is then passed back to the caller.

There is no equivalent for this in the base::*SharedMemoryRegion creators. The proposal is to add mojo/public/cpp/base/shared_memory_utils.h containing the appropriate create methods.

The implementations will be variations on the following:

  mojo::ScopedSharedBufferHandle handle = 
     mojo::SharedBufferHandle::Create(size);
  if (!handle.is_valid()) {
    DLOG(ERROR) << ....
    return;
  }

  base::WritableSharedMemoryRegion region =
          mojo::UnwrapWritableSharedMemoryRegion(std::move(handle));
  if (!region.IsValid()) {
    DLOG(ERROR) << ....
    return;
  }

And then doing the appropriate conversion to Unsafe or ReadOnly as necessary.

Comment 1 by alexilin@chromium.org, Aug 17 

I'm a little bit concerned about the fact that the mojo::SharedBufferHandle::Create() method is synchronous but may require cross-process communication. Should we at least put a warning that these methods shouldn't be called on the main thread?

Comment 2 by roc...@chromium.org, Aug 17 

Renderers already do tons of sync IPC from the main thread. I'm not sure we
should advise otherwise for this.

Comment 3 by alexilin@chromium.org, Aug 17 

Oh, I didn't know that this is already a common practice, good to know.

Comment 4 by alexilin@chromium.org, Aug 22

Cc: mattcary@chromium.org rsesek@chromium.org
Issue 876525 has been merged into this issue.
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 22 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3db31de0beb45cfc047e28b67548775268270113

commit 3db31de0beb45cfc047e28b67548775268270113
Author: Matthew Cary <mattcary@chromium.org>
Date: Wed Aug 22 16:40:02 2018

Add mojo::Create*SharedMemoryRegions

This adds *SharedMemoryRegion create methods that parallel those in
base::. These use mojo::SharedMemoryBuffer::Create, which will use a
broker if appropriate to create shared memory in unprivileged processes.

Also update comments to clarify migration.

Bug:  872778 
Change-Id: Ia32da8181fd37d88a49a26e3e7e158612147646b
Reviewed-on: https://chromium-review.googlesource.com/1169602
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Matthew Cary <mattcary@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585068}
[modify] https://crrev.com/3db31de0beb45cfc047e28b67548775268270113/base/memory/read_only_shared_memory_region.h
[modify] https://crrev.com/3db31de0beb45cfc047e28b67548775268270113/base/memory/unsafe_shared_memory_region.h
[modify] https://crrev.com/3db31de0beb45cfc047e28b67548775268270113/base/memory/writable_shared_memory_region.h
[modify] https://crrev.com/3db31de0beb45cfc047e28b67548775268270113/mojo/public/cpp/base/BUILD.gn
[add] https://crrev.com/3db31de0beb45cfc047e28b67548775268270113/mojo/public/cpp/base/shared_memory_utils.cc
[add] https://crrev.com/3db31de0beb45cfc047e28b67548775268270113/mojo/public/cpp/base/shared_memory_utils.h
[modify] https://crrev.com/3db31de0beb45cfc047e28b67548775268270113/mojo/public/cpp/system/buffer.h

Comment 6 by m...@chromium.org, Sep 7

Status: Fixed (was: Started)
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 11 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b4f775157f2e456630092e39fb0b3df5e46dcaea

commit b4f775157f2e456630092e39fb0b3df5e46dcaea
Author: Matthew Cary <mattcary@chromium.org>
Date: Tue Sep 11 15:19:54 2018

Update CommandBufferProxyImpl to mojo shm creation

This changes CommandBufferProxyImpl::AllocateAndMapSharedMemory
to use the new mojo shared memory region API for creating the
unsafe shared memory region it uses.

Bug:  872778 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: Ia1b5380eb21afe54a05e81d26df8307819de23c0
Reviewed-on: https://chromium-review.googlesource.com/1171229
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: Alexandr Ilin <alexilin@chromium.org>
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Commit-Queue: Matthew Cary <mattcary@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590313}
[modify] https://crrev.com/b4f775157f2e456630092e39fb0b3df5e46dcaea/gpu/ipc/client/command_buffer_proxy_impl.cc
►

Sign in to add a comment