Remove base::SharedMemory::ShareToProcess.

Remove base::SharedMemory::ShareToProcess.

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

The method was initially introduced to assist in IPC, before Chrome's IPC
subsystem was capable of transporting handles across processes. Now that the
IPC subsystem does have the capability, the functionality is no longer needed
in base::SharedMemory.

BUG=716206
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
TBR=brettw@chromium.org

Review-Url: https://codereview.chromium.org/2852803002
Cr-Commit-Position: refs/heads/master@{#468888}
Committed: https://chromium.googlesource.com/chromium/src/+/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd

[erikchen, 2017-04-28 22:03:36]

Description was changed from

==========
Remove base::SharedMemory::ShareToProcess.

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

The method was initially introduced to assist in IPC, before Chrome's IPC
subsystem was capable of transporting handles across resources. Now that the
IPC susbystem does have the capability, the functionality is no longer needed
in base::SharedMemory.

BUG=716206
==========

to

==========
Remove base::SharedMemory::ShareToProcess.

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

The method was initially introduced to assist in IPC, before Chrome's IPC
subsystem was capable of transporting handles across resources. Now that the
IPC susbystem does have the capability, the functionality is no longer needed
in base::SharedMemory.

BUG=716206
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
==========

[erikchen, 2017-04-28 22:05:34]

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

[erikchen, 2017-04-28 22:06:04]

Description was changed from

==========
Remove base::SharedMemory::ShareToProcess.

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

The method was initially introduced to assist in IPC, before Chrome's IPC
subsystem was capable of transporting handles across resources. Now that the
IPC susbystem does have the capability, the functionality is no longer needed
in base::SharedMemory.

BUG=716206
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
==========

to

==========
Remove base::SharedMemory::ShareToProcess.

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

The method was initially introduced to assist in IPC, before Chrome's IPC
subsystem was capable of transporting handles across processes. Now that the
IPC subsystem does have the capability, the functionality is no longer needed
in base::SharedMemory.

BUG=716206
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
==========

[commit-bot: I haz the power, 2017-04-28 22:06:13]

Dry run: CQ is trying da patch.

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

[erikchen, 2017-04-28 22:08:10]

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

[erikchen, 2017-04-28 22:08:11]

thakis: Please review.

[commit-bot: I haz the power, 2017-04-28 22:23:17]

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

[commit-bot: I haz the power, 2017-04-28 22:23:18]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[erikchen, 2017-05-01 12:27:42]

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

[commit-bot: I haz the power, 2017-05-01 12:27:47]

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-01 12:41:24]

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

[commit-bot: I haz the power, 2017-05-01 12:41:25]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[erikchen, 2017-05-01 18:56:38]

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

[commit-bot: I haz the power, 2017-05-01 18:57: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-01 19:24:12]

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

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

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_optional_gpu_tests_rel on master.tryserver.chromium.android
(JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_opti...)

[erikchen, 2017-05-01 19:35:07]

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

[commit-bot: I haz the power, 2017-05-01 19:35: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-01 22:24:22]

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

[commit-bot: I haz the power, 2017-05-01 22:24:23]

Dry run: This issue passed the CQ dry run.

[erikchen, 2017-05-02 01:58:37]

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

[commit-bot: I haz the power, 2017-05-02 01:59: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-02 04:03:01]

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

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

Dry run: This issue passed the CQ dry run.

[Nico, 2017-05-02 16:02:14]

