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

Issue 713763 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug
Hotlist-MemoryInfra

Blocking:
issue 604726



Sign in to add a comment

Add a GUID to base::SharedMemory.

Project Member Reported by erikc...@chromium.org, Apr 20 2017

Issue description

Memory-UMA needs to be able to dedupe base::SharedMemory regions across multiple processes. The current implementation uses POSIX sys-calls to get the inode's st_dev and st_ino. This doesn't scale to macOS, and requires some finessing to work on Windows.

The easiest platform-agnostic solution is to add a GUID to base::SharedMemory and base::SharedMemoryHandle and pass that GUID over IPC. I recommend a 128 bit GUID. [birthday paradox and some math shows we want more than 64-bits]. 

There's an email thread where Hajime and Ken were talking about changing MojoPlatformhandle to support this. We shouldn't be changing Mojo internals to support an application level change. When we serialize to Mojo, we should include both a platform handle and a GUID. I haven't looked in detail at Mojo serialization of base::SharedMemoryHandle but Chrome IPC serialization is pretty trivial to change:
https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?type=cs&q=sharedmemoryhandle+file:ipc&l=682


 

Comment 1 by danakj@chromium.org, Apr 20 2017

> commend a 128 bit GUID. [birthday paradox and some math shows we want more than 64-bits]. 

Sounds like https://cs.chromium.org/chromium/src/base/unguessable_token.h?rcl=4e92e8c0cf7929215a8381d75e9a08cc3862102b&l=21

Comment 2 by roc...@chromium.org, Apr 20 2017

We can talk more about this tomorrow but adding the UUID to SMH and
changing only the legacy IPC serialization is definitely not a robust
solution.
I spoke with rockot@, who said that we should be making a change to Mojo internals to support adding a GUID to shared memory handle, and that this is dependent on Mojo versioning, which is 6-8 weeks out.

rockot: Is there a bug we can block on?

Comment 4 by roc...@chromium.org, Apr 21 2017

696107 would be good.
Blockedon: 696107
Here are the steps needed to make this happen:

1) Rip out existing logic for base::SharedMemory::UniqueId w.r.t. SharedMemoryTracker.
2) Modify SharedMemoryHandle to be a struct on POSIX.
3) Add the field base::UnguessableToken to SharedMemoryHandle on all platforms.
  3a) Update constructor so that a new token is generated by default for all SharedMemoryhandles.
  3b) Make sure that Chrome IPC supports serialization/deserialization.
  3c) Make sure that Mojo IPC supports serialization/deserialization [blocked on 696107]
4) Modify SharedMemoryTracker to support base::UnguessableToken
+1. Shall I work on these steps?

For 1), to ripping out the current logic, I think SharedMemoryHandles need to have unique ids. So I guess 1) can be done after 4). Is this correct? 

For 2), I think just making SharedMemoryHandle a struct (without IDs) can be done anytime.

As discussed with erikchen@ on hungout, I'll take the tasks 1) and 4) since they are related to SharedMemoryTracker, and Erik will take the others. Thank you.
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 28 2017

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

commit 22a813bccaeb05fc47689b540701e6d590fa7648
Author: erikchen <erikchen@chromium.org>
Date: Fri Apr 28 17:10:50 2017

Make base::SharedMemoryHandle a class on POSIX.

This CL is *mostly* a refactor with no behavioral changes.

SharedMemoryHandle is already a struct on Windows and macOS. Making it a struct
on all platforms allows the introduction of a GUID field.

This CL includes two small fixes:
Previously, the implementation of SharedMemory on POSIX was inconsistent on the
definition of a valid handle. Sometimes it would check fd > 0, sometimes it
would check fd >= 0, and sometimes it would check fd != -1. This CL standardizes
on fd >= 0.

The previous implementation of SharedMemory on NaCl would leak a fd if it
failed to copy the fd in ShareToProcessCommon.

