Convert offscreen canvas to use FrameSinkManagerHost.

Convert offscreen canvas to use FrameSinkManagerHost.

This CL switches the content/browser/ part of offscreen canvas to use
FrameSinkManagerHost instead of directly accessing SurfaceMananger.

OffscreenCanvasProvider(Impl) exists for the renderer to request other
interfaces from the browser. The renderer uses it create both a
OffscreenCanvasSurface and MojoCompositorFrameSink. This is a
simplification of OffscreenCanvasCompositorFrameSinkProvider and
OffscreenCanvasSurfaceFactory interfaces.

RenderProcessHostImpl creates a single OffscreenCanvasProviderImpl on
demand and uses it for all offscreen canvas requests from the renderer.

The renderer will first request a OffscreenCanvasSurface and specify the
FrameSinkId. This always happens from the main renderer thread. A
OffscreenCanvasSurfaceImpl will be created for the browser side of this
connection.

The renderer will then likely request MojoCompositorFrameSink for the
FrameSinkId, from either the main thread or a worker thread. This
request is forwarded to MojoFrameSinkManager. This sets up two
connections. There is a MojoCompositorFrameSink from
MojoFrameSinkManager to the renderer. This is used to submit
CompositorFrames. There is MojoCompositorFrameSinkPrivate from
OffscreenCanvasSurfaceImpl to MojoFrameSinkManager. This gives the
browser control over the CompositorFrameSink created in
MojoFrameSinkManager.

The render can close the OffscreenCanvasSurface and
MojoCompositorFrameSink connections in any order. The assumption is they
will both be closed at approximately the same time.

The remaining direct access to SurfaceManager by offscreen canvas code
is blocked on other features being completed.