I got confused in file 2 :-(

https://codereview.chromium.org/2852803002/diff/100001/base/memory/discardabl...
File base/memory/discardable_shared_memory_unittest.cc (right):

https://codereview.chromium.org/2852803002/diff/100001/base/memory/discardabl...
base/memory/discardable_shared_memory_unittest.cc:47: SharedMemoryHandle
shared_handle = memory1.handle().Duplicate();
Doesn't this leak?

[Nico, 2017-05-02 16:06:12]

I another thing to be confused about, but it's kind of the same thing as the
first.

https://codereview.chromium.org/2852803002/diff/100001/content/browser/browse...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/2852803002/diff/100001/content/browser/browse...
content/browser/browser_child_process_host_impl.cc:517: Send(new
ChildProcessMsg_SetHistogramMemory(
Does sending a SharedMemoryHandle consume it? If not, why is the Duplicate()
needed?

https://codereview.chromium.org/2852803002/diff/100001/device/gamepad/gamepad...
File device/gamepad/gamepad_provider.cc (right):

https://codereview.chromium.org/2852803002/diff/100001/device/gamepad/gamepad...
device/gamepad/gamepad_provider.cc:85: base::SharedMemoryHandle
GamepadProvider::GetSharedMemoryHandleForProcess(
This whole function can probably go away now?

[erikchen, 2017-05-02 18:59:59]

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

[commit-bot: I haz the power, 2017-05-02 19:00:46]

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-02 19:00:52]

thakis: PTAL

https://codereview.chromium.org/2852803002/diff/100001/base/memory/discardabl...
File base/memory/discardable_shared_memory_unittest.cc (right):

https://codereview.chromium.org/2852803002/diff/100001/base/memory/discardabl...
base/memory/discardable_shared_memory_unittest.cc:47: SharedMemoryHandle
shared_handle = memory1.handle().Duplicate();
On 2017/05/02 16:02:14, Nico wrote:
> Doesn't this leak?

No, constructing SharedMemory from a SharedMemoryHandle takes "ownership" of the
underlying handle.

Ownership semantics for SharedMemoryHandle are super borked. In an ideal world
everything would be handled by Scoped* semantics, but that would require
significnat refactors which I'm not particularly inclined to start.

https://codereview.chromium.org/2852803002/diff/100001/content/browser/browse...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/2852803002/diff/100001/content/browser/browse...
content/browser/browser_child_process_host_impl.cc:517: Send(new
ChildProcessMsg_SetHistogramMemory(
On 2017/05/02 16:06:12, Nico wrote:
> Does sending a SharedMemoryHandle consume it? If not, why is the Duplicate()
> needed?

Yes, I've updated the documentation for Duplicate().

https://codereview.chromium.org/2852803002/diff/100001/device/gamepad/gamepad...
File device/gamepad/gamepad_provider.cc (right):

https://codereview.chromium.org/2852803002/diff/100001/device/gamepad/gamepad...
device/gamepad/gamepad_provider.cc:85: base::SharedMemoryHandle
GamepadProvider::GetSharedMemoryHandleForProcess(
On 2017/05/02 16:06:12, Nico wrote:
> This whole function can probably go away now?

The function is still needed to access the private member
gamepad_shared_buffer_, but the parameter is no longer needed. I removed taht
and rename the method.

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

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

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

Dry run: 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_...)
  android_optional_gpu_tests_rel on master.tryserver.chromium.android
(JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_opti...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Nico, 2017-05-02 19:36:25]

https://codereview.chromium.org/2852803002/diff/100001/base/memory/discardabl...
File base/memory/discardable_shared_memory_unittest.cc (right):

https://codereview.chromium.org/2852803002/diff/100001/base/memory/discardabl...
base/memory/discardable_shared_memory_unittest.cc:47: SharedMemoryHandle
shared_handle = memory1.handle().Duplicate();
On 2017/05/02 19:00:51, erikchen wrote:
> On 2017/05/02 16:02:14, Nico wrote:
> > Doesn't this leak?
> 
> No, constructing SharedMemory from a SharedMemoryHandle takes "ownership" of
the
> underlying handle.

But constructing a SharedMemoryHandle, not a SharedMemory here (?)

> 
> Ownership semantics for SharedMemoryHandle are super borked. In an ideal world
> everything would be handled by Scoped* semantics, but that would require
> significnat refactors which I'm not particularly inclined to start.

[erikchen, 2017-05-02 19:44:04]

https://codereview.chromium.org/2852803002/diff/100001/base/memory/discardabl...
File base/memory/discardable_shared_memory_unittest.cc (right):

https://codereview.chromium.org/2852803002/diff/100001/base/memory/discardabl...
base/memory/discardable_shared_memory_unittest.cc:47: SharedMemoryHandle
shared_handle = memory1.handle().Duplicate();
On 2017/05/02 19:36:25, Nico wrote:
> On 2017/05/02 19:00:51, erikchen wrote:
> > On 2017/05/02 16:02:14, Nico wrote:
> > > Doesn't this leak?
> > 
> > No, constructing SharedMemory from a SharedMemoryHandle takes "ownership" of
> the
> > underlying handle.
> 
> But constructing a SharedMemoryHandle, not a SharedMemory here (?)
> 
> > 
> > Ownership semantics for SharedMemoryHandle are super borked. In an ideal
world
> > everything would be handled by Scoped* semantics, but that would require
> > significnat refactors which I'm not particularly inclined to start.

It's immediately passed to TestDiscardableSharedMemory, which inherits from
DiscardableSharedMemory...whose destructor doesn't call Close() on the handle,
leaking memory. :(

This is actually a bug in DiscardableSharedMemory whose root cause is the
unclear semantics of SharedMemoryHandle ownership. I'll fix that separately,
since this CL is jsut a refactor [which maintains the existing leak].

[Nico, 2017-05-02 19:54:17]

Thanks, looking through the CL now.

https://codereview.chromium.org/2852803002/diff/100001/base/memory/discardabl...
File base/memory/discardable_shared_memory_unittest.cc (right):

https://codereview.chromium.org/2852803002/diff/100001/base/memory/discardabl...
base/memory/discardable_shared_memory_unittest.cc:47: SharedMemoryHandle
shared_handle = memory1.handle().Duplicate();
On 2017/05/02 19:44:04, erikchen wrote:
> On 2017/05/02 19:36:25, Nico wrote:
> > On 2017/05/02 19:00:51, erikchen wrote:
> > > On 2017/05/02 16:02:14, Nico wrote:
> > > > Doesn't this leak?
> > > 
> > > No, constructing SharedMemory from a SharedMemoryHandle takes "ownership"
of
> > the
> > > underlying handle.
> > 
> > But constructing a SharedMemoryHandle, not a SharedMemory here (?)
> > 
> > > 
> > > Ownership semantics for SharedMemoryHandle are super borked. In an ideal
> world
> > > everything would be handled by Scoped* semantics, but that would require
> > > significnat refactors which I'm not particularly inclined to start.
> 
> It's immediately passed to TestDiscardableSharedMemory, which inherits from
> DiscardableSharedMemory...whose destructor doesn't call Close() on the handle,
> leaking memory. :(
> 
> This is actually a bug in DiscardableSharedMemory whose root cause is the
> unclear semantics of SharedMemoryHandle ownership. I'll fix that separately,
> since this CL is jsut a refactor [which maintains the existing leak].

Ah, thanks for explaining. I feel there probably should be a at least a bug for
adding Scoped semantics. Maybe we could have an automated rewriter do most of
the work. (You don't have to work on this, but if there isn't a bug for it,
there really should be.)

[erikchen, 2017-05-02 19:54:18]

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

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

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-02 19:56:38]

> Ah, thanks for explaining. I feel there probably should be a at least a bug
for
> adding Scoped semantics. Maybe we could have an automated rewriter do most of
> the work. (You don't have to work on this, but if there isn't a bug for it,
> there really should be.)

crbug: https://bugs.chromium.org/p/chromium/issues/detail?id=640840

[Nico, 2017-05-02 20:08:01]

Thanks again for explaining, this now made much more sense. I tried to check if
everything that gets a handle consumes it; in a few places I didn't check, I
called those out below. I also didn't check that
https://cs.chromium.org/chromium/src/ppapi/proxy/resource_message_params.h?ty...
gets called with true for the SerializedHandles.

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

https://codereview.chromium.org/2852803002/diff/160001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:60: // parameter to an IPC message will cause
the IPC subsystem to consume the OS
Feels like a bit strange place to document this (I'd expected this somewhere in
the IPC code), but ok :-)

https://codereview.chromium.org/2852803002/diff/160001/chrome/renderer/pepper...
File chrome/renderer/pepper/pepper_shared_memory_message_filter.cc (right):

https://codereview.chromium.org/2852803002/diff/160001/chrome/renderer/pepper...
chrome/renderer/pepper/pepper_shared_memory_message_filter.cc:56:
->TrackSharedMemoryHandle(instance, host_shm_handle, size);
I assume this assumes ownership too. I didn't check.

https://codereview.chromium.org/2852803002/diff/160001/components/display_com...
File components/display_compositor/host_shared_bitmap_manager_unittest.cc
(right):

https://codereview.chromium.org/2852803002/diff/160001/components/display_com...
components/display_compositor/host_shared_bitmap_manager_unittest.cc:31:
client.ChildAllocatedSharedBitmap(size_in_bytes, handle, id);
I did check that this does assume ownership.

https://codereview.chromium.org/2852803002/diff/160001/extensions/browser/use...
File extensions/browser/user_script_loader.cc (right):

https://codereview.chromium.org/2852803002/diff/160001/extensions/browser/use...
extensions/browser/user_script_loader.cc:424: if
(base::SharedMemory::IsHandleValid(handle_for_process)) {
Is this just an alias for handle_for_process.isValid() now?

https://codereview.chromium.org/2852803002/diff/160001/gpu/ipc/service/gpu_ch...
File gpu/ipc/service/gpu_channel_test_common.cc (right):

https://codereview.chromium.org/2852803002/diff/160001/gpu/ipc/service/gpu_ch...
gpu/ipc/service/gpu_channel_test_common.cc:155: base::SharedMemoryHandle
GpuChannelTestCommon::GetSharedHandle() {
It's weird to have things transferring ownership start with just Get(). Maybe
that's Cocoa speaking, but it seems like this is just asking for bugs.

https://codereview.chromium.org/2852803002/diff/160001/media/audio/audio_inpu...
File media/audio/audio_input_device_unittest.cc (right):

https://codereview.chromium.org/2852803002/diff/160001/media/audio/audio_inpu...
media/audio/audio_input_device_unittest.cc:137: device.get(),
duplicated_memory_handle,
Since this is mocked out, we probably leak duplicated_memory_handle in this
test.

https://codereview.chromium.org/2852803002/diff/160001/media/audio/audio_outp...
File media/audio/audio_output_device_unittest.cc (right):

https://codereview.chromium.org/2852803002/diff/160001/media/audio/audio_outp...
media/audio/audio_output_device_unittest.cc:235: duplicated_memory_handle,
Didn't verify that this one assumes ownership.

https://codereview.chromium.org/2852803002/diff/160001/media/gpu/video_decode...
File media/gpu/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2852803002/diff/160001/media/gpu/video_decode...
media/gpu/video_decode_accelerator_unittest.cc:1090: BitstreamBuffer
bitstream_buffer(next_bitstream_buffer_id_, dup_handle,
I tried to verify that BitstreamBuffer assumes ownership, and from what I can
tell it doesn't. Please check and add TODO if you too think that this leaks.

https://codereview.chromium.org/2852803002/diff/160001/media/gpu/video_encode...
File media/gpu/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2852803002/diff/160001/media/gpu/video_encode...
media/gpu/video_encode_accelerator_unittest.cc:1652: BitstreamBuffer
bitstream_buffer(next_output_buffer_id_++, dup_handle,
likewise

https://codereview.chromium.org/2852803002/diff/160001/ppapi/proxy/video_deco...
File ppapi/proxy/video_decoder_resource_unittest.cc (right):

https://codereview.chromium.org/2852803002/diff/160001/ppapi/proxy/video_deco...
ppapi/proxy/video_decoder_resource_unittest.cc:150:
std::unique_ptr<base::SharedMemoryHandle> shm_handle;
I don't understand why this is heap-allocated instead of just stack-allocated.

[commit-bot: I haz the power, 2017-05-02 20:25:19]

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

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

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

[erikchen, 2017-05-02 21:30:55]

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

[commit-bot: I haz the power, 2017-05-02 21:31:25]

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-02 21:31:52]

On 2017/05/02 20:08:01, Nico wrote:
> Thanks again for explaining, this now made much more sense. I tried to check
if
> everything that gets a handle consumes it; in a few places I didn't check, I
> called those out below. I also didn't check that
>
https://cs.chromium.org/chromium/src/ppapi/proxy/resource_message_params.h?ty...
> gets called with true for the SerializedHandles.
That only gets called in the receiving process.
https://cs.chromium.org/chromium/src/ppapi/proxy/resource_message_params.cc?t...

[erikchen, 2017-05-02 21:33:11]

https://codereview.chromium.org/2852803002/diff/160001/chrome/renderer/pepper...
File chrome/renderer/pepper/pepper_shared_memory_message_filter.cc (right):

https://codereview.chromium.org/2852803002/diff/160001/chrome/renderer/pepper...
chrome/renderer/pepper/pepper_shared_memory_message_filter.cc:56:
->TrackSharedMemoryHandle(instance, host_shm_handle, size);
On 2017/05/02 20:08:00, Nico wrote:
> I assume this assumes ownership too. I didn't check.

I don't think it always does. I added a TODO.

https://codereview.chromium.org/2852803002/diff/160001/extensions/browser/use...
File extensions/browser/user_script_loader.cc (right):

https://codereview.chromium.org/2852803002/diff/160001/extensions/browser/use...
extensions/browser/user_script_loader.cc:424: if
(base::SharedMemory::IsHandleValid(handle_for_process)) {
On 2017/05/02 20:08:00, Nico wrote:
> Is this just an alias for handle_for_process.isValid() now?

Yes. We could do a whole other refactor to make that change - I'm avoiding doing
so, since it's not actually needed for the changes I'm *actually* trying to
make.

https://codereview.chromium.org/2852803002/diff/160001/gpu/ipc/service/gpu_ch...
File gpu/ipc/service/gpu_channel_test_common.cc (right):

https://codereview.chromium.org/2852803002/diff/160001/gpu/ipc/service/gpu_ch...
gpu/ipc/service/gpu_channel_test_common.cc:155: base::SharedMemoryHandle
GpuChannelTestCommon::GetSharedHandle() {
On 2017/05/02 20:08:00, Nico wrote:
> It's weird to have things transferring ownership start with just Get(). Maybe
> that's Cocoa speaking, but it seems like this is just asking for bugs.

This is a non-virtual method defined in a test class. *shrug*.

https://codereview.chromium.org/2852803002/diff/160001/media/audio/audio_inpu...
File media/audio/audio_input_device_unittest.cc (right):

https://codereview.chromium.org/2852803002/diff/160001/media/audio/audio_inpu...
media/audio/audio_input_device_unittest.cc:137: device.get(),
duplicated_memory_handle,
On 2017/05/02 20:08:01, Nico wrote:
> Since this is mocked out, we probably leak duplicated_memory_handle in this
> test.

I added a Close() to the end of this test.

https://codereview.chromium.org/2852803002/diff/160001/media/audio/audio_outp...
File media/audio/audio_output_device_unittest.cc (right):

https://codereview.chromium.org/2852803002/diff/160001/media/audio/audio_outp...
media/audio/audio_output_device_unittest.cc:235: duplicated_memory_handle,
On 2017/05/02 20:08:01, Nico wrote:
> Didn't verify that this one assumes ownership.

I looked at real use cases of OnStreamCreated, and am pretty sure that there
exists code paths that cause the handle to leak. Added a TODO here.

https://codereview.chromium.org/2852803002/diff/160001/media/gpu/video_decode...
File media/gpu/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2852803002/diff/160001/media/gpu/video_decode...
media/gpu/video_decode_accelerator_unittest.cc:1090: BitstreamBuffer
bitstream_buffer(next_bitstream_buffer_id_, dup_handle,
On 2017/05/02 20:08:01, Nico wrote:
> I tried to verify that BitstreamBuffer assumes ownership, and from what I can
> tell it doesn't. Please check and add TODO if you too think that this leaks.

Added a TODO

https://codereview.chromium.org/2852803002/diff/160001/media/gpu/video_encode...
File media/gpu/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2852803002/diff/160001/media/gpu/video_encode...
media/gpu/video_encode_accelerator_unittest.cc:1652: BitstreamBuffer
bitstream_buffer(next_output_buffer_id_++, dup_handle,
On 2017/05/02 20:08:01, Nico wrote:
> likewise

Done.

https://codereview.chromium.org/2852803002/diff/160001/ppapi/proxy/video_deco...
File ppapi/proxy/video_decoder_resource_unittest.cc (right):

https://codereview.chromium.org/2852803002/diff/160001/ppapi/proxy/video_deco...
ppapi/proxy/video_decoder_resource_unittest.cc:150:
std::unique_ptr<base::SharedMemoryHandle> shm_handle;
On 2017/05/02 20:08:01, Nico wrote:
> I don't understand why this is heap-allocated instead of just stack-allocated.

Changed to stack allocated.

[erikchen, 2017-05-02 21:34:18]

thakis: PTAL

I'm pretty sure there are several leaks of SharedMemoryHandle. I don't think
there's a sane way of fixing this without switching to ScopedSharedMemoryHandle.
We could theoretically do that incrementally [e.g. by adding
DuplicateWithScope]. I sprinkled some TODOs in the code base, linking to
https://bugs.chromium.org/p/chromium/issues/detail?id=640840.

[Nico, 2017-05-02 21:45:38]

Lgtm

[commit-bot: I haz the power, 2017-05-02 21:46:16]

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

[commit-bot: I haz the power, 2017-05-02 21:46:17]

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...)

[erikchen, 2017-05-03 01:18:54]

Description was changed from

==========
Remove base::SharedMemory::ShareToProcess.

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

The method was initially introduced to assist in IPC, before Chrome's IPC
subsystem was capable of transporting handles across processes. Now that the
IPC subsystem does have the capability, the functionality is no longer needed
in base::SharedMemory.

BUG=716206
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
==========

to

==========
Remove base::SharedMemory::ShareToProcess.

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

The method was initially introduced to assist in IPC, before Chrome's IPC
subsystem was capable of transporting handles across processes. Now that the
IPC subsystem does have the capability, the functionality is no longer needed
in base::SharedMemory.

BUG=716206
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
TBR=brettw@chromium.org
==========

[erikchen, 2017-05-03 01:19:31]

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

[erikchen, 2017-05-03 01:19:32]

TBRing brettw for a top-level stamp.

[erikchen, 2017-05-03 01:19:38]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-05-03 01:19:38]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2852803002/#ps200001
(title: "compile error.")

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

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-03 01:43:16]

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

[commit-bot: I haz the power, 2017-05-03 01:43:18]

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

[erikchen, 2017-05-03 02:07:54]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-05-03 02:07:54]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2852803002/#ps220001
(title: "Compile error.")

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

CQ is trying da patch.

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

[erikchen, 2017-05-03 02:08:28]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2017-05-03 02:08: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-03 04:05:05]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1493777308232620,
"parent_rev": "56e68afe8f6fbe6e9d5fb234795034601b475298", "commit_rev":
"37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd"}

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

Description was changed from

==========
Remove base::SharedMemory::ShareToProcess.

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

The method was initially introduced to assist in IPC, before Chrome's IPC
subsystem was capable of transporting handles across processes. Now that the
IPC subsystem does have the capability, the functionality is no longer needed
in base::SharedMemory.

BUG=716206
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
TBR=brettw@chromium.org
==========

to

==========
Remove base::SharedMemory::ShareToProcess.

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

The method was initially introduced to assist in IPC, before Chrome's IPC
subsystem was capable of transporting handles across processes. Now that the
IPC subsystem does have the capability, the functionality is no longer needed
in base::SharedMemory.

BUG=716206
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
TBR=brettw@chromium.org

Review-Url: https://codereview.chromium.org/2852803002
Cr-Commit-Position: refs/heads/master@{#468888}
Committed:
https://chromium.googlesource.com/chromium/src/+/37e7d8301a078bd1b6b3ef7cffdb...
==========

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

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/37e7d8301a078bd1b6b3ef7cffdb...