DisplayCompositor should enforce invariant that frame size and device scale factor are fixed |
|||||||
Issue descriptionpiman@ expressed a concern about allocating surface IDs in an untrusted process. Currently the parent kind of blindly assumes that the surface it's embedding has a certain size and device scale factor. The child can at any time reuse the surface ID for a different size or device scale factor. I thought that it might make sense for the display compositor (cc/surfaces) to help enforce that invariant for a given surface ID. Today we allocate a new surface through SurfaceManager::CreateSurface. That method just takes in a SurfaceFactory and SurfaceID. I propose it take in a SurfaceInfo (which is a Surface ID / Frame Size / Device Scale Factor). If cc::Surface::QueueFrame sees a CompositorFrame that has a different size or device scale factor, we can perhaps drop the frame, return resources, and tell the display compositor host (browser/window server) that some client with a provided FrameSinkId is misbehaving. Then the browser / window server can take appropriate measures (e.g. kill the client). WDYT?
,
Mar 17 2017
I'm not sure what we want to enforce about this. The device scale factor determines what scale the child wants to render at, but the child could rasterize everything at the wrong scale factor regardless of what the parent wants. The frame size I guess would be the size of the root render pass, but the child could still put too many quads around the outside of it, or not fill it completely with data, or make some of the root pass quads non-opaque.
,
Mar 17 2017
I guess I was thinking we shouldn't regress the property that frame.metadata.device_scale_factor and frame.render_pass_list.back()->output_rect.size() are the same for all CompositorFrames submitted to a given Surface ID. Other properties you mention John, aren't enforced today.
,
Mar 17 2017
I believe this is a good idea and it is easy to implement. Here is a prototype: https://codereview.chromium.org/2757953002/
,
Mar 17 2017
Sweet! That's pretty much what I had in mind, although it's unfortunate we need that check for render_pass_list.empty(). I think this makes sense. Does it matter if we return resources to the misbehaving child? WDYT?
,
Apr 18 2017
We have a couple of checks in RenderWidgetHostImpl::SubmitCompositorFrame that we're not submitting empty frames and that we retain the size and device scale factor given a fixed surface ID. Eventually this code needs to move out of RenderWidgetHostImpl. I do think we should enforce this in cc::Surface or CompositorFrameSinkSupport instead because that's generic code that isn't renderer specific. I know Antoine initially opposed this idea, but I do think we need to find a way to enforce this in a generic way... assigning to Saman since it relates to his DelegatedFrameHost cleanup work.
,
Apr 18 2017
For example, Chrome running in Mushrome or Mus+Ash does not use DelegatedFrameHost currently and CompositorFrames will not go to RenderWidgetHostImpl...
,
Apr 19 2017
,
May 3 2017
,
May 23 2017
,
May 29 2017
,
Jun 13 2017
,
Feb 26 2018
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by fsam...@chromium.org
, Mar 17 2017