mus: Add support for sending IOSurface mach_ports over mojo.

mus: Add support for sending IOSurface mach_ports over mojo.

BUG=643746
Committed: https://crrev.com/61b712594bab345c5b0f2bc1f44ee3cb1eaec184
Cr-Commit-Position: refs/heads/master@{#436161}

[sadrul, 2016-12-01 02:20:48]

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

[commit-bot: I haz the power, 2016-12-01 02:21:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2016-12-01 02:25:14]

sadrul@chromium.org changed reviewers:
+ rockot@chromium.org, tsepez@chromium.org

[sadrul, 2016-12-01 02:25:14]

rockot@ please review the mojo/ changes.

tsepez@ please review the gfx/mojo changes.

[commit-bot: I haz the power, 2016-12-01 02:49:03]

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

[commit-bot: I haz the power, 2016-12-01 02:49:04]

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

[sadrul, 2016-12-01 02:49:52]

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

[commit-bot: I haz the power, 2016-12-01 02:50:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2016-12-01 03:14:31]

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

[commit-bot: I haz the power, 2016-12-01 03:14:57]

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, 2016-12-01 05:08:54]

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

[commit-bot: I haz the power, 2016-12-01 05:08:55]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-12-01 16:32:52]

Description was changed from

==========
mus: Add support for sending IOSurface mach_ports over mojo.

BUG=643746
==========

to

==========
mus: Add support for sending IOSurface mach_ports over mojo.

BUG=643746
==========

[Tom Sepez, 2016-12-01 16:32:52]

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

[Tom Sepez, 2016-12-01 16:33:24]

mojom itself looks good, but +rsesek for all things mach port.

[Ken Rockot(use gerrit already), 2016-12-01 17:57:01]

LGTM and +sammc FYI since he's been thinking about introducing a generic scoped
handle union type (to base?) which encapsulates various types of platform
handles and can be cleanly typemapped to some common mojom type.

[Robert Sesek, 2016-12-01 18:51:34]

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

https://codereview.chromium.org/2537423004/diff/40001/mojo/public/cpp/system/...
mojo/public/cpp/system/platform_handle.cc:137:
mach_port_mod_refs(mach_task_self(), port, MACH_PORT_RIGHT_SEND, 1);
I can't find it in the code, but maybe I'm not looking in the right place… does
ScopedHandle balance this with a -1 if it is used without being transferred out
of process?

[Ken Rockot(use gerrit already), 2016-12-01 18:56:54]

It does not appear to. I don't know much about the Mach port support
details, but perhaps this is a bug.

Here's what currently happens when a ScopedHandle goes away if it's holding
a mach_port:

https://cs.chromium.org/chromium/src/mojo/edk/embedder/platform_handle.cc?rcl...

We apparently don't do any ref inc or dec when wrapping mach ports.

On Thu, Dec 1, 2016 at 10:51 AM, <rsesek@chromium.org> wrote:

>
> https://codereview.chromium.org/2537423004/diff/40001/
> mojo/public/cpp/system/platform_handle.cc
> File mojo/public/cpp/system/platform_handle.cc (right):
>
> https://codereview.chromium.org/2537423004/diff/40001/
> mojo/public/cpp/system/platform_handle.cc#newcode137
> mojo/public/cpp/system/platform_handle.cc:137:
> mach_port_mod_refs(mach_task_self(), port, MACH_PORT_RIGHT_SEND, 1);
> I can't find it in the code, but maybe I'm not looking in the right
> place… does ScopedHandle balance this with a -1 if it is used without
> being transferred out of process?
>
> https://codereview.chromium.org/2537423004/
>

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

[Ken Rockot(use gerrit already), 2016-12-01 19:01:49]

On 2016/12/01 at 18:56:54, Ken Rockot wrote:
> It does not appear to. I don't know much about the Mach port support
> details, but perhaps this is a bug.
> 
> Here's what currently happens when a ScopedHandle goes away if it's holding
> a mach_port:
> 
>
https://cs.chromium.org/chromium/src/mojo/edk/embedder/platform_handle.cc?rcl...
> 
> We apparently don't do any ref inc or dec when wrapping mach ports.
> 
> On Thu, Dec 1, 2016 at 10:51 AM, <rsesek@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/2537423004/diff/40001/
> > mojo/public/cpp/system/platform_handle.cc
> > File mojo/public/cpp/system/platform_handle.cc (right):
> >
> > https://codereview.chromium.org/2537423004/diff/40001/
> > mojo/public/cpp/system/platform_handle.cc#newcode137
> > mojo/public/cpp/system/platform_handle.cc:137:
> > mach_port_mod_refs(mach_task_self(), port, MACH_PORT_RIGHT_SEND, 1);
> > I can't find it in the code, but maybe I'm not looking in the right
> > place… does ScopedHandle balance this with a -1 if it is used without
> > being transferred out of process?
> >
> > https://codereview.chromium.org/2537423004/
> >
> 
> -- 
> 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.
> 

If mach ports are fundamentally a ref-counted object at the system layer, then I
think clearly a scoped wrapper for them should just inc and dec ref, rather than
assume ownership and deallocate on destruciton. Is this accurate?

[Robert Sesek, 2016-12-01 20:11:59]

