[mojo-edk] Expose portable API for platform handle wrapping

[mojo-edk] Expose portable API for platform handle wrapping

Introduces a public C API allowing embedders and apps to exchange
Mojo handles for generic platform handles or platform shared
memory handles.

BUG=
R=amistry@chromium.org
TBR=ben@chromium.org

Committed: https://crrev.com/28a287e8850ffd41da04ef0fe8f41e7a5dfb888c
Cr-Commit-Position: refs/heads/master@{#395521}

[Ken Rockot(use gerrit already), 2016-05-18 20:05:21]

As discussed previously, since this is going to start blocking other peoples'
work.

One question and one comment:

Q: How do you feel about MojoWrap/UnwrapPlatformFile instead of
MojoWrap/UnwrapPlatformHandle? I didn't come up with a not-horrible way of
attaching additional context to MojoPlatformHandle, so there's no way to
distinguish between a Mach port and an FD. So this API is just implemented to
assume the platform handle is an FD. "PlatformFile" would make that more
explicit, though it's not strictly true on Windows for example, where platform
handle could be anything at all. Alternatively, any better ideas?

Comment: I added some semi-redundant static methods to PlatformSharedBuffer. I
think these will move to the public/cpp/system API in a follow-up patch but I
was going to defer until then.

[Anand Mistry (off Chromium), 2016-05-20 06:25:40]

I'm happy using the "Handle" naming instead of "File". I think using file is
misleading since it could be a pipe/socket/shm/other on posix, or practically
anything on windows.

As for moving the static methods in PlatformSharedBuffer to public, I don't see
why. It's an embedder API, as are most uses of
PlatformFile/PlatformSharedBuffer. What you want in public is a C++ wrapper
around the C functions.

https://codereview.chromium.org/1995753002/diff/40001/mojo/edk/embedder/entry...
File mojo/edk/embedder/entrypoints.cc (right):

https://codereview.chromium.org/1995753002/diff/40001/mojo/edk/embedder/entry...
mojo/edk/embedder/entrypoints.cc:7: #include "base/process/process_handle.h"
This looks unused here.

https://codereview.chromium.org/1995753002/diff/40001/mojo/edk/embedder/platf...
File mojo/edk/embedder/platform_shared_buffer.cc (right):

https://codereview.chromium.org/1995753002/diff/40001/mojo/edk/embedder/platf...
mojo/edk/embedder/platform_shared_buffer.cc:248: #if defined(OS_WIN)
Please change this to use the new function.

https://codereview.chromium.org/1995753002/diff/40001/mojo/public/c/system/ty...
File mojo/public/c/system/types.h (right):

https://codereview.chromium.org/1995753002/diff/40001/mojo/public/c/system/ty...
mojo/public/c/system/types.h:43: typedef uint64_t MojoPlatformHandle;
I don't like the fact that the meaning here is context specific, and behavior
isn't hidden behind an interface (unlike MojoHandle).

How about a struct instead:
typedef struct _MojoPlatformHandle {
  uint64_t handle;
  uint64_t flags;
} MojoPlatformHandle;

For Mach ports, have a THIS_IS_A_MACH_PORT flag.

See MojoHandleSignalsState for precedence.

https://codereview.chromium.org/1995753002/diff/40001/mojo/public/c/system/ty...
mojo/public/c/system/types.h:46: const MojoPlatformHandle
MOJO_PLATFORM_HANDLE_INVALID = 0xffffffffffffffffULL;
static_cast<>(-1) is the common pattern here.

[Ken Rockot(use gerrit already), 2016-05-20 22:10:27]

Patchset #4 (id:60001) has been deleted

[Ken Rockot(use gerrit already), 2016-05-20 22:19:43]

I reworked things a bit (see inline comments).

I also didn't want to end up in an unfortunate state where I either had to
temporarily rename MojoPlatformHandle here, or rename the existing one in
//mojo/platform_handle, so I took the liberty of introducing the public C++ API
here as well and updating all call sites to use it.

I've replaced the build targets in //mojo/platform_handle with empty ones, and I
can remove that dir and references to those targets (there are a lot) in a
future patch.

Please feel free to ignore changes outside of //mojo

One last note: I opted to duplicate code between the unit tests and the public
C++ library, but I don't feel strongly about this. I'd be happy to use the C++
library from tests instead. WDYT?

https://codereview.chromium.org/1995753002/diff/40001/mojo/edk/embedder/entry...
File mojo/edk/embedder/entrypoints.cc (right):

https://codereview.chromium.org/1995753002/diff/40001/mojo/edk/embedder/entry...
mojo/edk/embedder/entrypoints.cc:7: #include "base/process/process_handle.h"
On 2016/05/20 at 06:25:40, Anand Mistry wrote:
> This looks unused here.

Removed

https://codereview.chromium.org/1995753002/diff/40001/mojo/edk/embedder/platf...
File mojo/edk/embedder/platform_shared_buffer.cc (right):

https://codereview.chromium.org/1995753002/diff/40001/mojo/edk/embedder/platf...
mojo/edk/embedder/platform_shared_buffer.cc:248: #if defined(OS_WIN)
On 2016/05/20 at 06:25:40, Anand Mistry wrote:
> Please change this to use the new function.

No longer relevant since I removed the new function

https://codereview.chromium.org/1995753002/diff/40001/mojo/public/c/system/ty...
File mojo/public/c/system/types.h (right):

https://codereview.chromium.org/1995753002/diff/40001/mojo/public/c/system/ty...
mojo/public/c/system/types.h:43: typedef uint64_t MojoPlatformHandle;
On 2016/05/20 at 06:25:40, Anand Mistry wrote:
> I don't like the fact that the meaning here is context specific, and behavior
isn't hidden behind an interface (unlike MojoHandle).
> 
> How about a struct instead:
> typedef struct _MojoPlatformHandle {
>   uint64_t handle;
>   uint64_t flags;
> } MojoPlatformHandle;
> 
> For Mach ports, have a THIS_IS_A_MACH_PORT flag.
> 
> See MojoHandleSignalsState for precedence.

I took it a step further and decided we should have an explicit
MojoPlatformHandleType value to make the handle type explicit. This seems
clearer and more extensible than using flags to modify the treatment of the
handle value.

https://codereview.chromium.org/1995753002/diff/40001/mojo/public/c/system/ty...
mojo/public/c/system/types.h:46: const MojoPlatformHandle
MOJO_PLATFORM_HANDLE_INVALID = 0xffffffffffffffffULL;
On 2016/05/20 at 06:25:40, Anand Mistry wrote:
> static_cast<>(-1) is the common pattern here.

WontFix (Obsolete)

With a concept of "handle type", there's no an INVALID handle type, so no need
for special handle values.

[Ken Rockot(use gerrit already), 2016-05-20 22:21:57]

Patchset #4 (id:80001) has been deleted

[Ken Rockot(use gerrit already), 2016-05-20 23:23:15]

Patchset #5 (id:120001) has been deleted

[Ken Rockot(use gerrit already), 2016-05-20 23:23:20]

Patchset #4 (id:100001) has been deleted

[Ken Rockot(use gerrit already), 2016-05-21 18:08:26]

Patchset #5 (id:160001) has been deleted

[Ken Rockot(use gerrit already), 2016-05-21 18:08:34]

Patchset #4 (id:140001) has been deleted

[Ken Rockot(use gerrit already), 2016-05-21 19:45:33]

Patchset #4 (id:180001) has been deleted

[Ken Rockot(use gerrit already), 2016-05-22 00:05:39]

Patchset #4 (id:200001) has been deleted

[Anand Mistry (off Chromium), 2016-05-23 07:16:53]

https://codereview.chromium.org/1995753002/diff/220001/mojo/edk/system/core.cc
File mojo/edk/system/core.cc (right):

https://codereview.chromium.org/1995753002/diff/220001/mojo/edk/system/core.c...
mojo/edk/system/core.cc:64: ScopedPlatformHandle* out_handle) {
indenting

https://codereview.chromium.org/1995753002/diff/220001/mojo/edk/system/core.c...
mojo/edk/system/core.cc:68: if (platform_handle->type ==
MOJO_PLATFORM_HANDLE_TYPE_INVALID) {
This case exists twice with different error return codes. Which one is correct?

https://codereview.chromium.org/1995753002/diff/220001/mojo/edk/system/core.c...
mojo/edk/system/core.cc:128: #endif
#endif  // defined(OS_MACOSX) && !defined(OS_IOS)

For clarity since you have nested #ifs

https://codereview.chromium.org/1995753002/diff/220001/mojo/edk/system/core.c...
mojo/edk/system/core.cc:1015: return MOJO_RESULT_RESOURCE_EXHAUSTED;
You need to call Close() on the dispatcher if it wasn't successfully added.

https://codereview.chromium.org/1995753002/diff/220001/mojo/edk/system/node_c...
File mojo/edk/system/node_channel.cc (right):

https://codereview.chromium.org/1995753002/diff/220001/mojo/edk/system/node_c...
mojo/edk/system/node_channel.cc:719: LOG(ERROR) << "OWNER OF HANDLE GOING TO BE
" << remote_process_handle;
debugging?

https://codereview.chromium.org/1995753002/diff/220001/mojo/public/c/system/p...
File mojo/public/c/system/platform_handle.h (right):

https://codereview.chromium.org/1995753002/diff/220001/mojo/public/c/system/p...
mojo/public/c/system/platform_handle.h:99: // NOTE: It is not alwawys possible
to detect if |platform_handle| is valid, so
s/alwawys/always

https://codereview.chromium.org/1995753002/diff/220001/mojo/public/c/system/p...
mojo/public/c/system/platform_handle.h:100: // this call may succeed even when
|handle| invalid (e.g. a valid file
s/|handle|/|platform_handle|

Also, I think you really mean |platform_handle->value|, but now I'm really nit
picking.

https://codereview.chromium.org/1995753002/diff/220001/mojo/public/c/system/p...
mojo/public/c/system/platform_handle.h:107: // the Mojo handle is closed. If
this call succeeds, ownership of the underlying
"the mojo handle is closed"
This sentence is redundant based on the last sentence "the mojo handle is always
closed". Also, this and the next sentence don't flow well (they both begin with
"if this call succeeds").

https://codereview.chromium.org/1995753002/diff/220001/mojo/public/cpp/system...
File mojo/public/cpp/system/platform_handle.h (right):

https://codereview.chromium.org/1995753002/diff/220001/mojo/public/cpp/system...
mojo/public/cpp/system/platform_handle.h:8: // Please see
"mojo/public/c/system/buffer.h" for complete documentation of the
s/buffer/platform_handle

https://codereview.chromium.org/1995753002/diff/220001/mojo/public/cpp/system...
mojo/public/cpp/system/platform_handle.h:54: #error Unsupported platform.
Hmmm. We don't support platforms that are non-posix and non-windows. Should we
even bother with this case? I'm pretty sure we don't consistently handle it.

https://codereview.chromium.org/1995753002/diff/220001/mojo/public/cpp/system...
mojo/public/cpp/system/platform_handle.h:55: #endif
#endif  // defined(OS_POSIX)

https://codereview.chromium.org/1995753002/diff/220001/mojo/public/cpp/system...
mojo/public/cpp/system/platform_handle.h:57: inline uint64_t
PlatformHandleValueFromPlatformFile(base::PlatformFile file) {
Shouldn't these two be in an anonymous or internal namespace?

https://codereview.chromium.org/1995753002/diff/220001/mojo/public/cpp/system...
mojo/public/cpp/system/platform_handle.h:74: inline ScopedHandle
WrapPlatformFile(base::PlatformFile platform_file) {
These implementations are non-trivial and not performance sensitive. Please move
to a .cc file.

[Ken Rockot(use gerrit already), 2016-05-23 17:17:54]

https://codereview.chromium.org/1995753002/diff/220001/mojo/edk/system/core.cc
File mojo/edk/system/core.cc (right):

https://codereview.chromium.org/1995753002/diff/220001/mojo/edk/system/core.c...
mojo/edk/system/core.cc:64: ScopedPlatformHandle* out_handle) {
On 2016/05/23 at 07:16:52, Anand Mistry wrote:
> indenting

done

https://codereview.chromium.org/1995753002/diff/220001/mojo/edk/system/core.c...
mojo/edk/system/core.cc:68: if (platform_handle->type ==
MOJO_PLATFORM_HANDLE_TYPE_INVALID) {
On 2016/05/23 at 07:16:52, Anand Mistry wrote:
> This case exists twice with different error return codes. Which one is
correct?

Woops - the first one is correct. Removed the second, which was supposed to have
been deleted already.

https://codereview.chromium.org/1995753002/diff/220001/mojo/edk/system/core.c...
mojo/edk/system/core.cc:128: #endif
On 2016/05/23 at 07:16:52, Anand Mistry wrote:
> #endif  // defined(OS_MACOSX) && !defined(OS_IOS)
> 
> For clarity since you have nested #ifs

Done

https://codereview.chromium.org/1995753002/diff/220001/mojo/edk/system/core.c...
mojo/edk/system/core.cc:1015: return MOJO_RESULT_RESOURCE_EXHAUSTED;
On 2016/05/23 at 07:16:52, Anand Mistry wrote:
> You need to call Close() on the dispatcher if it wasn't successfully added.

Done

https://codereview.chromium.org/1995753002/diff/220001/mojo/edk/system/node_c...
File mojo/edk/system/node_channel.cc (right):

https://codereview.chromium.org/1995753002/diff/220001/mojo/edk/system/node_c...
mojo/edk/system/node_channel.cc:719: LOG(ERROR) << "OWNER OF HANDLE GOING TO BE
" << remote_process_handle;
On 2016/05/23 at 07:16:52, Anand Mistry wrote:
> debugging?

Woops. Removed.

https://codereview.chromium.org/1995753002/diff/220001/mojo/public/c/system/p...
File mojo/public/c/system/platform_handle.h (right):

https://codereview.chromium.org/1995753002/diff/220001/mojo/public/c/system/p...
mojo/public/c/system/platform_handle.h:99: // NOTE: It is not alwawys possible
to detect if |platform_handle| is valid, so
On 2016/05/23 at 07:16:52, Anand Mistry wrote:
> s/alwawys/always

done

https://codereview.chromium.org/1995753002/diff/220001/mojo/public/c/system/p...
mojo/public/c/system/platform_handle.h:100: // this call may succeed even when
|handle| invalid (e.g. a valid file
On 2016/05/23 at 07:16:52, Anand Mistry wrote:
> s/|handle|/|platform_handle|
> 
> Also, I think you really mean |platform_handle->value|, but now I'm really nit
picking.

I do - it used to be called |handle|. In any case, I tried to clarify the docs
here.

https://codereview.chromium.org/1995753002/diff/220001/mojo/public/c/system/p...
mojo/public/c/system/platform_handle.h:107: // the Mojo handle is closed. If
this call succeeds, ownership of the underlying
On 2016/05/23 at 07:16:52, Anand Mistry wrote:
> "the mojo handle is closed"
> This sentence is redundant based on the last sentence "the mojo handle is
always closed". Also, this and the next sentence don't flow well (they both
begin with "if this call succeeds").

Fixed.

https://codereview.chromium.org/1995753002/diff/220001/mojo/public/cpp/system...
File mojo/public/cpp/system/platform_handle.h (right):

https://codereview.chromium.org/1995753002/diff/220001/mojo/public/cpp/system...
mojo/public/cpp/system/platform_handle.h:8: // Please see
"mojo/public/c/system/buffer.h" for complete documentation of the
On 2016/05/23 at 07:16:53, Anand Mistry wrote:
> s/buffer/platform_handle

done

https://codereview.chromium.org/1995753002/diff/220001/mojo/public/cpp/system...
mojo/public/cpp/system/platform_handle.h:54: #error Unsupported platform.
On 2016/05/23 at 07:16:53, Anand Mistry wrote:
> Hmmm. We don't support platforms that are non-posix and non-windows. Should we
even bother with this case? I'm pretty sure we don't consistently handle it.

Fair point - removed all these

https://codereview.chromium.org/1995753002/diff/220001/mojo/public/cpp/system...
mojo/public/cpp/system/platform_handle.h:55: #endif
On 2016/05/23 at 07:16:53, Anand Mistry wrote:
> #endif  // defined(OS_POSIX)

done

https://codereview.chromium.org/1995753002/diff/220001/mojo/public/cpp/system...
mojo/public/cpp/system/platform_handle.h:57: inline uint64_t
PlatformHandleValueFromPlatformFile(base::PlatformFile file) {
On 2016/05/23 at 07:16:53, Anand Mistry wrote:
> Shouldn't these two be in an anonymous or internal namespace?

Done

https://codereview.chromium.org/1995753002/diff/220001/mojo/public/cpp/system...
mojo/public/cpp/system/platform_handle.h:74: inline ScopedHandle
WrapPlatformFile(base::PlatformFile platform_file) {
On 2016/05/23 at 07:16:53, Anand Mistry wrote:
> These implementations are non-trivial and not performance sensitive. Please
move to a .cc file.

Done

[Anand Mistry (off Chromium), 2016-05-24 00:36:45]

LGTM with a couple of minor nits.

https://codereview.chromium.org/1995753002/diff/240001/mojo/public/c/system/p...
File mojo/public/c/system/platform_handle.h (right):

https://codereview.chromium.org/1995753002/diff/240001/mojo/public/c/system/p...
mojo/public/c/system/platform_handle.h:128: // |num_bytes|: The size of the
shared buffer in bytes.
nit: missing |flags|

https://codereview.chromium.org/1995753002/diff/240001/mojo/public/c/system/p...
mojo/public/c/system/platform_handle.h:156: //        |*platform_handle|
contains a platform shared buffer handle,
Note somewhere that |platform_handle|, |num_bytes|, and |flags| must all be
non-null.

https://codereview.chromium.org/1995753002/diff/240001/mojo/public/cpp/system...
File mojo/public/cpp/system/platform_handle.h (right):

https://codereview.chromium.org/1995753002/diff/240001/mojo/public/cpp/system...
mojo/public/cpp/system/platform_handle.h:36: const base::PlatformFile
kInvalidPlatformFile = -1;
not in scope, so no action necessary.
kInvalidPlatformFile should really be in base/files/file.h. There are at least 3
other definitions of the same constant, with the same name.

[Ken Rockot(use gerrit already), 2016-05-24 00:53:38]

https://codereview.chromium.org/1995753002/diff/240001/mojo/public/c/system/p...
File mojo/public/c/system/platform_handle.h (right):

https://codereview.chromium.org/1995753002/diff/240001/mojo/public/c/system/p...
mojo/public/c/system/platform_handle.h:128: // |num_bytes|: The size of the
shared buffer in bytes.
On 2016/05/24 at 00:36:45, Anand Mistry wrote:
> nit: missing |flags|

Fixed

https://codereview.chromium.org/1995753002/diff/240001/mojo/public/c/system/p...
mojo/public/c/system/platform_handle.h:156: //        |*platform_handle|
contains a platform shared buffer handle,
On 2016/05/24 at 00:36:45, Anand Mistry wrote:
> Note somewhere that |platform_handle|, |num_bytes|, and |flags| must all be
non-null.

Done

https://codereview.chromium.org/1995753002/diff/240001/mojo/public/cpp/system...
File mojo/public/cpp/system/platform_handle.h (right):

https://codereview.chromium.org/1995753002/diff/240001/mojo/public/cpp/system...
mojo/public/cpp/system/platform_handle.h:36: const base::PlatformFile
kInvalidPlatformFile = -1;
On 2016/05/24 at 00:36:45, Anand Mistry wrote:
> not in scope, so no action necessary.
> kInvalidPlatformFile should really be in base/files/file.h. There are at least
3 other definitions of the same constant, with the same name.

Yep, noticed that too. I'll try to post a patch later this week to clean that
up.

[Ken Rockot(use gerrit already), 2016-05-24 00:53:40]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-05-24 00:53:40]

The patchset sent to the CQ was uploaded after l-g-t-m from amistry@chromium.org
Link to the patchset: https://codereview.chromium.org/1995753002/#ps260001
(title: "nits")

[commit-bot: I haz the power, 2016-05-24 00:54:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1995753002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1995753002/260001

[commit-bot: I haz the power, 2016-05-24 01:03:10]

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

[commit-bot: I haz the power, 2016-05-24 01:03:11]

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

[Ken Rockot(use gerrit already), 2016-05-24 01:05:04]

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

[Ken Rockot(use gerrit already), 2016-05-24 01:06:00]

Description was changed from

==========
[mojo-edk] Expose portable API for platform handle wrapping

Introduces a public C API allowing embedders and apps to exchange
Mojo handles for generic platform handles or platform shared
memory handles.

R=amistry@chromium.org
BUG=
==========

to

==========
[mojo-edk] Expose portable API for platform handle wrapping

Introduces a public C API allowing embedders and apps to exchange
Mojo handles for generic platform handles or platform shared
memory handles.

BUG=
R=amistry@chromium.org
TBR=ben@chromium.org
==========

[Ken Rockot(use gerrit already), 2016-05-24 01:06:00]

rockot@chromium.org changed reviewers:
+ ben@chromium.org
- erg@chromium.org

[Ken Rockot(use gerrit already), 2016-05-24 01:06:06]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2016-05-24 01:06:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1995753002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1995753002/260001

[commit-bot: I haz the power, 2016-05-24 02:51:49]

Committed patchset #6 (id:260001)

[commit-bot: I haz the power, 2016-05-24 02:53:35]

Description was changed from

==========
[mojo-edk] Expose portable API for platform handle wrapping

Introduces a public C API allowing embedders and apps to exchange
Mojo handles for generic platform handles or platform shared
memory handles.

BUG=
R=amistry@chromium.org
TBR=ben@chromium.org
==========

to

==========
[mojo-edk] Expose portable API for platform handle wrapping

Introduces a public C API allowing embedders and apps to exchange
Mojo handles for generic platform handles or platform shared
memory handles.

BUG=
R=amistry@chromium.org
TBR=ben@chromium.org

Committed: https://crrev.com/28a287e8850ffd41da04ef0fe8f41e7a5dfb888c
Cr-Commit-Position: refs/heads/master@{#395521}
==========

[commit-bot: I haz the power, 2016-05-24 02:53:36]

Patchset 6 (id:??) landed as
https://crrev.com/28a287e8850ffd41da04ef0fe8f41e7a5dfb888c
Cr-Commit-Position: refs/heads/master@{#395521}