Implement temporary reference assignment with DisplayCompositor.

Implement temporary reference assignment with DisplayCompositor.

This CL implements the temporary reference assignment via
DisplayCompositor. When DisplayCompositorClient finds out a new surface
has been created, it tries to find the CompositorFrameSink that will
embed the new surface. This is done in mus-ws as it is both fully
trusted and has the full ServerWindow heirarchy.

If a parent is found an IPC is sent back to DisplayCompositor to assign
ownership of the temporary reference. If the CompositorFrameSink that
owns a temporary reference is invalidated, the temporary reference is
removed because the owner can't add a real reference after invalidation.

If no parent is found, for example if the ServerWindow no longer exists
or isn't parented to something visible, then an IPC is sent back to
DisplayCompositor to drop the temporary reference. In this case mus-ws
doesn't expect anything to embed the new surface.

BUG=683738
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2715663007
Cr-Commit-Position: refs/heads/master@{#454389}
Committed: https://chromium.googlesource.com/chromium/src/+/5c97d3ea18174c41ad3ab15f0a9c5fe14717829d

[kylechar, 2017-02-24 16:40:05]

Description was changed from

==========
Implement temporary reference assignment with DisplayCompositor.

This CL implements the temporary reference assignment via
DisplayCompositor. When DisplayCompositorClient finds out a new surface
has been created, it tries to find the CompositorFrameSink that will
embed the new surface. This is done in mus-ws as it is both fully
trusted and has the full ServerWindow heirarchy.

If a parent is found an IPC is sent back to DisplayCompositor to assign
ownership of the temporary reference. If the CompositorFrameSink that
owns a temporary reference is invalidated, the temporary reference is
removed because the owner can't add a real reference after invalidation.

If no parent is found, for example if the ServerWindow no longer exists
or isn't parented to something visible, then an IPC is sent back to
DisplayCompositor to drop the temporary reference. In this case mus-ws
doesn't expect anything to embed the new surface.

BUG=683738
==========

to

==========
Implement temporary reference assignment with DisplayCompositor.

This CL implements the temporary reference assignment via
DisplayCompositor. When DisplayCompositorClient finds out a new surface
has been created, it tries to find the CompositorFrameSink that will
embed the new surface. This is done in mus-ws as it is both fully
trusted and has the full ServerWindow heirarchy.

If a parent is found an IPC is sent back to DisplayCompositor to assign
ownership of the temporary reference. If the CompositorFrameSink that
owns a temporary reference is invalidated, the temporary reference is
removed because the owner can't add a real reference after invalidation.

If no parent is found, for example if the ServerWindow no longer exists
or isn't parented to something visible, then an IPC is sent back to
DisplayCompositor to drop the temporary reference. In this case mus-ws
doesn't expect anything to embed the new surface.

BUG=683738
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[kylechar, 2017-02-24 17:05:19]

kylechar@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2017-02-24 18:07:38]

lgtm + nits.

https://codereview.chromium.org/2715663007/diff/20001/services/ui/surfaces/di...
File services/ui/surfaces/display_compositor.cc (right):

https://codereview.chromium.org/2715663007/diff/20001/services/ui/surfaces/di...
services/ui/surfaces/display_compositor.cc:172: // reference to the new surface.
With surface synchronization this might not
More details please? Why won't this be necessary, again? Is it because a parent
may have already claimed ownership?

