make base::SharedMemoryHandle a class on POSIX.

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}
Committed: https://chromium.googlesource.com/chromium/src/+/22a813bccaeb05fc47689b540701e6d590fa7648

[erikchen, 2017-04-26 22:27:31]

Description was changed from

==========
make base::SharedMemoryHandle a class on POSIX.

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

to

==========
make base::SharedMemoryHandle a class on POSIX.

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

It 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=
==========

[erikchen, 2017-04-26 22:34:37]

Description was changed from

==========
make base::SharedMemoryHandle a class on POSIX.

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

It 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=
==========

to

==========
make base::SharedMemoryHandle a class on POSIX.

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

It 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=
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-26 22:48:42]

Description was changed from

==========
make base::SharedMemoryHandle a class on POSIX.

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

It 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=
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

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

It 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
==========

[erikchen, 2017-04-26 22:48:58]

Description was changed from

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

It 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
==========

to

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

It 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
==========

[erikchen, 2017-04-26 23:17:32]

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

[commit-bot: I haz the power, 2017-04-26 23:18:07]

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-04-26 23:45:05]

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

[commit-bot: I haz the power, 2017-04-26 23:45:05]

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-04-27 00:45:17]

Description was changed from

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

It 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
==========

to

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

[erikchen, 2017-04-27 00:45:25]

Description was changed from

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

to

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

[erikchen, 2017-04-27 00:47:36]

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

[commit-bot: I haz the power, 2017-04-27 00:48:24]

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-04-27 02:48:18]

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

[commit-bot: I haz the power, 2017-04-27 02:48:19]

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

[erikchen, 2017-04-27 03:28:35]

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

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

mark: Please review.

[erikchen, 2017-04-27 03:28:47]

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

[commit-bot: I haz the power, 2017-04-27 03:29:17]

Dry run: CQ is trying da patch.

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

[Mark Mentovai, 2017-04-27 04:06:50]

I won’t be able to review this quickly. It may not happen until Monday.

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

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

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

Dry run: This issue passed the CQ dry run.

[erikchen, 2017-04-27 15:54:31]

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

[erikchen, 2017-04-27 15:54:32]

thakis: Please review.

[Nico, 2017-04-27 16:24:44]

I can look before Monday, but I'll be more confused than Mark.