BUG= 713763 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2843113002
Cr-Commit-Position: refs/heads/master@{#468035}

[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/base/BUILD.gn
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/base/memory/discardable_shared_memory.cc
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/base/memory/shared_memory.h
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/base/memory/shared_memory_android.cc
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/base/memory/shared_memory_handle.h
[add] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/base/memory/shared_memory_handle_posix.cc
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/base/memory/shared_memory_nacl.cc
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/base/memory/shared_memory_posix.cc
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/base/memory/shared_memory_unittest.cc
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/base/metrics/field_trial.cc
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/chrome/gpu/arc_gpu_video_decode_accelerator.cc
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/components/exo/wayland/clients/client_base.cc
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/components/exo/wayland/server.cc
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/components/printing/renderer/print_web_view_helper_linux.cc
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/content/browser/renderer_host/sandbox_ipc_linux.cc
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/content/child/blob_storage/blob_transport_controller_unittest.cc
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/ipc/ipc_message_utils.cc
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/ipc/ipc_message_utils.h
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/media/base/video_frame.cc
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/media/gpu/ipc/client/gpu_jpeg_decode_accelerator_host.cc
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/mojo/edk/embedder/platform_shared_buffer.cc
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/mojo/edk/system/platform_wrapper_unittest.cc
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/mojo/public/cpp/system/platform_handle.cc
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/ui/gfx/mojo/buffer_types_struct_traits.cc
[modify] https://crrev.com/22a813bccaeb05fc47689b540701e6d590fa7648/ui/ozone/platform/wayland/wayland_surface_factory.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Apr 28 2017

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

commit d2bc65e52a5d63520f974b98c5efa863b116f4a0
Author: erikchen <erikchen@chromium.org>
Date: Fri Apr 28 17:19:56 2017

Used a single class for SharedMemoryHandle.

This CL is a refactor with no intended behavior change.

Now that the class is defined on all platforms, make it a single class rather
than 3 separate classes. This allows the removal of some duplicate code in the
public interface.

BUG= 713763 

Review-Url: https://codereview.chromium.org/2846893003
Cr-Commit-Position: refs/heads/master@{#468039}

[modify] https://crrev.com/d2bc65e52a5d63520f974b98c5efa863b116f4a0/base/memory/shared_memory_handle.h
[modify] https://crrev.com/d2bc65e52a5d63520f974b98c5efa863b116f4a0/base/memory/shared_memory_handle_posix.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Apr 29 2017

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

commit d804e105f1e27e1ffafb7ac454d445188d6f602c
Author: erikchen <erikchen@chromium.org>
Date: Sat Apr 29 02:24:36 2017

Make PlatformFileForTransit its own class on Windows.

This CL is *mostly* a refactor with no intended behavior change.

Previously, PlatformFileForTransit had the same semantics as SharedMemoryHandle,
so it was typedef-ed to SharedMemoryHandle. However, the semantics for
SharedMemoryHandle are changing, so it's no longer appropriate to do so.

This CL includes a small fix to ppb_nacl_private_impl.cc, which previously
leaked a HANDLE on Windows, but not on an fd on POSIX.

The leak was introduced by "Make PlatformFileForTransit a class on Windows."
[commit: 19e5f6905203ecd6adbbc4bfe57e086eb61028c6]. Prior to that CL:
  On Windows, NaCl would duplicate the handle into the destination process
  [closing the original handle in the process], and then pass the result as a
  raw int.

  On POSIX, NaCl would duplicate the handle using Chrome IPC, using
  base::FileDescriptor.auto_close = true, which would cause the IPC subsystem
  to close the handle.

After that CL:
  On Windows, NaCl would let Chrome IPC transport the handle, but failed to
  call PlatformFileForTransit::SetOwnershipPassesToIPC(true) [the equivalent of
  base::FileDescriptor.auto_close]. This caused a leak of the handle in the
  browser process.

In this CL:
  PlatformFileForTransit is now its own class, with no auto_close parameter
  because the IPC subsystem always closes the handle in the source process.

BUG= 713763 

Review-Url: https://codereview.chromium.org/2846293002
Cr-Commit-Position: refs/heads/master@{#468213}

[modify] https://crrev.com/d804e105f1e27e1ffafb7ac454d445188d6f602c/components/nacl/renderer/ppb_nacl_private_impl.cc
[modify] https://crrev.com/d804e105f1e27e1ffafb7ac454d445188d6f602c/ipc/ipc_message_utils.cc
[modify] https://crrev.com/d804e105f1e27e1ffafb7ac454d445188d6f602c/ipc/ipc_message_utils.h
[modify] https://crrev.com/d804e105f1e27e1ffafb7ac454d445188d6f602c/ipc/ipc_platform_file.cc
[modify] https://crrev.com/d804e105f1e27e1ffafb7ac454d445188d6f602c/ipc/ipc_platform_file.h

Project Member

Comment 12 by bugdroid1@chromium.org, Apr 29 2017

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

commit a40775d038de9a6a75db3965104abe56b77c9ed6
Author: erikchen <erikchen@chromium.org>
Date: Sat Apr 29 07:15:49 2017

Get rid of base::SharedMemory::NULLHandle();

This CL is a refactor and has no intended behavior change.

NullHandle() has two use cases:
  * Create an invalid handle.
  * Return an object that can be compared against a handle to check validity.
The former is also the behavior of the default constructor of
base::SharedMemoryHandle, and the latter should be done with the member
IsValid().

Fixing the latter also allows the removal of operator== and operator!=.

BUG= 713763 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2849633002
Cr-Commit-Position: refs/heads/master@{#468231}

[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/base/memory/shared_memory.h
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/base/memory/shared_memory_handle.h
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/base/memory/shared_memory_handle_mac.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/base/memory/shared_memory_handle_win.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/base/memory/shared_memory_mac.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/base/memory/shared_memory_nacl.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/base/memory/shared_memory_posix.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/base/memory/shared_memory_win.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/components/discardable_memory/service/discardable_shared_memory_manager.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/components/nacl/common/nacl_types.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/components/nacl/renderer/nexe_load_manager.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/content/browser/media/capture/web_contents_video_capture_device_unittest.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/content/browser/renderer_host/clipboard_message_filter_unittest.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/content/browser/renderer_host/render_message_filter.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/content/renderer/media/audio_message_filter_unittest.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/content/renderer/pepper/mock_renderer_ppapi_host.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/content/renderer/pepper/pepper_audio_input_host.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/content/renderer/pepper/pepper_audio_output_host.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/content/renderer/renderer_blink_platform_impl.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/gpu/ipc/client/gpu_channel_host.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/media/base/video_frame.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/media/capture/content/thread_safe_capture_oracle.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/mojo/edk/embedder/embedder.h
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/ppapi/nacl_irt/ppapi_dispatcher.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/ppapi/proxy/gamepad_resource.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/ppapi/proxy/media_stream_track_resource_base.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/ppapi/proxy/plugin_array_buffer_var.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/ppapi/proxy/ppb_audio_proxy.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/ppapi/proxy/ppb_graphics_3d_proxy.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/ppapi/proxy/proxy_channel.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/ppapi/proxy/serialized_handle.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/ppapi/proxy/serialized_handle.h
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/ppapi/proxy/video_decoder_resource.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/ui/gfx/gpu_memory_buffer.cc
[modify] https://crrev.com/a40775d038de9a6a75db3965104abe56b77c9ed6/ui/surface/transport_dib.h

Cc: etienneb@chromium.org
Project Member

Comment 14 by bugdroid1@chromium.org, May 3 2017

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

commit 3df1dd51204370d6289ce46252d3c416c272447e
Author: erikchen <erikchen@chromium.org>
Date: Wed May 03 22:53:40 2017

Use SharedMemoryHandle instead ScopedHandle as ivar for SharedMemory

Only Windows was using ScopedHandle. Other platforms already used a
SharedMemoryHandle directly. The theoretical benefit of ScopedHandle is that it
correctly deals with ownership. But all that does is ensure that the handle is
closed during destruction of SharedMemory, which is already explicitly coded,
since it's needed on all platforms. Switching to SharedMemoryHandle is needed to
correctly propagate other state on SharedMemoryHandle, which is being added in
the near future.

BUG= 713763 

Review-Url: https://codereview.chromium.org/2854833004
Cr-Commit-Position: refs/heads/master@{#469179}

[modify] https://crrev.com/3df1dd51204370d6289ce46252d3c416c272447e/base/memory/shared_memory.h
[modify] https://crrev.com/3df1dd51204370d6289ce46252d3c416c272447e/base/memory/shared_memory_handle.h
[modify] https://crrev.com/3df1dd51204370d6289ce46252d3c416c272447e/base/memory/shared_memory_mac.cc
[modify] https://crrev.com/3df1dd51204370d6289ce46252d3c416c272447e/base/memory/shared_memory_posix.cc
[modify] https://crrev.com/3df1dd51204370d6289ce46252d3c416c272447e/base/memory/shared_memory_win.cc

Project Member

Comment 15 by bugdroid1@chromium.org, May 6 2017

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

commit 145252018a78e2102e148f238970fd411e10a706
Author: erikchen <erikchen@chromium.org>
Date: Sat May 06 19:16:51 2017

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}

[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/base/BUILD.gn
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/base/memory/shared_memory.h
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/base/memory/shared_memory_android.cc
[add] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/base/memory/shared_memory_handle.cc
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/base/memory/shared_memory_handle.h
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/base/memory/shared_memory_handle_mac.cc
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/base/memory/shared_memory_handle_posix.cc
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/base/memory/shared_memory_handle_win.cc
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/base/memory/shared_memory_mac.cc
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/base/memory/shared_memory_mac_unittest.cc
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/base/memory/shared_memory_posix.cc
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/base/memory/shared_memory_unittest.cc
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/base/memory/shared_memory_win.cc
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/base/metrics/field_trial.cc
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/chrome/gpu/arc_gpu_video_decode_accelerator.cc
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/ipc/ipc_message_utils.cc
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/mojo/edk/embedder/platform_shared_buffer.cc
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/mojo/edk/system/platform_wrapper_unittest.cc
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/mojo/public/cpp/system/platform_handle.cc
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/ppapi/proxy/nacl_message_scanner.cc
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/sandbox/win/src/process_mitigations_win32k_dispatcher.cc
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/sandbox/win/tests/common/controller.cc
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/tools/gn/bootstrap/bootstrap.py
[modify] https://crrev.com/145252018a78e2102e148f238970fd411e10a706/ui/gfx/mojo/buffer_types_struct_traits.cc

Project Member

Comment 16 by bugdroid1@chromium.org, May 10 2017

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

commit 2525d998c7c50bce5ca17fb934c2aeb8e27e1db0
Author: erikchen <erikchen@chromium.org>
Date: Wed May 10 20:46:22 2017

Pass the GUID for the SharedMemoryHandle used by base::FieldTrialList.

SharedMemoryHandle now has a GUID used to track shared memory usage across
processes. The SharedMemoryHandle used by base::FieldTrialList is passed
manually by command line flag and Process creation options. This CL appends the
GUID to the command line flag.

BUG= 713763 

Review-Url: https://codereview.chromium.org/2862123002
Cr-Commit-Position: refs/heads/master@{#470700}

[modify] https://crrev.com/2525d998c7c50bce5ca17fb934c2aeb8e27e1db0/base/metrics/field_trial.cc
[modify] https://crrev.com/2525d998c7c50bce5ca17fb934c2aeb8e27e1db0/base/metrics/field_trial.h
[modify] https://crrev.com/2525d998c7c50bce5ca17fb934c2aeb8e27e1db0/base/metrics/field_trial_unittest.cc
[modify] https://crrev.com/2525d998c7c50bce5ca17fb934c2aeb8e27e1db0/content/browser/child_process_launcher_helper_posix.cc

Project Member

Comment 17 by bugdroid1@chromium.org, May 18 2017

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

commit 9d6afd71a4937e9f400609f839c2b39b22c4c497
Author: erikchen <erikchen@chromium.org>
Date: Thu May 18 17:49:06 2017

Add a size parameter to SharedMemoryHandle.

This is needed to construct a Mojo shared buffer from a SharedMemoryHandle,
which in turn is needed to pass GUIDs through Mojo. Previously, classes like
gfx::GpuMemoryBufferHandle were being serialized as a file descriptor or Windows
HANDLE, rather than a SharedMemoryHandle because they did not have access to a
size field.

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

Review-Url: https://codereview.chromium.org/2875453002
Cr-Commit-Position: refs/heads/master@{#472865}

[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/base/memory/shared_memory.h
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/base/memory/shared_memory_android.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/base/memory/shared_memory_handle.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/base/memory/shared_memory_handle.h
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/base/memory/shared_memory_handle_mac.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/base/memory/shared_memory_handle_posix.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/base/memory/shared_memory_handle_win.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/base/memory/shared_memory_mac.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/base/memory/shared_memory_posix.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/base/memory/shared_memory_unittest.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/base/memory/shared_memory_win.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/base/metrics/field_trial.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/chrome/gpu/arc_gpu_video_decode_accelerator.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/components/exo/wayland/server.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/ipc/ipc_message_utils.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/ipc/ipc_message_utils_unittest.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/mojo/edk/embedder/platform_shared_buffer.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/mojo/edk/system/platform_wrapper_unittest.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/mojo/public/cpp/system/platform_handle.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/ppapi/proxy/nacl_message_scanner.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/sandbox/win/src/process_mitigations_win32k_dispatcher.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/sandbox/win/tests/common/controller.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/ui/gfx/mojo/buffer_types.mojom
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/ui/gfx/mojo/buffer_types_struct_traits.cc
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/ui/gfx/mojo/buffer_types_struct_traits.h
[modify] https://crrev.com/9d6afd71a4937e9f400609f839c2b39b22c4c497/ui/surface/transport_dib_posix.cc

Project Member

Comment 18 by bugdroid1@chromium.org, May 25 2017

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

commit e370c17ca1827988dd84a545bb728f9b6699d42d
Author: hajimehoshi <hajimehoshi@chromium.org>
Date: Thu May 25 06:12:16 2017

Replace SharedMemory::UniqueID usages with SharedMemoryHandle's guid

Shared memory's unique IDs are required to dump memory usages and before
this CL, files' inode values are used for them on POSIX. The problem was
that this was not portable. This CL replaces the current unique id with
the new shared memory handler's GUID introduced at crrev/2859843002,
which is available on any platforms.

Note that passing GUID across mojo is not implemented so the current
shared memory usages on memory-infra are still not accurate. Mojo fix
will be done in the future.

BUG= 604726 ,  713763 
TEST=n/a

Review-Url: https://codereview.chromium.org/2873433004
Cr-Commit-Position: refs/heads/master@{#474579}

[modify] https://crrev.com/e370c17ca1827988dd84a545bb728f9b6699d42d/base/memory/shared_memory.h
[modify] https://crrev.com/e370c17ca1827988dd84a545bb728f9b6699d42d/base/memory/shared_memory_posix.cc
[modify] https://crrev.com/e370c17ca1827988dd84a545bb728f9b6699d42d/base/memory/shared_memory_tracker.cc
[modify] https://crrev.com/e370c17ca1827988dd84a545bb728f9b6699d42d/base/memory/shared_memory_tracker.h

Blocking: 604726
I spoke with rockot@, who said that versioning is blocked on lazy-serialization, and is  still 6-8 weeks out.

He said he would try prototyping a reasonably-simple workaround next week that would allow us to transport GUIDs.
Blockedon: -696107
Based on discussions with appropriate stakeholders it is no longer a requirement that Mojo shared buffer handle serialization remain stable across binary versions in the near term. In light of this I'm going to implement persistence of these GUIDs across process bounaries ASAP to unblock further work.
Hi, I was wondering what is going on the current status.
Status: Fixed (was: Available)
This is fixed by rockot@ by  https://chromium-review.googlesource.com/c/540915/. Thanks rockot!

Sign in to add a comment