https://codereview.chromium.org/2715663007/diff/20001/services/ui/ws/window_s...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2715663007/diff/20001/services/ui/ws/window_s...
services/ui/ws/window_server.cc:609: ServerWindow*
WindowServer::FindCompositorFrameSinkParent(
nit: TODO that we should plumb a bool to ServerWindow to indicate that they will
create CompositorFrames / CompositorFrameSinks.

https://codereview.chromium.org/2715663007/diff/20001/services/ui/ws/window_s...
services/ui/ws/window_server.cc:825: ServerWindow* parent =
FindCompositorFrameSinkParent(window);
nit: compositing_parent

https://codereview.chromium.org/2715663007/diff/20001/services/ui/ws/window_s...
services/ui/ws/window_server.cc:828:
frame_sink_manager->ClaimTemporaryReference(surface_info.id());
Add a comment here about what this is doing.

[kylechar, 2017-02-28 18:05:54]

Patchset #3 (id:40001) has been deleted

[kylechar, 2017-02-28 18:07:18]

https://codereview.chromium.org/2715663007/diff/20001/services/ui/surfaces/di...
File services/ui/surfaces/display_compositor.cc (right):

https://codereview.chromium.org/2715663007/diff/20001/services/ui/surfaces/di...
services/ui/surfaces/display_compositor.cc:172: // reference to the new surface.
With surface synchronization this might not
On 2017/02/24 18:07:38, Fady Samuel wrote:
> More details please? Why won't this be necessary, again? Is it because a
parent
> may have already claimed ownership?

Done.

https://codereview.chromium.org/2715663007/diff/20001/services/ui/ws/window_s...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2715663007/diff/20001/services/ui/ws/window_s...
services/ui/ws/window_server.cc:609: ServerWindow*
WindowServer::FindCompositorFrameSinkParent(
On 2017/02/24 18:07:38, Fady Samuel wrote:
> nit: TODO that we should plumb a bool to ServerWindow to indicate that they
will
> create CompositorFrames / CompositorFrameSinks.

Done.

https://codereview.chromium.org/2715663007/diff/20001/services/ui/ws/window_s...
services/ui/ws/window_server.cc:825: ServerWindow* parent =
FindCompositorFrameSinkParent(window);
On 2017/02/24 18:07:38, Fady Samuel wrote:
> nit: compositing_parent

Done.

https://codereview.chromium.org/2715663007/diff/20001/services/ui/ws/window_s...
services/ui/ws/window_server.cc:828:
frame_sink_manager->ClaimTemporaryReference(surface_info.id());
On 2017/02/24 18:07:38, Fady Samuel wrote:
> Add a comment here about what this is doing.

Done.

[Fady Samuel, 2017-02-28 18:10:35]

lgtm

[kylechar, 2017-02-28 18:10:59]

kylechar@chromium.org changed reviewers:
+ jbauman@chromium.org, sky@chromium.org, tsepez@chromium.org

[kylechar, 2017-02-28 18:11:00]

+sky for services/ui
+jbauman for cc/surfaces and components/display_compositor/
+tsepez for mojoms

[Tom Sepez, 2017-02-28 18:46:58]

https://codereview.chromium.org/2715663007/diff/60001/cc/ipc/mojo_compositor_...
File cc/ipc/mojo_compositor_frame_sink.mojom (right):

https://codereview.chromium.org/2715663007/diff/60001/cc/ipc/mojo_compositor_...
cc/ipc/mojo_compositor_frame_sink.mojom:79: ClaimTemporaryReference(SurfaceId
surface_id);
can this fail?  how do things break if an ill-behaving client fails to call this
or unexpectedly releases it?

[kylechar, 2017-02-28 18:59:52]

https://codereview.chromium.org/2715663007/diff/60001/cc/ipc/mojo_compositor_...
File cc/ipc/mojo_compositor_frame_sink.mojom (right):

https://codereview.chromium.org/2715663007/diff/60001/cc/ipc/mojo_compositor_...
cc/ipc/mojo_compositor_frame_sink.mojom:79: ClaimTemporaryReference(SurfaceId
surface_id);
On 2017/02/28 18:46:58, Tom Sepez wrote:
> can this fail?  how do things break if an ill-behaving client fails to call
this
> or unexpectedly releases it?

MojoCompositorFrameSinkPrivate is the private interface between WS (or browser)
and GPU that is trusted. It corresponds to a MojoCompositorFrameSink interface
between the client and GPU that is untrusted.

We expect the WS to always respond correctly. If there is no temporary reference
(for example the client beat the WS by adding a reference) then this will "fail"
somewhere in the GPU but only because it's not needed. The WS doesn't need to
know if it fails, no further action is necessary.

[Tom Sepez, 2017-02-28 19:01:43]

OK, LGTM. Thanks.

[sky, 2017-02-28 20:42:54]

https://codereview.chromium.org/2715663007/diff/60001/services/ui/ws/window_s...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2715663007/diff/60001/services/ui/ws/window_s...
services/ui/ws/window_server.cc:617: // If the parent of |window| is part of a
WindowTree, the WindowTree root that
Can you describe why does this code need to check the parent?

[kylechar, 2017-02-28 20:54:05]

https://codereview.chromium.org/2715663007/diff/60001/services/ui/ws/window_s...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2715663007/diff/60001/services/ui/ws/window_s...
services/ui/ws/window_server.cc:617: // If the parent of |window| is part of a
WindowTree, the WindowTree root that
On 2017/02/28 20:42:54, sky wrote:
> Can you describe why does this code need to check the parent?

A new cc::Surface that corresponds to |window| was just created. The WS needs to
find the FrameSinkId of the CompositorFrameSink that will embed that new surface
so it can tell the GPU. This uses the ServerWindow hierarchy to find that parent
(and the correspond FrameSinkId).

Assuming that all CFSs had been created then we could walk up window->parent()
until we found the ServerWindow that had a
ServerWindowCompositorFrameSinkManager. Unfortunately that happens
asynchronously and the embedded client could submit a surface before the parent
client has created a CompositorFrameSink. So walking up the parents won't work.
The TODO above alludes to that and would make this easier.

This assumes that the root of each WindowTree has a cc::Surface and that the
surface hierarchy for each client is only ever 1 level deep. This is true right
now (AFAIK) but probably needs the bool value to work in all future cases.

[kylechar, 2017-02-28 20:54:52]

https://codereview.chromium.org/2715663007/diff/60001/services/ui/ws/window_s...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2715663007/diff/60001/services/ui/ws/window_s...
services/ui/ws/window_server.cc:617: // If the parent of |window| is part of a
WindowTree, the WindowTree root that
On 2017/02/28 20:54:05, kylechar wrote:
> On 2017/02/28 20:42:54, sky wrote:
> > Can you describe why does this code need to check the parent?
> 
> A new cc::Surface that corresponds to |window| was just created. The WS needs
to
> find the FrameSinkId of the CompositorFrameSink that will embed that new
surface
> so it can tell the GPU. This uses the ServerWindow hierarchy to find that
parent
> (and the correspond FrameSinkId).
> 
> Assuming that all CFSs had been created then we could walk up window->parent()
> until we found the ServerWindow that had a
> ServerWindowCompositorFrameSinkManager. Unfortunately that happens
> asynchronously and the embedded client could submit a surface before the
parent
> client has created a CompositorFrameSink. So walking up the parents won't
work.
> The TODO above alludes to that and would make this easier.
> 
> This assumes that the root of each WindowTree has a cc::Surface and that the
> surface hierarchy for each client is only ever 1 level deep. This is true
right
> now (AFAIK) but probably needs the bool value to work in all future cases.

Want the comments for this method expanded to explain?

[Fady Samuel, 2017-03-01 04:25:17]

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

[commit-bot: I haz the power, 2017-03-01 04:25:27]

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-03-01 04:27:33]

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

[commit-bot: I haz the power, 2017-03-01 04:27:36]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kylechar, 2017-03-01 15:50:33]