Generally this looks nice. I was a bit confused about how much
SharedMemoryHandle owns the handle it contains, maybe you could say more about
that in the CL description or something.

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

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:31: class BASE_EXPORT SharedMemoryHandle {
Since they're now classes everywhere, I think it'd be good to move the #ifs into
the class definition -- some of the class internals are now identical on all
platforms (default ctor, IsValid(), Close() so it'd be nice if those were listed
just once. (Not in this CL.)

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:44: static SharedMemoryHandle
ImportHandle(int fd);
This type of thing seems to be a ctor instead of a factory function on win and
mac, why the difference?

Also, why even have that, couldn't clients just call the FileDescriptor ctor?

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:57: // Invalidates [but doesn't close] the
underlying OS resource.
Maybe say "leak unless careful" in the comment. Some of our smart pointery
classes call this "Release" and return the internal thing, maybe that'd be good
here?

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:63: FileDescriptor file_descriptor;
make private

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
File base/memory/shared_memory_handle_posix.cc (right):

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
base/memory/shared_memory_handle_posix.cc:19: 
do you want to call Close() in the dtor if auto_close is set? (see IMPORTANT
comment in
https://cs.chromium.org/chromium/src/base/file_descriptor_posix.h?q=filedescr...)

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
base/memory/shared_memory_handle_posix.cc:38: PLOG(ERROR) << "close";
Should this then call Invalidate()? Closing but keeping an invalid fd around
seems weird.

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
File base/memory/shared_memory_nacl.cc (right):

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
base/memory/shared_memory_nacl.cc:119: handle_copy.file_descriptor.auto_close =
true;
Isn't this the default?

I'm actually not sure, I don't see a copy ctor in FileDescriptor so I'm unsure
what the SharedMemoryHandle copy ctor does :-/

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

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
base/memory/shared_memory_posix.cc:43: : shm_(handle),
This file seems to ignore shm_.file_descriptor.auto_close

https://codereview.chromium.org/2843113002/diff/100001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/2843113002/diff/100001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:817: // TODO(morrita): Seems like this should return
false.
I take it you copied this block from somewhere; I didn't look at this file
closely.

https://codereview.chromium.org/2843113002/diff/100001/mojo/public/cpp/system...
File mojo/public/cpp/system/platform_handle.cc (right):

https://codereview.chromium.org/2843113002/diff/100001/mojo/public/cpp/system...
mojo/public/cpp/system/platform_handle.cc:125:
base::FileDescriptor(static_cast<int>(platform_handle.value), false));
random complaint about bare bool values looking meaningless

[erikchen, 2017-04-27 17:08:11]

Thanks thakis, PTAL.

Many of the issues come from the super weird/broken semantics for
SharedMemoryhandle [it really should be ScopedSharedMemoryHandle], which
unfortunately can't be fixed without a complete overhaul of the semantics of
IPC, which requires at a minimum moving away from Chrome IPC.

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

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:31: class BASE_EXPORT SharedMemoryHandle {
On 2017/04/27 16:24:44, Nico wrote:
> Since they're now classes everywhere, I think it'd be good to move the #ifs
into
> the class definition -- some of the class internals are now identical on all
> platforms (default ctor, IsValid(), Close() so it'd be nice if those were
listed
> just once. (Not in this CL.)

Added a TODO.

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:44: static SharedMemoryHandle
ImportHandle(int fd);
On 2017/04/27 16:24:44, Nico wrote:
> This type of thing seems to be a ctor instead of a factory function on win and
> mac, why the difference?
The constructors on Mac/Win will be going away. Once I introduce GUIDs [see the
crbug], all constructors should explicitly specify whether a new GUID needs to
be generated. Importing a handle will require generating a new GUID.

> 
> Also, why even have that, couldn't clients just call the FileDescriptor ctor?
That constructor is deprecated and going away.

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:57: // Invalidates [but doesn't close] the
underlying OS resource.
On 2017/04/27 16:24:44, Nico wrote:
> Maybe say "leak unless careful" in the comment. Some of our smart pointery
> classes call this "Release" and return the internal thing, maybe that'd be
good
> here?

Done.

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:63: FileDescriptor file_descriptor;
On 2017/04/27 16:24:44, Nico wrote:
> make private

Can't do that, Chrome IPC serialization requires that this member be public.
Sorry. :(

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
File base/memory/shared_memory_handle_posix.cc (right):

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
base/memory/shared_memory_handle_posix.cc:19: 
On 2017/04/27 16:24:44, Nico wrote:
> do you want to call Close() in the dtor if auto_close is set? (see IMPORTANT
> comment in
>
https://cs.chromium.org/chromium/src/base/file_descriptor_posix.h?q=filedescr...)

No, that's done in ipc_message_utils.cc. 

This CL is a refactor. SharedMemoryHandle is just a wrapper around
FileDescriptor.

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
base/memory/shared_memory_handle_posix.cc:38: PLOG(ERROR) << "close";
On 2017/04/27 16:24:44, Nico wrote:
> Should this then call Invalidate()? Closing but keeping an invalid fd around
> seems weird.

It should, but it never has on any platform. I've added a TODO in the header and
filed a bug against myself to look into this:
https://bugs.chromium.org/p/chromium/issues/detail?id=716072

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
File base/memory/shared_memory_nacl.cc (right):

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
base/memory/shared_memory_nacl.cc:119: handle_copy.file_descriptor.auto_close =
true;
On 2017/04/27 16:24:44, Nico wrote:
> Isn't this the default?
> 
> I'm actually not sure, I don't see a copy ctor in FileDescriptor so I'm unsure
> what the SharedMemoryHandle copy ctor does :-/

It doesn't matter what the default is [which can change w/o touching this file].
The point is that auto_close should be true for the new SharedMemoryHandle.

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

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
base/memory/shared_memory_posix.cc:43: : shm_(handle),
On 2017/04/27 16:24:44, Nico wrote:
> This file seems to ignore shm_.file_descriptor.auto_close

Yes, that's only used by ipc_message_utils.cc

https://codereview.chromium.org/2843113002/diff/100001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/2843113002/diff/100001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:817: // TODO(morrita): Seems like this should return
false.
On 2017/04/27 16:24:44, Nico wrote:
> I take it you copied this block from somewhere; I didn't look at this file
> closely.

This file needs a separate IPC review anyways. I'll get another reviewer.

https://codereview.chromium.org/2843113002/diff/100001/mojo/public/cpp/system...
File mojo/public/cpp/system/platform_handle.cc (right):

https://codereview.chromium.org/2843113002/diff/100001/mojo/public/cpp/system...
mojo/public/cpp/system/platform_handle.cc:125:
base::FileDescriptor(static_cast<int>(platform_handle.value), false));
On 2017/04/27 16:24:44, Nico wrote:
> random complaint about bare bool values looking meaningless

Sorry? This is just a refactor...

[erikchen, 2017-04-27 17:09:27]

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

[erikchen, 2017-04-27 17:09:27]

rockot: Please review ipc* and *mojo*.

Note that I left you a TODO in shared_memory_handle.h. Once Mojo supports
versioning and thus transport of GUIDs on SharedMemoryHandle, please remove the
constructor and fix all call-sites.

[erikchen, 2017-04-27 17:09:32]

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

[commit-bot: I haz the power, 2017-04-27 17:10:17]

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-04-27 17:30:45]

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

[commit-bot: I haz the power, 2017-04-27 17:30:46]

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

[Ken Rockot(use gerrit already), 2017-04-27 17:35:02]

lgtm

https://codereview.chromium.org/2843113002/diff/120001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/2843113002/diff/120001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:817: // TODO(morrita): Seems like this should return
false.
nit: Let's remove this TODO (and the one in FileDescriptor's traits). First,
morrita is long done, but more importantly I think the TODO is just wrong: if
writing an invalid handle is legal, reading one should be legal too. Returning
false would indicate a deserialization failure, and that's not what this is.

[Nico, 2017-04-27 17:38:50]

Lgtm

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

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:63: FileDescriptor file_descriptor;
On 2017/04/27 17:08:10, erikchen wrote:
> On 2017/04/27 16:24:44, Nico wrote:
> > make private
> 
> Can't do that, Chrome IPC serialization requires that this member be public.
> Sorry. :(

Add accessors for what's needed by IPC?

[erikchen, 2017-04-27 18:13:42]

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

https://codereview.chromium.org/2843113002/diff/100001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:63: FileDescriptor file_descriptor;
On 2017/04/27 17:38:50, Nico wrote:
> On 2017/04/27 17:08:10, erikchen wrote:
> > On 2017/04/27 16:24:44, Nico wrote:
> > > make private
> > 
> > Can't do that, Chrome IPC serialization requires that this member be public.
> > Sorry. :(
> 
> Add accessors for what's needed by IPC?

Done.

[The Chrome IPC macros don't support non-public members, but we're doing custom
serialization, so everything works.]

https://codereview.chromium.org/2843113002/diff/120001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/2843113002/diff/120001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:817: // TODO(morrita): Seems like this should return
false.
On 2017/04/27 17:35:02, Ken Rockot wrote:
> nit: Let's remove this TODO (and the one in FileDescriptor's traits). First,
> morrita is long done, but more importantly I think the TODO is just wrong: if
> writing an invalid handle is legal, reading one should be legal too. Returning
> false would indicate a deserialization failure, and that's not what this is.

Done.

[erikchen, 2017-04-27 18:13:54]

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

[commit-bot: I haz the power, 2017-04-27 18:14:24]

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-27 18:15:14]

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

[erikchen, 2017-04-27 18:15:15]

brettw: Please stamp chrome/, components/, content/, media/  and ui/.

This CL is a refactor - the major changes were reviewed by thakis and rockot,
now I just need a stamp for everything else.

[Nico, 2017-04-27 18:20:23]

I already cover chrome/ and ui/

On Apr 27, 2017 2:15 PM, <erikchen@chromium.org> wrote:

> brettw: Please stamp chrome/, components/, content/, media/ and ui/.
>
> This CL is a refactor - the major changes were reviewed by thakis and
> rockot,
> now I just need a stamp for everything else.
>
> https://codereview.chromium.org/2843113002/
>

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

[commit-bot: I haz the power, 2017-04-27 18:31:23]

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

[commit-bot: I haz the power, 2017-04-27 18:31:24]

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

[erikchen, 2017-04-27 18:57:41]

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

[commit-bot: I haz the power, 2017-04-27 18:58:22]

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-04-27 19:34:21]

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

[commit-bot: I haz the power, 2017-04-27 19:34:22]

Dry run: Try jobs failed on following builders:
  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_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  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-04-27 21:04:53]

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

[commit-bot: I haz the power, 2017-04-27 21:05:33]

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-04-27 21:37:27]

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