On 2016/12/01 19:01:49, Ken Rockot wrote:
> On 2016/12/01 at 18:56:54, Ken Rockot wrote:
> > It does not appear to. I don't know much about the Mach port support
> > details, but perhaps this is a bug.
> > 
> > Here's what currently happens when a ScopedHandle goes away if it's holding
> > a mach_port:
> > 
> >
>
https://cs.chromium.org/chromium/src/mojo/edk/embedder/platform_handle.cc?rcl...
> > 
> > We apparently don't do any ref inc or dec when wrapping mach ports.
> > 

Thanks! That's sufficient, since mach_port_deallocate() is a specialized form of
mod_refs(-1) that handles some edge cases with port names.

> > On Thu, Dec 1, 2016 at 10:51 AM, <mailto:rsesek@chromium.org> wrote:
> > 
> > >
> > > https://codereview.chromium.org/2537423004/diff/40001/
> > > mojo/public/cpp/system/platform_handle.cc
> > > File mojo/public/cpp/system/platform_handle.cc (right):
> > >
> > > https://codereview.chromium.org/2537423004/diff/40001/
> > > mojo/public/cpp/system/platform_handle.cc#newcode137
> > > mojo/public/cpp/system/platform_handle.cc:137:
> > > mach_port_mod_refs(mach_task_self(), port, MACH_PORT_RIGHT_SEND, 1);
> > > I can't find it in the code, but maybe I'm not looking in the right
> > > place… does ScopedHandle balance this with a -1 if it is used without
> > > being transferred out of process?
> > >
> > > https://codereview.chromium.org/2537423004/
> > >
> > 
> > -- 
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > 
> 
> If mach ports are fundamentally a ref-counted object at the system layer, then
I
> think clearly a scoped wrapper for them should just inc and dec ref, rather
than
> assume ownership and deallocate on destruciton. Is this accurate?

Ports are reference counted by the kernel, yes. But I think a lot of our scopers
assume ownership and then release it on destruction, so I don't see a problem
with that, but it wouldn't be incorrect to do balancing as you suggest, either.

[sadrul, 2016-12-02 00:44:14]

On 2016/12/01 20:11:59, Robert Sesek wrote:
> On 2016/12/01 19:01:49, Ken Rockot wrote:
> > On 2016/12/01 at 18:56:54, Ken Rockot wrote:
> > > It does not appear to. I don't know much about the Mach port support
> > > details, but perhaps this is a bug.
> > > 
> > > Here's what currently happens when a ScopedHandle goes away if it's
holding
> > > a mach_port:
> > > 
> > >
> >
>
https://cs.chromium.org/chromium/src/mojo/edk/embedder/platform_handle.cc?rcl...
> > > 
> > > We apparently don't do any ref inc or dec when wrapping mach ports.
> > > 
> 
> Thanks! That's sufficient, since mach_port_deallocate() is a specialized form
of
> mod_refs(-1) that handles some edge cases with port names.

Does this mean this CL is doing the right thing, or is it missing something?

[Robert Sesek, 2016-12-02 18:11:17]

On 2016/12/02 00:44:14, sadrul wrote:
> On 2016/12/01 20:11:59, Robert Sesek wrote:
> > On 2016/12/01 19:01:49, Ken Rockot wrote:
> > > On 2016/12/01 at 18:56:54, Ken Rockot wrote:
> > > > It does not appear to. I don't know much about the Mach port support
> > > > details, but perhaps this is a bug.
> > > > 
> > > > Here's what currently happens when a ScopedHandle goes away if it's
> holding
> > > > a mach_port:
> > > > 
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/mojo/edk/embedder/platform_handle.cc?rcl...
> > > > 
> > > > We apparently don't do any ref inc or dec when wrapping mach ports.
> > > > 
> > 
> > Thanks! That's sufficient, since mach_port_deallocate() is a specialized
form
> of
> > mod_refs(-1) that handles some edge cases with port names.
> 
> Does this mean this CL is doing the right thing, or is it missing something?

Yes, I think this is doing the right thing (assuming that we only ever transfer
SEND rights and not RECEIVE, in which case mach_port_deallocate() is not the
right choice). LGTM.

I'm not sure if rockot@ wants to change the behavior to use the kernel refcount,
but that can be done separately.

[sadrul, 2016-12-03 02:38:09]

The CQ bit was checked by sadrul@chromium.org

[commit-bot: I haz the power, 2016-12-03 02:38:34]

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, 2016-12-03 03:42:04]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480732689727200,
"parent_rev": "a1419c28616a50be0ef3d5da315ae23f92b94ca4", "commit_rev":
"6f5347f19ef6b3894b68a511710e986d8c6f4ce4"}

[commit-bot: I haz the power, 2016-12-03 03:42:26]

Description was changed from

==========
mus: Add support for sending IOSurface mach_ports over mojo.

BUG=643746
==========

to

==========
mus: Add support for sending IOSurface mach_ports over mojo.

BUG=643746
==========

[commit-bot: I haz the power, 2016-12-03 03:42:27]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-03 03:45:28]

Description was changed from

==========
mus: Add support for sending IOSurface mach_ports over mojo.

BUG=643746
==========

to

==========
mus: Add support for sending IOSurface mach_ports over mojo.

BUG=643746

Committed: https://crrev.com/61b712594bab345c5b0f2bc1f44ee3cb1eaec184
Cr-Commit-Position: refs/heads/master@{#436161}
==========

[commit-bot: I haz the power, 2016-12-03 03:45:30]

Patchset 3 (id:??) landed as
https://crrev.com/61b712594bab345c5b0f2bc1f44ee3cb1eaec184
Cr-Commit-Position: refs/heads/master@{#436161}