I've made some changes in how we find the parent client in the WS after talking
to sky offline.

[Fady Samuel, 2017-03-01 16:07:46]

https://codereview.chromium.org/2715663007/diff/80001/services/ui/ws/window_s...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2715663007/diff/80001/services/ui/ws/window_s...
services/ui/ws/window_server.cc:823: ClaimTemporaryReference(window,
surface_info.id());
Can't we generalize this to be a single code path for ClaimTemporaryReference?

[sky, 2017-03-01 17:01:34]

This look good to me, but his is very subtle in particular the
ClaimTemporaryReference code). Can you write a test for this?

[Fady Samuel, 2017-03-01 17:08:27]

https://codereview.chromium.org/2715663007/diff/80001/services/ui/ws/window_s...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2715663007/diff/80001/services/ui/ws/window_s...
services/ui/ws/window_server.cc:823: ClaimTemporaryReference(window,
surface_info.id());
On 2017/03/01 16:07:46, Fady Samuel wrote:
> Can't we generalize this to be a single code path for ClaimTemporaryReference?

Or at least add a comment explaining why this is a special case.

[kylechar, 2017-03-01 17:15:24]

https://codereview.chromium.org/2715663007/diff/80001/services/ui/ws/window_s...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2715663007/diff/80001/services/ui/ws/window_s...
services/ui/ws/window_server.cc:823: ClaimTemporaryReference(window,
surface_info.id());
On 2017/03/01 16:07:46, Fady Samuel wrote:
> Can't we generalize this to be a single code path for ClaimTemporaryReference?

I'll add a comment with why. The client_id doesn't change from display root
(0:2) to the WM root (0:3) like it does for other clients.

[jbauman, 2017-03-01 23:55:13]

lgtm

[kylechar, 2017-03-02 15:52:08]

Patchset #5 (id:100001) has been deleted

[kylechar, 2017-03-02 16:01:33]

I improved the structure/documentation and added some comments.

I was looking at testing this and it's not going to be simple. I don't think any
of the existing test suites can be added onto. WindowServerTest is actually
service test and it verifies WindowServer behaves nicely from the point of view
of a WindowManager. It can't really test WindowServer internals.

We would need a test that does something similar from the perspective of the
GPU, which is problematic because the GPU is part of the WS still.