[commit-bot: I haz the power, 2017-04-27 21:37:28]

Dry run: 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_...)

[brettw, 2017-04-27 22:59:21]

lgtm

[erikchen, 2017-04-28 01:45:02]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-04-28 01:45:03]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2843113002/#ps170001
(title: "minor rename")

[commit-bot: I haz the power, 2017-04-28 01:46:20]

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-04-28 01:58:29]

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

[commit-bot: I haz the power, 2017-04-28 01:58:30]

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

[erikchen, 2017-04-28 04:09:40]

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

[erikchen, 2017-04-28 04:09:41]

rsesek: Looking for a SECURITY_OWNER review for
ui/gfx/mojo/buffer_types_struct_traits.cc.

Apparently rockot@ is an OWNER of ipc/ but not a security owner.

[erikchen, 2017-04-28 04:10:21]

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

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

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-04-28 05:02:02]

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

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

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-04-28 06:38:37]

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

[commit-bot: I haz the power, 2017-04-28 06:38:52]

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-04-28 09:01:59]

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

[commit-bot: I haz the power, 2017-04-28 09:02:00]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2017-04-28 16:19:13]

LGTM

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

https://codereview.chromium.org/2843113002/diff/190001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:52: // Takes ownership of the OS resource.
What happens if this is called on a Handle that already has an FD?