BUG=664547, 686861
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2789753002
Cr-Commit-Position: refs/heads/master@{#464381}
Committed: https://chromium.googlesource.com/chromium/src/+/30b894d4f8a1c58183749fb7e0982f53dba0ba40

[kylechar, 2017-03-30 21:12:40]

Description was changed from

==========
Convert offscreen canvas to use <insert_name>.

WIP.

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

to

==========
Convert offscreen canvas to use <insert_name>.

WIP.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[kylechar, 2017-03-31 16:41:38]

Patchset #3 (id:40001) has been deleted

[xlai (Olivia), 2017-04-03 15:42:58]

xlai@chromium.org changed reviewers:
+ xlai@chromium.org

[xlai (Olivia), 2017-04-03 15:42:59]

Nice refactoring work! I have no problems with content/browser/renderer_host/*
and blink changes except one small question as written in-line.

https://codereview.chromium.org/2789753002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right):

https://codereview.chromium.org/2789753002/diff/80001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_impl.cc:32:
content::GetDisplayCompositor()->UnregisterFrameSinkHierarchy(
Previously this is done in destroy of OffscreenCanvasCompositorFrameSink.
What happened when CompositorFrameSink is torn down whilst the
OffscreenCanvasSurface is still alive (worker thread dead but main thread
alive)? Then you'll have a (parent_frame_sink_id, child_frame_sink_id) pair in
the frame sink hierarchy of the display compositor such that the parent one is
alive but the child one will point to no compositor frame sink. Will that cause
a problem?

[kylechar, 2017-04-03 16:00:20]

https://codereview.chromium.org/2789753002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right):

https://codereview.chromium.org/2789753002/diff/80001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_impl.cc:32:
content::GetDisplayCompositor()->UnregisterFrameSinkHierarchy(
On 2017/04/03 15:42:59, xlai (Olivia) wrote:
> Previously this is done in destroy of OffscreenCanvasCompositorFrameSink.
> What happened when CompositorFrameSink is torn down whilst the
> OffscreenCanvasSurface is still alive (worker thread dead but main thread
> alive)? Then you'll have a (parent_frame_sink_id, child_frame_sink_id) pair in
> the frame sink hierarchy of the display compositor such that the parent one is
> alive but the child one will point to no compositor frame sink. Will that
cause
> a problem?

The GpuCompositorFrameSink in DisplayCompositor is only destroyed when both
interfaces (MojoCompositorFrameSink and MojoCompositorFrameSinkPrivate) are
closed. So there will be a GpuCompositorFrameSink until this gets torn down.

I think the only constraints this puts on things are:
1. Renderer closes the OffscreenCanvasSurface connection only after it has
stopped submitting CFs that embed the offscreen canvas SurfaceId. This is
because the heirarchy unregistered here.
2. Renderer doesn't close MojoCompositorFrameSink connection (which I guess
means destroying OffscreenCanvasFrameDispatcherImpl) and then create a new
MojoCompositorFrameSink connection again using the same OffscreenCanvasSurface.

The order in which MojoCompositorFrameSink and OffscreenCanvasSurface
connections are closed shouldn't matter, although I'd assume they would
generally be closed around the same time? It's also possible to have started and
created an OffscreenCanvasSurface connection but never create the
MojoCompositorFrameSink connection associated with it.

[xlai (Olivia), 2017-04-05 15:05:12]

On 2017/04/03 16:00:20, kylechar wrote:
>
https://codereview.chromium.org/2789753002/diff/80001/content/browser/rendere...
> File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right):
> 
>
https://codereview.chromium.org/2789753002/diff/80001/content/browser/rendere...
> content/browser/renderer_host/offscreen_canvas_surface_impl.cc:32:
> content::GetDisplayCompositor()->UnregisterFrameSinkHierarchy(
> On 2017/04/03 15:42:59, xlai (Olivia) wrote:
> > Previously this is done in destroy of OffscreenCanvasCompositorFrameSink.
> > What happened when CompositorFrameSink is torn down whilst the
> > OffscreenCanvasSurface is still alive (worker thread dead but main thread
> > alive)? Then you'll have a (parent_frame_sink_id, child_frame_sink_id) pair
in
> > the frame sink hierarchy of the display compositor such that the parent one
is
> > alive but the child one will point to no compositor frame sink. Will that
> cause
> > a problem?
> 
> The GpuCompositorFrameSink in DisplayCompositor is only destroyed when both
> interfaces (MojoCompositorFrameSink and MojoCompositorFrameSinkPrivate) are
> closed. So there will be a GpuCompositorFrameSink until this gets torn down.
> 
> I think the only constraints this puts on things are:
> 1. Renderer closes the OffscreenCanvasSurface connection only after it has
> stopped submitting CFs that embed the offscreen canvas SurfaceId. This is
> because the heirarchy unregistered here.

I understand the hierarchy unregister handles the situation when
OffscreenCanvasSurface
is closed. But my concern is a different issue.

> 2. Renderer doesn't close MojoCompositorFrameSink connection (which I guess
> means destroying OffscreenCanvasFrameDispatcherImpl) and then create a new
> MojoCompositorFrameSink connection again using the same
OffscreenCanvasSurface.
> 

Scenario 2 will not happen because the Blink side will prevent transfer control
to
Offscreen from the same HTML canvas element twice.

> The order in which MojoCompositorFrameSink and OffscreenCanvasSurface
> connections are closed shouldn't matter, although I'd assume they would
> generally be closed around the same time? 

Why are the order in which these two are closed shouldn't matter? Even if they
are generally
closed around the same time, it is possible that OffscreenCanvasSurface is alive
and 
MojoCompositorFrameSink is destroyed. In that case, you didn't unregister the
frame sink id
hierarchy.

Note that the parent frame sink id is associated with the HTML canvas element on
main thread
and it is still alive. Then in your FrameSinkManager::frame_sink_source_map_,
you are going to
have a pair of (parent, child) frame sink ids such that the parent one is
pointing to an 
active source and the child one is not pointing to an active source. 


> It's also possible to have started and
> created an OffscreenCanvasSurface connection but never create the
> MojoCompositorFrameSink connection associated with it.

Yes I know that this was handled in the code but my question was not on this.

[kylechar, 2017-04-05 18:16:44]

On 2017/04/05 15:05:12, xlai (Olivia) wrote:
> Why are the order in which these two are closed shouldn't matter? Even if they
> are generally
> closed around the same time, it is possible that OffscreenCanvasSurface is
alive
> and 
> MojoCompositorFrameSink is destroyed. In that case, you didn't unregister the
> frame sink id
> hierarchy.
> 
> Note that the parent frame sink id is associated with the HTML canvas element
on
> main thread
> and it is still alive. Then in your FrameSinkManager::frame_sink_source_map_,
> you are going to
> have a pair of (parent, child) frame sink ids such that the parent one is
> pointing to an 
> active source and the child one is not pointing to an active source. 

When MojoCompositorFrameSink on the renderer closes
GpuCompositorFrameSink::OnClientConnectionLost() is what finds ou. Since
OffscreenCanvasSurface is still alive the private interface is still alive and
GpuCompositorFrameSink is not destroyed. If
GpuCompositorFrameSink::OnBeginFrame() gets called when the client connection is
gone then it will do nothing since |client_| is no longer bound. When
OffscreenCanvasSurface is later destroyed the private interface closes, frame
sink hierarchy is unregistered and the unbound |client_| gets destroyed along
with any state it holds.

There might be an optimization missing here. I think it would be better to call
client_.reset() in GpuCompositorFrameSink::OnClientConnectionLost(). I'm trying
to figure out if we queue messages on a closed pipe the same way we queue
messages on a yet to opened pipe.

[kylechar, 2017-04-11 14:14:29]

Description was changed from

==========
Convert offscreen canvas to use <insert_name>.

WIP.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Convert offscreen canvas to use FrameSinkManagerHost.

This CL switches the content/browser/ part of offscreen canvas to use
FrameSinkManagerHost instead of directly accessing SurfaceMananger.

WIP.

BUG=664547,686861
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[kylechar, 2017-04-11 15:34:24]

Description was changed from

==========
Convert offscreen canvas to use FrameSinkManagerHost.

This CL switches the content/browser/ part of offscreen canvas to use
FrameSinkManagerHost instead of directly accessing SurfaceMananger.

WIP.

BUG=664547,686861
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Convert offscreen canvas to use FrameSinkManagerHost.

This CL switches the content/browser/ part of offscreen canvas to use
FrameSinkManagerHost instead of directly accessing SurfaceMananger.

WIP.

BUG=664547,686861
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[kylechar, 2017-04-11 17:13:40]

Description was changed from

==========
Convert offscreen canvas to use FrameSinkManagerHost.

This CL switches the content/browser/ part of offscreen canvas to use
FrameSinkManagerHost instead of directly accessing SurfaceMananger.

WIP.

BUG=664547,686861
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Convert offscreen canvas to use FrameSinkManagerHost.

This CL switches the content/browser/ part of offscreen canvas to use
FrameSinkManagerHost instead of directly accessing SurfaceMananger.

OffscreenCanvasProvider(Impl) exists for the renderer to request other
interfaces from the browser. The renderer uses it create both a
OffscreenCanvasSurface and MojoCompositorFrameSink. This is a
simplification of OffscreenCanvasCompositorFrameSinkProvider and
OffscreenCanvasSurfaceFactory interfaces.

RenderProcessHostImpl creates a single OffscreenCanvasProviderImpl on
demand and uses it for all offscreen canvas requests from the renderer.

The renderer will first request a OffscreenCanvasSurface and specify the
FrameSinkId. This always happens from the main renderer thread. A
OffscreenCanvasSurfaceImpl will be created for the browser side of this
connection.

The renderer will then likely request MojoCompositorFrameSink for the
FrameSinkId, from either the main thread or a worker thread. This
request is forwarded to MojoFrameSinkManager. This sets up two
connections. There is a MojoCompositorFrameSink from
MojoFrameSinkManager to the client. This is used to submit
CompositorFrames. There is MojoCompositorFrameSinkPrivate from
OffscreenCanvasSurfaceImpl to MojoFrameSinkManager. This gives the
browser control over the CompositorFrameSink interface.

The render can close the OffscreenCanvasSurface and
MojoCompositorFrameSink connections in any order without issues.

The only direct usage of SurfaceManager is for Require() and Satisfy().
There is no Mojo equivalent and these will be going away in the future
with surface references.

BUG=664547,686861
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[kylechar, 2017-04-11 17:34:28]

Description was changed from

==========
Convert offscreen canvas to use FrameSinkManagerHost.

This CL switches the content/browser/ part of offscreen canvas to use
FrameSinkManagerHost instead of directly accessing SurfaceMananger.

OffscreenCanvasProvider(Impl) exists for the renderer to request other
interfaces from the browser. The renderer uses it create both a
OffscreenCanvasSurface and MojoCompositorFrameSink. This is a
simplification of OffscreenCanvasCompositorFrameSinkProvider and
OffscreenCanvasSurfaceFactory interfaces.

RenderProcessHostImpl creates a single OffscreenCanvasProviderImpl on
demand and uses it for all offscreen canvas requests from the renderer.

The renderer will first request a OffscreenCanvasSurface and specify the
FrameSinkId. This always happens from the main renderer thread. A
OffscreenCanvasSurfaceImpl will be created for the browser side of this
connection.

The renderer will then likely request MojoCompositorFrameSink for the
FrameSinkId, from either the main thread or a worker thread. This
request is forwarded to MojoFrameSinkManager. This sets up two
connections. There is a MojoCompositorFrameSink from
MojoFrameSinkManager to the client. This is used to submit
CompositorFrames. There is MojoCompositorFrameSinkPrivate from
OffscreenCanvasSurfaceImpl to MojoFrameSinkManager. This gives the
browser control over the CompositorFrameSink interface.

The render can close the OffscreenCanvasSurface and
MojoCompositorFrameSink connections in any order without issues.

The only direct usage of SurfaceManager is for Require() and Satisfy().
There is no Mojo equivalent and these will be going away in the future
with surface references.

BUG=664547,686861
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Convert offscreen canvas to use FrameSinkManagerHost.

This CL switches the content/browser/ part of offscreen canvas to use
FrameSinkManagerHost instead of directly accessing SurfaceMananger.

OffscreenCanvasProvider(Impl) exists for the renderer to request other
interfaces from the browser. The renderer uses it create both a
OffscreenCanvasSurface and MojoCompositorFrameSink. This is a
simplification of OffscreenCanvasCompositorFrameSinkProvider and
OffscreenCanvasSurfaceFactory interfaces.

RenderProcessHostImpl creates a single OffscreenCanvasProviderImpl on
demand and uses it for all offscreen canvas requests from the renderer.

The renderer will first request a OffscreenCanvasSurface and specify the
FrameSinkId. This always happens from the main renderer thread. A
OffscreenCanvasSurfaceImpl will be created for the browser side of this
connection.

The renderer will then likely request MojoCompositorFrameSink for the
FrameSinkId, from either the main thread or a worker thread. This
request is forwarded to MojoFrameSinkManager. This sets up two
connections. There is a MojoCompositorFrameSink from
MojoFrameSinkManager to the renderer. This is used to submit
CompositorFrames. There is MojoCompositorFrameSinkPrivate from
OffscreenCanvasSurfaceImpl to MojoFrameSinkManager. This gives the
browser control over the CompositorFrameSink created in
MojoFrameSinkManager.

The render can close the OffscreenCanvasSurface and
MojoCompositorFrameSink connections in any order. The assumption is they
will both be closed at approximately the same time.

The only direct usage of SurfaceManager is for Require() and Satisfy().
There is no Mojo equivalent and these will be going away in the future
with surface references.

BUG=664547,686861
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[kylechar, 2017-04-11 17:34:35]

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

[commit-bot: I haz the power, 2017-04-11 17:35:16]

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-11 18:06:48]

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

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

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

[kylechar, 2017-04-11 20:43:04]

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

[commit-bot: I haz the power, 2017-04-11 20:43:32]

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-11 22:27:19]

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

[commit-bot: I haz the power, 2017-04-11 22:27:20]

Dry run: This issue passed the CQ dry run.

[kylechar, 2017-04-12 12:30:48]

Description was changed from

==========
Convert offscreen canvas to use FrameSinkManagerHost.

This CL switches the content/browser/ part of offscreen canvas to use
FrameSinkManagerHost instead of directly accessing SurfaceMananger.

OffscreenCanvasProvider(Impl) exists for the renderer to request other
interfaces from the browser. The renderer uses it create both a
OffscreenCanvasSurface and MojoCompositorFrameSink. This is a
simplification of OffscreenCanvasCompositorFrameSinkProvider and
OffscreenCanvasSurfaceFactory interfaces.

RenderProcessHostImpl creates a single OffscreenCanvasProviderImpl on
demand and uses it for all offscreen canvas requests from the renderer.

The renderer will first request a OffscreenCanvasSurface and specify the
FrameSinkId. This always happens from the main renderer thread. A
OffscreenCanvasSurfaceImpl will be created for the browser side of this
connection.

The renderer will then likely request MojoCompositorFrameSink for the
FrameSinkId, from either the main thread or a worker thread. This
request is forwarded to MojoFrameSinkManager. This sets up two
connections. There is a MojoCompositorFrameSink from
MojoFrameSinkManager to the renderer. This is used to submit
CompositorFrames. There is MojoCompositorFrameSinkPrivate from
OffscreenCanvasSurfaceImpl to MojoFrameSinkManager. This gives the
browser control over the CompositorFrameSink created in
MojoFrameSinkManager.

The render can close the OffscreenCanvasSurface and
MojoCompositorFrameSink connections in any order. The assumption is they
will both be closed at approximately the same time.

The only direct usage of SurfaceManager is for Require() and Satisfy().
There is no Mojo equivalent and these will be going away in the future
with surface references.

BUG=664547,686861
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Convert offscreen canvas to use FrameSinkManagerHost.

This CL switches the content/browser/ part of offscreen canvas to use
FrameSinkManagerHost instead of directly accessing SurfaceMananger.

OffscreenCanvasProvider(Impl) exists for the renderer to request other
interfaces from the browser. The renderer uses it create both a
OffscreenCanvasSurface and MojoCompositorFrameSink. This is a
simplification of OffscreenCanvasCompositorFrameSinkProvider and
OffscreenCanvasSurfaceFactory interfaces.

RenderProcessHostImpl creates a single OffscreenCanvasProviderImpl on
demand and uses it for all offscreen canvas requests from the renderer.

The renderer will first request a OffscreenCanvasSurface and specify the
FrameSinkId. This always happens from the main renderer thread. A
OffscreenCanvasSurfaceImpl will be created for the browser side of this
connection.

The renderer will then likely request MojoCompositorFrameSink for the
FrameSinkId, from either the main thread or a worker thread. This
request is forwarded to MojoFrameSinkManager. This sets up two
connections. There is a MojoCompositorFrameSink from
MojoFrameSinkManager to the renderer. This is used to submit
CompositorFrames. There is MojoCompositorFrameSinkPrivate from
OffscreenCanvasSurfaceImpl to MojoFrameSinkManager. This gives the
browser control over the CompositorFrameSink created in
MojoFrameSinkManager.

The render can close the OffscreenCanvasSurface and
MojoCompositorFrameSink connections in any order. The assumption is they
will both be closed at approximately the same time.

The remaining direct access to SurfaceManager by offscreen canvas code
is blocked on other features being complete.

BUG=664547,686861
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[kylechar, 2017-04-12 12:30:57]

Description was changed from

==========
Convert offscreen canvas to use FrameSinkManagerHost.

This CL switches the content/browser/ part of offscreen canvas to use
FrameSinkManagerHost instead of directly accessing SurfaceMananger.

OffscreenCanvasProvider(Impl) exists for the renderer to request other
interfaces from the browser. The renderer uses it create both a
OffscreenCanvasSurface and MojoCompositorFrameSink. This is a
simplification of OffscreenCanvasCompositorFrameSinkProvider and
OffscreenCanvasSurfaceFactory interfaces.

RenderProcessHostImpl creates a single OffscreenCanvasProviderImpl on
demand and uses it for all offscreen canvas requests from the renderer.

The renderer will first request a OffscreenCanvasSurface and specify the
FrameSinkId. This always happens from the main renderer thread. A
OffscreenCanvasSurfaceImpl will be created for the browser side of this
connection.

The renderer will then likely request MojoCompositorFrameSink for the
FrameSinkId, from either the main thread or a worker thread. This
request is forwarded to MojoFrameSinkManager. This sets up two
connections. There is a MojoCompositorFrameSink from
MojoFrameSinkManager to the renderer. This is used to submit
CompositorFrames. There is MojoCompositorFrameSinkPrivate from
OffscreenCanvasSurfaceImpl to MojoFrameSinkManager. This gives the
browser control over the CompositorFrameSink created in
MojoFrameSinkManager.

The render can close the OffscreenCanvasSurface and
MojoCompositorFrameSink connections in any order. The assumption is they
will both be closed at approximately the same time.

The remaining direct access to SurfaceManager by offscreen canvas code
is blocked on other features being complete.

BUG=664547,686861
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Convert offscreen canvas to use FrameSinkManagerHost.

This CL switches the content/browser/ part of offscreen canvas to use
FrameSinkManagerHost instead of directly accessing SurfaceMananger.

OffscreenCanvasProvider(Impl) exists for the renderer to request other
interfaces from the browser. The renderer uses it create both a
OffscreenCanvasSurface and MojoCompositorFrameSink. This is a
simplification of OffscreenCanvasCompositorFrameSinkProvider and
OffscreenCanvasSurfaceFactory interfaces.

RenderProcessHostImpl creates a single OffscreenCanvasProviderImpl on
demand and uses it for all offscreen canvas requests from the renderer.

The renderer will first request a OffscreenCanvasSurface and specify the
FrameSinkId. This always happens from the main renderer thread. A
OffscreenCanvasSurfaceImpl will be created for the browser side of this
connection.

The renderer will then likely request MojoCompositorFrameSink for the
FrameSinkId, from either the main thread or a worker thread. This
request is forwarded to MojoFrameSinkManager. This sets up two
connections. There is a MojoCompositorFrameSink from
MojoFrameSinkManager to the renderer. This is used to submit
CompositorFrames. There is MojoCompositorFrameSinkPrivate from
OffscreenCanvasSurfaceImpl to MojoFrameSinkManager. This gives the
browser control over the CompositorFrameSink created in
MojoFrameSinkManager.

The render can close the OffscreenCanvasSurface and
MojoCompositorFrameSink connections in any order. The assumption is they
will both be closed at approximately the same time.

The remaining direct access to SurfaceManager by offscreen canvas code
is blocked on other features being completed.

BUG=664547,686861
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[kylechar, 2017-04-12 12:42:46]

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

[kylechar, 2017-04-12 12:42:47]

All of the prerequisite CLs are committed and this is ready for review. Let me
know if there are any concerns about this from an offscreen canvas perspective.
This isn't intended to change any behaviour, just simplify the offscreen canvas
code in the browser and start using FrameSinkManagerHost.

+xlai for content/browser/renderer_host/*offscreen_canvas*
+junov for third_party/WebKit/*

[Justin Novosad, 2017-04-12 14:44:41]

lgtm fo third_party/WebKit

[Fady Samuel, 2017-04-12 14:52:43]

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

[Fady Samuel, 2017-04-12 14:52:44]

Looks good to me once Olivia is happy with it.

[xlai (Olivia), 2017-04-12 15:45:27]

I see your point now. lgtm for offscreen_canvas* changes.

Btw you need reviews from security owners and content/* owners too.

On 2017/04/05 18:16:44, kylechar wrote:
> On 2017/04/05 15:05:12, xlai (Olivia) wrote:
> > Why are the order in which these two are closed shouldn't matter? Even if
they
> > are generally
> > closed around the same time, it is possible that OffscreenCanvasSurface is
> alive
> > and 
> > MojoCompositorFrameSink is destroyed. In that case, you didn't unregister
the
> > frame sink id
> > hierarchy.
> > 
> > Note that the parent frame sink id is associated with the HTML canvas
element
> on
> > main thread
> > and it is still alive. Then in your
FrameSinkManager::frame_sink_source_map_,
> > you are going to
> > have a pair of (parent, child) frame sink ids such that the parent one is
> > pointing to an 
> > active source and the child one is not pointing to an active source. 
> 
> When MojoCompositorFrameSink on the renderer closes
> GpuCompositorFrameSink::OnClientConnectionLost() is what finds ou. Since
> OffscreenCanvasSurface is still alive the private interface is still alive and
> GpuCompositorFrameSink is not destroyed. If
> GpuCompositorFrameSink::OnBeginFrame() gets called when the client connection
is
> gone then it will do nothing since |client_| is no longer bound. When
> OffscreenCanvasSurface is later destroyed the private interface closes, frame
> sink hierarchy is unregistered and the unbound |client_| gets destroyed along
> with any state it holds.
> 
> There might be an optimization missing here. I think it would be better to
call
> client_.reset() in GpuCompositorFrameSink::OnClientConnectionLost(). I'm
trying
> to figure out if we queue messages on a closed pipe the same way we queue
> messages on a yet to opened pipe.

[kylechar, 2017-04-12 15:54:22]

kylechar@chromium.org changed reviewers:
+ piman@chromium.org, tsepez@chromium.org

[kylechar, 2017-04-12 15:54:23]

+piman for other things in content/*
+tsepez for mojom/security review

[Tom Sepez, 2017-04-12 16:12:06]

lgtm

[piman, 2017-04-12 18:08:43]

https://codereview.chromium.org/2789753002/diff/180001/content/browser/render...
File content/browser/renderer_host/offscreen_canvas_provider_impl.cc (right):

https://codereview.chromium.org/2789753002/diff/180001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_provider_impl.cc:37:
DCHECK(surface_impl);
What makes this DCHECK valid? frame_sink_id comes from the (untrusted) renderer,
right? It could give an arbitrary value that makes the lookup fail.

https://codereview.chromium.org/2789753002/diff/180001/content/browser/render...
File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right):

https://codereview.chromium.org/2789753002/diff/180001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_surface_impl.cc:55:
DCHECK(!compositor_frame_sink_private_.is_bound());
What makes this DCHECK valid? Can't the (untrusted) renderer call
CreateCompositorFrameSink twice?

[kylechar, 2017-04-12 18:48:35]

https://codereview.chromium.org/2789753002/diff/180001/content/browser/render...
File content/browser/renderer_host/offscreen_canvas_provider_impl.cc (right):

https://codereview.chromium.org/2789753002/diff/180001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_provider_impl.cc:37:
DCHECK(surface_impl);
On 2017/04/12 18:08:43, piman wrote:
> What makes this DCHECK valid? frame_sink_id comes from the (untrusted)
renderer,
> right? It could give an arbitrary value that makes the lookup fail.

It would dereference the nullptr and (probably?) crash. I've changed it to
verify the lookup is successful. Done.

piman/tsepez: I never really thought about this before. What are the security
implications of dereferencing a pointer to nullptr? I know it's UB, although I
sort of figured it would always seg fault. Is this potentially exploitable?

https://codereview.chromium.org/2789753002/diff/180001/content/browser/render...
File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right):

https://codereview.chromium.org/2789753002/diff/180001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_surface_impl.cc:55:
DCHECK(!compositor_frame_sink_private_.is_bound());
On 2017/04/12 18:08:43, piman wrote:
> What makes this DCHECK valid? Can't the (untrusted) renderer call
> CreateCompositorFrameSink twice?

It wouldn't be valid to call this twice, but I don't think it would be an issue
if a malicious renderer did it. The malicious renderer would just replace the
MojoCompositorFrameSink connection it owns with a new identical connection. It
could also reopen a connection that it chose to close.

However, maybe it's better to strictly enforce CreateCompositorFrameSink only
ever gets called once? Done.

[piman, 2017-04-12 19:07:43]

https://codereview.chromium.org/2789753002/diff/180001/content/browser/render...
File content/browser/renderer_host/offscreen_canvas_provider_impl.cc (right):

https://codereview.chromium.org/2789753002/diff/180001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_provider_impl.cc:37:
DCHECK(surface_impl);
On 2017/04/12 18:48:35, kylechar wrote:
> On 2017/04/12 18:08:43, piman wrote:
> > What makes this DCHECK valid? frame_sink_id comes from the (untrusted)
> renderer,
> > right? It could give an arbitrary value that makes the lookup fail.
> 
> It would dereference the nullptr and (probably?) crash. I've changed it to
> verify the lookup is successful. Done.
> 
> piman/tsepez: I never really thought about this before. What are the security
> implications of dereferencing a pointer to nullptr? I know it's UB, although I
> sort of figured it would always seg fault. Is this potentially exploitable?

3 things:
1- UB is UB and the compiler is allowed to make assumptions that it can never
happen. There are famous security vulnerabilities introduced by things similar
to this (see e.g.
https://people.csail.mit.edu/nickolai/papers/wang-undef-2012-08-21.pdf ). I
don't know that it would be the case today for this, but compilers get smarter
every day.
2- CreateCompositorFrameSink is not virtual, so it wouldn't segfault here. It
might segfault in the implementation, or we might add |this| (now NULL) to a
container, and later assume it's not NULL etc.
3- segfaulting the browser from the renderer is bad anyway, let's not do it.

https://codereview.chromium.org/2789753002/diff/200001/content/browser/render...
File content/browser/renderer_host/offscreen_canvas_provider_impl.cc (right):

https://codereview.chromium.org/2789753002/diff/200001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_provider_impl.cc:38:
NOTREACHED();
nit: not NOTREACHED(). It can be reached. Ideally a test would exist that
actually exercises that path and verifies that we handle things gracefully.
Consider DLOG(ERROR), or even possibly killing the renderer (via
bad_message::ReceivedBadMessage)

https://codereview.chromium.org/2789753002/diff/200001/content/browser/render...
File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right):

https://codereview.chromium.org/2789753002/diff/200001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_surface_impl.cc:56: NOTREACHED();
nit: ditto, can be reached.

[kylechar, 2017-04-12 21:06:29]

https://codereview.chromium.org/2789753002/diff/200001/content/browser/render...
File content/browser/renderer_host/offscreen_canvas_provider_impl.cc (right):

https://codereview.chromium.org/2789753002/diff/200001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_provider_impl.cc:38:
NOTREACHED();
On 2017/04/12 19:07:43, piman wrote:
> nit: not NOTREACHED(). It can be reached. Ideally a test would exist that
> actually exercises that path and verifies that we handle things gracefully.
> Consider DLOG(ERROR), or even possibly killing the renderer (via
> bad_message::ReceivedBadMessage)

Done.

https://codereview.chromium.org/2789753002/diff/200001/content/browser/render...
File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right):

https://codereview.chromium.org/2789753002/diff/200001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_surface_impl.cc:56: NOTREACHED();
On 2017/04/12 19:07:43, piman wrote:
> nit: ditto, can be reached.

Done.

[piman, 2017-04-12 21:08:42]

LGTM, thanks!

[kylechar, 2017-04-12 22:19:11]

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

[commit-bot: I haz the power, 2017-04-12 22:19:47]

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-12 23:58:03]

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

[commit-bot: I haz the power, 2017-04-12 23:58:04]

Dry run: This issue passed the CQ dry run.

[kylechar, 2017-04-13 12:05:09]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2017-04-13 12:05:10]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
xlai@chromium.org, junov@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2789753002/#ps240001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-04-13 12:05:21]

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-13 12:09:29]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1492085109958550,
"parent_rev": "6044a51d3e0eff211412224d21f62777db7641a4", "commit_rev":
"30b894d4f8a1c58183749fb7e0982f53dba0ba40"}

[commit-bot: I haz the power, 2017-04-13 12:10:24]

Description was changed from

==========
Convert offscreen canvas to use FrameSinkManagerHost.

This CL switches the content/browser/ part of offscreen canvas to use
FrameSinkManagerHost instead of directly accessing SurfaceMananger.

OffscreenCanvasProvider(Impl) exists for the renderer to request other
interfaces from the browser. The renderer uses it create both a
OffscreenCanvasSurface and MojoCompositorFrameSink. This is a
simplification of OffscreenCanvasCompositorFrameSinkProvider and
OffscreenCanvasSurfaceFactory interfaces.

RenderProcessHostImpl creates a single OffscreenCanvasProviderImpl on
demand and uses it for all offscreen canvas requests from the renderer.

The renderer will first request a OffscreenCanvasSurface and specify the
FrameSinkId. This always happens from the main renderer thread. A
OffscreenCanvasSurfaceImpl will be created for the browser side of this
connection.

The renderer will then likely request MojoCompositorFrameSink for the
FrameSinkId, from either the main thread or a worker thread. This
request is forwarded to MojoFrameSinkManager. This sets up two
connections. There is a MojoCompositorFrameSink from
MojoFrameSinkManager to the renderer. This is used to submit
CompositorFrames. There is MojoCompositorFrameSinkPrivate from
OffscreenCanvasSurfaceImpl to MojoFrameSinkManager. This gives the
browser control over the CompositorFrameSink created in
MojoFrameSinkManager.

The render can close the OffscreenCanvasSurface and
MojoCompositorFrameSink connections in any order. The assumption is they
will both be closed at approximately the same time.

The remaining direct access to SurfaceManager by offscreen canvas code
is blocked on other features being completed.

BUG=664547,686861
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Convert offscreen canvas to use FrameSinkManagerHost.

This CL switches the content/browser/ part of offscreen canvas to use
FrameSinkManagerHost instead of directly accessing SurfaceMananger.

OffscreenCanvasProvider(Impl) exists for the renderer to request other
interfaces from the browser. The renderer uses it create both a
OffscreenCanvasSurface and MojoCompositorFrameSink. This is a
simplification of OffscreenCanvasCompositorFrameSinkProvider and
OffscreenCanvasSurfaceFactory interfaces.

RenderProcessHostImpl creates a single OffscreenCanvasProviderImpl on
demand and uses it for all offscreen canvas requests from the renderer.

The renderer will first request a OffscreenCanvasSurface and specify the
FrameSinkId. This always happens from the main renderer thread. A
OffscreenCanvasSurfaceImpl will be created for the browser side of this
connection.

The renderer will then likely request MojoCompositorFrameSink for the
FrameSinkId, from either the main thread or a worker thread. This
request is forwarded to MojoFrameSinkManager. This sets up two
connections. There is a MojoCompositorFrameSink from
MojoFrameSinkManager to the renderer. This is used to submit
CompositorFrames. There is MojoCompositorFrameSinkPrivate from
OffscreenCanvasSurfaceImpl to MojoFrameSinkManager. This gives the
browser control over the CompositorFrameSink created in
MojoFrameSinkManager.

The render can close the OffscreenCanvasSurface and
MojoCompositorFrameSink connections in any order. The assumption is they
will both be closed at approximately the same time.

The remaining direct access to SurfaceManager by offscreen canvas code
is blocked on other features being completed.

BUG=664547,686861
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2789753002
Cr-Commit-Position: refs/heads/master@{#464381}
Committed:
https://chromium.googlesource.com/chromium/src/+/30b894d4f8a1c58183749fb7e098...
==========

[commit-bot: I haz the power, 2017-04-13 12:10:26]

Committed patchset #12 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/30b894d4f8a1c58183749fb7e098...