The logic in HandleTemporaryReferenceForNewSurface() is actually pretty simple,
we walk up the parents to find the ServerWindow with a different client id, that
is the embedder.

What the test would actually confirm is that clients continue to create
ServerWindows with that same patter of hierarchy.

[sky, 2017-03-02 19:29:18]

On Thu, Mar 2, 2017 at 8:01 AM,  <kylechar@chromium.org> wrote:
> I improved the structure/documentation and added some comments.
>
> I was looking at testing this and it's not going to be simple. I don't think
> any
> of the existing test suites can be added onto. WindowServerTest is actually
> service test and it verifies WindowServer behaves nicely from the point of
> view
> of a WindowManager. It can't really test WindowServer internals.

There are a bunch of unit_tests in services/ui/ws that don't use mojo.
To enable this some classes hide the InterfacePtr in a binding class
and only talk to the pure virtual class.
https://chromium.googlesource.com/chromium/src/+/master/services/ui/ws/window...
is an example of what I mean. There are plenty of tests that use it.

I don't want to hold up this change, but someone needs to take the
time to make it possible to write tests for this code otherwise it's
not going to be particularly stable.

  -Scott

> We would need a test that does something similar from the perspective of the
> GPU, which is problematic because the GPU is part of the WS still.
>
> The logic in HandleTemporaryReferenceForNewSurface() is actually pretty
> simple,
> we walk up the parents to find the ServerWindow with a different client id,
> that
> is the embedder.
>
> What the test would actually confirm is that clients continue to create
> ServerWindows with that same patter of hierarchy.
>
> https://codereview.chromium.org/2715663007/

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

[sky, 2017-03-02 19:29:25]

LGTM

[kylechar, 2017-03-02 20:41:03]

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

[commit-bot: I haz the power, 2017-03-02 20:41:38]

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

[commit-bot: I haz the power, 2017-03-02 21:47:31]

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

[commit-bot: I haz the power, 2017-03-02 21:47:32]

Dry run: This issue passed the CQ dry run.

[kylechar, 2017-03-02 21:48:47]

Thanks!

[kylechar, 2017-03-02 21:48:54]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2017-03-02 21:48:54]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
fsamuel@chromium.org, jbauman@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2715663007/#ps140001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-03-02 21:49:35]

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-03-02 21:54:59]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488491334335860,
"parent_rev": "f91e7de4d71622edb2d7298771db357734e57ad3", "commit_rev":
"5c97d3ea18174c41ad3ab15f0a9c5fe14717829d"}

[commit-bot: I haz the power, 2017-03-02 21:55:44]

Description was changed from

==========
Implement temporary reference assignment with DisplayCompositor.

This CL implements the temporary reference assignment via
DisplayCompositor. When DisplayCompositorClient finds out a new surface
has been created, it tries to find the CompositorFrameSink that will
embed the new surface. This is done in mus-ws as it is both fully
trusted and has the full ServerWindow heirarchy.

If a parent is found an IPC is sent back to DisplayCompositor to assign
ownership of the temporary reference. If the CompositorFrameSink that
owns a temporary reference is invalidated, the temporary reference is
removed because the owner can't add a real reference after invalidation.

If no parent is found, for example if the ServerWindow no longer exists
or isn't parented to something visible, then an IPC is sent back to
DisplayCompositor to drop the temporary reference. In this case mus-ws
doesn't expect anything to embed the new surface.

BUG=683738
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Implement temporary reference assignment with DisplayCompositor.

This CL implements the temporary reference assignment via
DisplayCompositor. When DisplayCompositorClient finds out a new surface
has been created, it tries to find the CompositorFrameSink that will
embed the new surface. This is done in mus-ws as it is both fully
trusted and has the full ServerWindow heirarchy.

If a parent is found an IPC is sent back to DisplayCompositor to assign
ownership of the temporary reference. If the CompositorFrameSink that
owns a temporary reference is invalidated, the temporary reference is
removed because the owner can't add a real reference after invalidation.

If no parent is found, for example if the ServerWindow no longer exists
or isn't parented to something visible, then an IPC is sent back to
DisplayCompositor to drop the temporary reference. In this case mus-ws
doesn't expect anything to embed the new surface.

BUG=683738
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2715663007
Cr-Commit-Position: refs/heads/master@{#454389}
Committed:
https://chromium.googlesource.com/chromium/src/+/5c97d3ea18174c41ad3ab15f0a9c...
==========

[commit-bot: I haz the power, 2017-03-02 21:55:46]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/5c97d3ea18174c41ad3ab15f0a9c...