https://codereview.chromium.org/2843113002/diff/190001/base/memory/shared_mem...
File base/memory/shared_memory_handle_posix.cc (right):

https://codereview.chromium.org/2843113002/diff/190001/base/memory/shared_mem...
base/memory/shared_memory_handle_posix.cc:57: return SharedMemoryHandle();
Deliberately dropping the PLOG that was in ShareToProcessCommon?

[erikchen, 2017-04-28 17:03:39]

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

https://codereview.chromium.org/2843113002/diff/190001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:52: // Takes ownership of the OS resource.
On 2017/04/28 16:19:13, Robert Sesek wrote:
> What happens if this is called on a Handle that already has an FD?

other than a light slap on the wrist?

This is only used in ShareToProcessCommon, which I'm getting rid of shortly:
https://bugs.chromium.org/p/chromium/issues/detail?id=716206&q=owner%3Aerikch...

https://codereview.chromium.org/2843113002/diff/190001/base/memory/shared_mem...
File base/memory/shared_memory_handle_posix.cc (right):

https://codereview.chromium.org/2843113002/diff/190001/base/memory/shared_mem...
base/memory/shared_memory_handle_posix.cc:57: return SharedMemoryHandle();
On 2017/04/28 16:19:13, Robert Sesek wrote:
> Deliberately dropping the PLOG that was in ShareToProcessCommon?

I'll add it back in a followup CL.
https://bugs.chromium.org/p/chromium/issues/detail?id=716536

[erikchen, 2017-04-28 17:03:47]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-04-28 17:03:48]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
thakis@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2843113002/#ps190001
(title: "Fix test error.")

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

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-04-28 17:10:54]

CQ is committing da patch.

Bot data: {"patchset_id": 190001, "attempt_start_ts": 1493399027891510,
"parent_rev": "ea44a038e5c5d78b5756e9b1db213c505c260c93", "commit_rev":
"22a813bccaeb05fc47689b540701e6d590fa7648"}

[commit-bot: I haz the power, 2017-04-28 17:11:06]

Description was changed from

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

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/22a813bccaeb05fc47689b540701...
==========

[commit-bot: I haz the power, 2017-04-28 17:11:08]

Committed patchset #11 (id:190001) as
https://chromium.googlesource.com/chromium/src/+/22a813bccaeb05fc47689b540701...