Replace IOSurfaceManager by directly passing IOSurface Mach ports over Chrome IPC.

Replace IOSurfaceManager by directly passing IOSurface Mach ports over Chrome IPC.

IOSurfaceManager was created as a mechanism to exchange IOSurfaces using Mach
ports, at a time when Chrome IPC was unable to broker Mach port attachments. As
part of fixing https://crbug.com/466437, Chrome IPC grew the capability to also
also send Mach ports as part of a message.

With this new capability, the custom Mach IPC channel used by IOSurfaceManager
is no longer necessary, and its complexity can be removed.

BUG=569226, 323304
Committed: https://crrev.com/5c7c3e9be27c3a79e9462e3d275780b0bc6ad559
Cr-Commit-Position: refs/heads/master@{#367474}

[Robert Sesek, 2015-12-17 14:31:55]

Patchset #1 (id:1) has been deleted

[Robert Sesek, 2015-12-18 21:44:17]

Patchset #2 (id:40001) has been deleted

[Robert Sesek, 2015-12-19 02:09:12]

Patchset #3 (id:80001) has been deleted

[Robert Sesek, 2015-12-19 02:16:26]

Description was changed from

==========
Replace IOSurfaceManager by directly passing IOSurface Mach ports over Chrome
IPC.

BUG=569226
==========

to

==========
Replace IOSurfaceManager by directly passing IOSurface Mach ports over Chrome
IPC.

IOSurfaceManager was created as a mechanism to exchange IOSurfaces using Mach
ports, at a time when Chrome IPC was unable to broker Mach port attachments. As
part of fixing https://crbug.com/466437, Chrome IPC grew the capability to also
also send Mach ports as part of a message.

With this new capability, the custom Mach IPC channel used by IOSurfaceManager
is no longer necessary, and its complexity can be removed.

BUG=569226
==========

[Robert Sesek, 2015-12-19 02:19:31]

Description was changed from

==========
Replace IOSurfaceManager by directly passing IOSurface Mach ports over Chrome
IPC.

IOSurfaceManager was created as a mechanism to exchange IOSurfaces using Mach
ports, at a time when Chrome IPC was unable to broker Mach port attachments. As
part of fixing https://crbug.com/466437, Chrome IPC grew the capability to also
also send Mach ports as part of a message.

With this new capability, the custom Mach IPC channel used by IOSurfaceManager
is no longer necessary, and its complexity can be removed.

BUG=569226
==========

to

==========
Replace IOSurfaceManager by directly passing IOSurface Mach ports over Chrome
IPC.

IOSurfaceManager was created as a mechanism to exchange IOSurfaces using Mach
ports, at a time when Chrome IPC was unable to broker Mach port attachments. As
part of fixing https://crbug.com/466437, Chrome IPC grew the capability to also
also send Mach ports as part of a message.

With this new capability, the custom Mach IPC channel used by IOSurfaceManager
is no longer necessary, and its complexity can be removed.

BUG=569226,323304
==========

[Robert Sesek, 2015-12-19 02:19:31]

rsesek@chromium.org changed reviewers:
+ ccameron@chromium.org, erikchen@chromium.org, reveman@chromium.org

[Robert Sesek, 2015-12-20 20:28:36]

Patchset #4 (id:120001) has been deleted

[Robert Sesek, 2015-12-21 15:30:03]

ccameron, reveman: Please review
erikchen: //ipc changes

This CL looks substantially larger than it is. Most of the changes are removals.

[ccameron, 2015-12-21 19:30:14]

This lgtm. We will probably want to add some end-to-end tests (like, the full
multiprocess delegated renderer) to ensure that we don't hold ports open
indefinitely.

https://codereview.chromium.org/1532813002/diff/140001/ui/gfx/gpu_memory_buff...
File ui/gfx/gpu_memory_buffer.h (right):

https://codereview.chromium.org/1532813002/diff/140001/ui/gfx/gpu_memory_buff...
ui/gfx/gpu_memory_buffer.h:49: base::mac::ScopedRefCountedMachSendRight
mach_port;
The existence of this port will result in the IOSurface being seen as "in use"
for as long as any GpuMemoryBufferHandle to that IOSurface exists in any
process.

This appears to be okay, but we will need to be careful to not hold
GpuMemoryBufferHandles around any longer than they are needed (e.g, if the
renderer which owns a GMB were to cache a handle to it, the result would be that
it would always look that the GMB were in use by the system).

[erikchen, 2015-12-21 20:02:55]

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

[erikchen, 2015-12-21 20:02:57]

+tsepez, in case he wants to chime in on my comments on the Chrome IPC stack.

https://codereview.chromium.org/1532813002/diff/140001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/1532813002/diff/140001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:1056: bool
ParamTraits<base::mac::ScopedRefCountedMachSendRight>::Read(
The Chrome IPC stack doesn't have the right semantics to properly support
passing scoped objects.

The code that you've written will work, but implicitly relies on the assumption
that translation only happens once. Attempting to translate a message twice will
result in over-releasing the underlying primitive. Most messages are only
translated once, but some are translated more than once, and more importantly,
there is no enforcement mechanism to ensure single-translation.

I would suggest using MachPortMac directly as the parameter to the IPC message.

[reveman, 2015-12-21 20:11:16]

thanks! lgtm

https://codereview.chromium.org/1532813002/diff/140001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_factory_io_surface.h (right):

https://codereview.chromium.org/1532813002/diff/140001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_io_surface.h:69: IOSurfaceMap
io_surfaces_;
FYI, this map could now be removed in favor of passing a mach_port with
CreateImageCHROMIUM as that will fix the problem where an IOSurface can become
invalid if the GPU process crashes. We can do that as a follow up but can you
add the following TODO here for now?

TOOD(reveman): Remove |io_surfaces_| and allow IOSurface backed GMBs to be used
with any GPU process by passing a mach_port to CreateImageCHROMIUM.

[Robert Sesek, 2015-12-21 20:33:54]

https://codereview.chromium.org/1532813002/diff/140001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/1532813002/diff/140001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:1056: bool
ParamTraits<base::mac::ScopedRefCountedMachSendRight>::Read(
On 2015/12/21 20:02:57, erikchen wrote:
> The Chrome IPC stack doesn't have the right semantics to properly support
> passing scoped objects.
> 
> The code that you've written will work, but implicitly relies on the
assumption
> that translation only happens once. Attempting to translate a message twice
will
> result in over-releasing the underlying primitive. Most messages are only
> translated once, but some are translated more than once, and more importantly,
> there is no enforcement mechanism to ensure single-translation.
> 
> I would suggest using MachPortMac directly as the parameter to the IPC
message. 

The issue here is that GpuMemoryBufferHandle is both an IPC transit type and an
object used in other ways. Not having the ownership semantics expressed in the
object form I think would make the type extremely easy to misuse (and
underscores ccameron's point). One thing I could do, though, is not offer this
type in base::, but make it more narrowly named for passing IOSurface Mach ports
(e.g., define it in //ui/gfx), where usage may be easier to control. (I also
just have some reservations about adding this type to base:: anyways). WDYT?

[erikchen, 2015-12-21 21:06:16]

On 2015/12/21 20:33:54, Robert Sesek wrote:
>
https://codereview.chromium.org/1532813002/diff/140001/ipc/ipc_message_utils.cc
> File ipc/ipc_message_utils.cc (right):
> 
>
https://codereview.chromium.org/1532813002/diff/140001/ipc/ipc_message_utils....
> ipc/ipc_message_utils.cc:1056: bool
> ParamTraits<base::mac::ScopedRefCountedMachSendRight>::Read(
> On 2015/12/21 20:02:57, erikchen wrote:
> > The Chrome IPC stack doesn't have the right semantics to properly support
> > passing scoped objects.
> > 
> > The code that you've written will work, but implicitly relies on the
> assumption
> > that translation only happens once. Attempting to translate a message twice
> will
> > result in over-releasing the underlying primitive. Most messages are only
> > translated once, but some are translated more than once, and more
importantly,
> > there is no enforcement mechanism to ensure single-translation.
> > 
> > I would suggest using MachPortMac directly as the parameter to the IPC
> message. 
> 
> The issue here is that GpuMemoryBufferHandle is both an IPC transit type and
an
> object used in other ways. Not having the ownership semantics expressed in the
> object form I think would make the type extremely easy to misuse (and
> underscores ccameron's point). One thing I could do, though, is not offer this
> type in base::, but make it more narrowly named for passing IOSurface Mach
ports
> (e.g., define it in //ui/gfx), where usage may be easier to control. (I also
> just have some reservations about adding this type to base:: anyways). WDYT?

Moving the definition into a less-exposed module makes your solution more
palatable, but I still prefer matching the semantics of existing resources.

1. Passing a parameter without ownership semantics through Chrome IPC makes it
possible to leak the resource.
2. Passing a parameter with ownership semantics through Chrome IPC makes it
possible to release a resource that is still in use. 

Both problems are serious, and can be difficult to diagnose when they occur.

I prefer (1) because that's how all other resources are passed through Chrome
IPC (fds, HANDLEs, etc.). 

If you do think that passing a Scoped Mach right as a parameter is the correct
solution, you might as well discuss it directly with an OWNER of ipc/, since
you'll need an l-g-t-m from them anyways.

[Robert Sesek, 2015-12-21 22:00:05]

On 2015/12/21 21:06:16, erikchen wrote:
> On 2015/12/21 20:33:54, Robert Sesek wrote:
> >
>
https://codereview.chromium.org/1532813002/diff/140001/ipc/ipc_message_utils.cc
> > File ipc/ipc_message_utils.cc (right):
> > 
> >
>
https://codereview.chromium.org/1532813002/diff/140001/ipc/ipc_message_utils....
> > ipc/ipc_message_utils.cc:1056: bool
> > ParamTraits<base::mac::ScopedRefCountedMachSendRight>::Read(
> > On 2015/12/21 20:02:57, erikchen wrote:
> > > The Chrome IPC stack doesn't have the right semantics to properly support
> > > passing scoped objects.
> > > 
> > > The code that you've written will work, but implicitly relies on the
> > assumption
> > > that translation only happens once. Attempting to translate a message
twice
> > will
> > > result in over-releasing the underlying primitive. Most messages are only
> > > translated once, but some are translated more than once, and more
> importantly,
> > > there is no enforcement mechanism to ensure single-translation.
> > > 
> > > I would suggest using MachPortMac directly as the parameter to the IPC
> > message. 
> > 
> > The issue here is that GpuMemoryBufferHandle is both an IPC transit type and
> an
> > object used in other ways. Not having the ownership semantics expressed in
the
> > object form I think would make the type extremely easy to misuse (and
> > underscores ccameron's point). One thing I could do, though, is not offer
this
> > type in base::, but make it more narrowly named for passing IOSurface Mach
> ports
> > (e.g., define it in //ui/gfx), where usage may be easier to control. (I also
> > just have some reservations about adding this type to base:: anyways). WDYT?
> 
> Moving the definition into a less-exposed module makes your solution more
> palatable, but I still prefer matching the semantics of existing resources.
> 
> 1. Passing a parameter without ownership semantics through Chrome IPC makes it
> possible to leak the resource.
> 2. Passing a parameter with ownership semantics through Chrome IPC makes it
> possible to release a resource that is still in use. 
> 
> Both problems are serious, and can be difficult to diagnose when they occur.
> 
> I prefer (1) because that's how all other resources are passed through Chrome
> IPC (fds, HANDLEs, etc.). 
> 
> If you do think that passing a Scoped Mach right as a parameter is the correct
> solution, you might as well discuss it directly with an OWNER of ipc/, since
> you'll need an l-g-t-m from them anyways.

I don't see how (2) is possible given the scoper as-written. On copy and
assignment, the actual number of extant send rights is incremented in the
kernel, and on destruction it is decremented. Mach ports do not behave the same
as fds or HANDLEs, since they have a reference count independent of their name,
so it is possible to transfer a properly-accounted right without losing the
name. On some level, GpuMemoryBufferHandle needs to be able to convey ownership
transfer to IPC (see
https://code.google.com/p/chromium/codesearch#chromium/src/content/gpu/gpu_ch....
I think using the ref-counting semantics is superior to SharedMemoryHandle's
auto_close_/ownership_passes_to_ipc_ approach.

[erikchen, 2015-12-21 23:00:26]

On 2015/12/21 22:00:05, Robert Sesek wrote:
> On 2015/12/21 21:06:16, erikchen wrote:
> > On 2015/12/21 20:33:54, Robert Sesek wrote:
> > >
> >
>
https://codereview.chromium.org/1532813002/diff/140001/ipc/ipc_message_utils.cc
> > > File ipc/ipc_message_utils.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1532813002/diff/140001/ipc/ipc_message_utils....
> > > ipc/ipc_message_utils.cc:1056: bool
> > > ParamTraits<base::mac::ScopedRefCountedMachSendRight>::Read(
> > > On 2015/12/21 20:02:57, erikchen wrote:
> > > > The Chrome IPC stack doesn't have the right semantics to properly
support
> > > > passing scoped objects.
> > > > 
> > > > The code that you've written will work, but implicitly relies on the
> > > assumption
> > > > that translation only happens once. Attempting to translate a message
> twice
> > > will
> > > > result in over-releasing the underlying primitive. Most messages are
only
> > > > translated once, but some are translated more than once, and more
> > importantly,
> > > > there is no enforcement mechanism to ensure single-translation.
> > > > 
> > > > I would suggest using MachPortMac directly as the parameter to the IPC
> > > message. 
> > > 
> > > The issue here is that GpuMemoryBufferHandle is both an IPC transit type
and
> > an
> > > object used in other ways. Not having the ownership semantics expressed in
> the
> > > object form I think would make the type extremely easy to misuse (and
> > > underscores ccameron's point). One thing I could do, though, is not offer
> this
> > > type in base::, but make it more narrowly named for passing IOSurface Mach
> > ports
> > > (e.g., define it in //ui/gfx), where usage may be easier to control. (I
also
> > > just have some reservations about adding this type to base:: anyways).
WDYT?
> > 
> > Moving the definition into a less-exposed module makes your solution more
> > palatable, but I still prefer matching the semantics of existing resources.
> > 
> > 1. Passing a parameter without ownership semantics through Chrome IPC makes
it
> > possible to leak the resource.
> > 2. Passing a parameter with ownership semantics through Chrome IPC makes it
> > possible to release a resource that is still in use. 
> > 
> > Both problems are serious, and can be difficult to diagnose when they occur.
> > 
> > I prefer (1) because that's how all other resources are passed through
Chrome
> > IPC (fds, HANDLEs, etc.). 
> > 
> > If you do think that passing a Scoped Mach right as a parameter is the
correct
> > solution, you might as well discuss it directly with an OWNER of ipc/, since
> > you'll need an l-g-t-m from them anyways.
> 
> I don't see how (2) is possible given the scoper as-written. On copy and
> assignment, the actual number of extant send rights is incremented in the
> kernel, and on destruction it is decremented. Mach ports do not behave the
same
> as fds or HANDLEs, since they have a reference count independent of their
name,
> so it is possible to transfer a properly-accounted right without losing the
> name. On some level, GpuMemoryBufferHandle needs to be able to convey
ownership
> transfer to IPC (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/gpu/gpu_ch....
> I think using the ref-counting semantics is superior to SharedMemoryHandle's
> auto_close_/ownership_passes_to_ipc_ approach.

ipc/ lgtm with nits.

I'll provide an example of how (2) is possible, but I no longer think it is
likely to happen. 

Process A sends a Scoped Mach port (P) to Process B. This is sent via an
indirection as a message to a newly created mach port in Process B. If Process B
already has a send right to the Mach port (P'), then the reference count to P'
is incremented (let's say to 2), but no new name is created. Your newly added
method ParamTraits<base::mac::ScopedRefCountedMachSendRight>::Read() creates a
scoped object to own one reference to P'. In a sense, this Read() method is
passing ownership from the Chrome IPC stack to the receiver of the IPC message.

It's theoretically possible for Read() to be called twice. In that case, two
instances of ScopedRefCountedMachSendRight will be created, and when those
objects are destroyed, both references to P' will be released, and P' will be
marked as invalid, even though there may have been another entity with a
reference to P'. 

There are cases where parameters of a Message are translated more than once, but
this is pretty rare, and the examples I found were all for primitives (int32,
etc.), so I'm less worried that a consumer of an IPC::Message might try to
translate the Scoped Mach Port parameter twice.

[erikchen, 2015-12-21 23:00:38]

https://codereview.chromium.org/1532813002/diff/140001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/1532813002/diff/140001/ipc/ipc_message_utils....
ipc/ipc_message_utils.h:940: static bool Read(const Message* m,
base::PickleIterator* iter, param_type* r);
Please add a comment indicating that Read() takes ownership of the underlying
Mach Send right, and that it *must* be called exactly once on any given IPC
message parameter.

[Robert Sesek, 2015-12-22 23:30:39]

Patchset #5 (id:160001) has been deleted

[Robert Sesek, 2015-12-23 14:43:29]

Patchset #6 (id:200001) has been deleted

[Robert Sesek, 2015-12-23 15:45:14]

On 2015/12/21 23:00:26, erikchen wrote:
> I'll provide an example of how (2) is possible, but I no longer think it is
> likely to happen. 
> 
> Process A sends a Scoped Mach port (P) to Process B. This is sent via an
> indirection as a message to a newly created mach port in Process B. If Process
B
> already has a send right to the Mach port (P'), then the reference count to P'
> is incremented (let's say to 2), but no new name is created. Your newly added
> method ParamTraits<base::mac::ScopedRefCountedMachSendRight>::Read() creates a
> scoped object to own one reference to P'. In a sense, this Read() method is
> passing ownership from the Chrome IPC stack to the receiver of the IPC
message.
> 
> It's theoretically possible for Read() to be called twice. In that case, two
> instances of ScopedRefCountedMachSendRight will be created, and when those
> objects are destroyed, both references to P' will be released, and P' will be
> marked as invalid, even though there may have been another entity with a
> reference to P'. 
> 
> There are cases where parameters of a Message are translated more than once,
but
> this is pretty rare, and the examples I found were all for primitives (int32,
> etc.), so I'm less worried that a consumer of an IPC::Message might try to
> translate the Scoped Mach Port parameter twice.

Right, I guess I don't understand when a param will be ::Read() twice. Do you
know when this happens? I don't think there's a good way otherwise to guard
against your scenario, as the kernel coalesces all rights of a port to the same
name in a task (i.e. you can't have 2 send rights on name Q and 1 on name R,
both referring to the receive port P).

https://codereview.chromium.org/1532813002/diff/140001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_factory_io_surface.h (right):

https://codereview.chromium.org/1532813002/diff/140001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_io_surface.h:69: IOSurfaceMap
io_surfaces_;
On 2015/12/21 20:11:16, reveman wrote:
> FYI, this map could now be removed in favor of passing a mach_port with
> CreateImageCHROMIUM as that will fix the problem where an IOSurface can become
> invalid if the GPU process crashes. We can do that as a follow up but can you
> add the following TODO here for now?
> 
> TOOD(reveman): Remove |io_surfaces_| and allow IOSurface backed GMBs to be
used
> with any GPU process by passing a mach_port to CreateImageCHROMIUM.

Done.

https://codereview.chromium.org/1532813002/diff/140001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/1532813002/diff/140001/ipc/ipc_message_utils....
ipc/ipc_message_utils.h:940: static bool Read(const Message* m,
base::PickleIterator* iter, param_type* r);
On 2015/12/21 23:00:38, erikchen wrote:
> Please add a comment indicating that Read() takes ownership of the underlying
> Mach Send right, and that it *must* be called exactly once on any given IPC
> message parameter.

Done.

[Robert Sesek, 2015-12-23 15:45:33]

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

[Robert Sesek, 2015-12-23 15:45:33]

+avi for //content and //ui OWNERS

[Avi (use Gerrit), 2015-12-24 00:42:09]

lgtm

approved for the sheer amount of code deleted

[Robert Sesek, 2016-01-04 21:55:03]

tsepez: Happy new year, please review :-)

[Tom Sepez, 2016-01-04 21:58:23]

Deferring to erik's LGTM.

[Robert Sesek, 2016-01-05 02:15:41]

The CQ bit was checked by rsesek@chromium.org

[Robert Sesek, 2016-01-05 02:15:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikchen@chromium.org, reveman@chromium.org, ccameron@chromium.org,
tsepez@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1532813002/#ps240001
(title: "Rebase")

[commit-bot: I haz the power, 2016-01-05 02:16:46]

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

[commit-bot: I haz the power, 2016-01-05 03:37:20]

Description was changed from

==========
Replace IOSurfaceManager by directly passing IOSurface Mach ports over Chrome
IPC.

IOSurfaceManager was created as a mechanism to exchange IOSurfaces using Mach
ports, at a time when Chrome IPC was unable to broker Mach port attachments. As
part of fixing https://crbug.com/466437, Chrome IPC grew the capability to also
also send Mach ports as part of a message.

With this new capability, the custom Mach IPC channel used by IOSurfaceManager
is no longer necessary, and its complexity can be removed.

BUG=569226,323304
==========

to

==========
Replace IOSurfaceManager by directly passing IOSurface Mach ports over Chrome
IPC.

IOSurfaceManager was created as a mechanism to exchange IOSurfaces using Mach
ports, at a time when Chrome IPC was unable to broker Mach port attachments. As
part of fixing https://crbug.com/466437, Chrome IPC grew the capability to also
also send Mach ports as part of a message.

With this new capability, the custom Mach IPC channel used by IOSurfaceManager
is no longer necessary, and its complexity can be removed.

BUG=569226,323304
==========

[commit-bot: I haz the power, 2016-01-05 03:37:21]

Committed patchset #7 (id:240001)

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

Description was changed from

==========
Replace IOSurfaceManager by directly passing IOSurface Mach ports over Chrome
IPC.

IOSurfaceManager was created as a mechanism to exchange IOSurfaces using Mach
ports, at a time when Chrome IPC was unable to broker Mach port attachments. As
part of fixing https://crbug.com/466437, Chrome IPC grew the capability to also
also send Mach ports as part of a message.

With this new capability, the custom Mach IPC channel used by IOSurfaceManager
is no longer necessary, and its complexity can be removed.

BUG=569226,323304
==========

to

==========
Replace IOSurfaceManager by directly passing IOSurface Mach ports over Chrome
IPC.

IOSurfaceManager was created as a mechanism to exchange IOSurfaces using Mach
ports, at a time when Chrome IPC was unable to broker Mach port attachments. As
part of fixing https://crbug.com/466437, Chrome IPC grew the capability to also
also send Mach ports as part of a message.

With this new capability, the custom Mach IPC channel used by IOSurfaceManager
is no longer necessary, and its complexity can be removed.

BUG=569226,323304

Committed: https://crrev.com/5c7c3e9be27c3a79e9462e3d275780b0bc6ad559
Cr-Commit-Position: refs/heads/master@{#367474}
==========

[commit-bot: I haz the power, 2016-01-05 03:38:06]

Patchset 7 (id:??) landed as
https://crrev.com/5c7c3e9be27c3a79e9462e3d275780b0bc6ad559
Cr-Commit-Position: refs/heads/master@{#367474}