DisplayCompositor should enforce invariant that frame size and device scale factor are fixed

DisplayCompositor should enforce invariant that frame size and device scale factor are fixed
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

[Saman Sami, 2017-03-17 19:42:17]

Description was changed from

==========
DisplayCompositor should enforce invariant that frame size and device scale
factor are fixed
==========

to

==========
DisplayCompositor should enforce invariant that frame size and device scale
factor are fixed
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-03-17 19:57:44]

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

[Fady Samuel, 2017-03-17 19:57:45]

Some initial thoughts.

https://codereview.chromium.org/2757953002/diff/1/cc/surfaces/surface.h
File cc/surfaces/surface.h (right):

https://codereview.chromium.org/2757953002/diff/1/cc/surfaces/surface.h#newco...
cc/surfaces/surface.h:144: SurfaceId surface_id_;
We don't need this?

https://codereview.chromium.org/2757953002/diff/1/cc/surfaces/surface.h#newco...
cc/surfaces/surface.h:145: SurfaceId previous_frame_surface_id_;
previous_surface_info_?

https://codereview.chromium.org/2757953002/diff/1/cc/surfaces/surface.h#newco...
cc/surfaces/surface.h:163: base::Optional<SurfaceInfo> surface_info_;
I wouldn't make this optional. I would instead take this in the constructor?

[piman, 2017-03-17 20:12:41]

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

[piman, 2017-03-17 20:12:42]

https://codereview.chromium.org/2757953002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/delegated_frame_host.cc (right):

https://codereview.chromium.org/2757953002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/delegated_frame_host.cc:518: void
DelegatedFrameHost::OnBadFrameReceived() {
The callback mechanism seems extremely unfortunate:
1- you end up with a deep stack which makes it hard to reason about what you can
and can't do (e.g. can you destroy things?)
2- it makes it difficult to do things in response. For example in
SwapDelegatedFrame we would want to avoid processing any further, if the data is
inconsistent. As is, we would still do a bunch of stuff with that inconsistent
data (everything after SubmitCompositorFrame, including SetShowPrimarySurface,
updating current_surface_size_/current_scale_factor_, calling CheckResizeLocks,
UpdateGutters etc.)
3- the callback may come too late. We may have done things (everything before
SubmitCompositorFrame) that we may want to undo, or something. It's not very
robust to refactoring

How about instead a bool Surface::CheckConsistentFrame(const CompositorFrame&) -
that can be forwarded through the call chain
(CompositorFrameSinkSupport->SurfaceFactory->Surface) and also DCHECK'ed in
Surface::QueueFrame? We could call that one early and just early out the
SwapDelegatedFrame (with or without a ReceivedBadMessage).

[Fady Samuel, 2017-03-17 20:17:11]

On 2017/03/17 20:12:42, piman wrote:
>
https://codereview.chromium.org/2757953002/diff/1/content/browser/renderer_ho...
> File content/browser/renderer_host/delegated_frame_host.cc (right):
> 
>
https://codereview.chromium.org/2757953002/diff/1/content/browser/renderer_ho...
> content/browser/renderer_host/delegated_frame_host.cc:518: void
> DelegatedFrameHost::OnBadFrameReceived() {
> The callback mechanism seems extremely unfortunate:
> 1- you end up with a deep stack which makes it hard to reason about what you
can
> and can't do (e.g. can you destroy things?)
> 2- it makes it difficult to do things in response. For example in
> SwapDelegatedFrame we would want to avoid processing any further, if the data
is
> inconsistent. As is, we would still do a bunch of stuff with that inconsistent
> data (everything after SubmitCompositorFrame, including SetShowPrimarySurface,
> updating current_surface_size_/current_scale_factor_, calling
CheckResizeLocks,
> UpdateGutters etc.)
> 3- the callback may come too late. We may have done things (everything before
> SubmitCompositorFrame) that we may want to undo, or something. It's not very
> robust to refactoring
> 
> How about instead a bool Surface::CheckConsistentFrame(const CompositorFrame&)
-
> that can be forwarded through the call chain
> (CompositorFrameSinkSupport->SurfaceFactory->Surface) and also DCHECK'ed in
> Surface::QueueFrame? We could call that one early and just early out the
> SwapDelegatedFrame (with or without a ReceivedBadMessage).

What if DelegatedFrameHost instead listened to SurfaceObserver::OnSurfaceCreated
to update gutter/surfacelayer? This better mimics the end state behavior when
the display compositor moves out of process and CompositorFrames do not arrive
directly?

[piman, 2017-03-17 20:21:18]

On Fri, Mar 17, 2017 at 1:17 PM, <fsamuel@chromium.org> wrote:

> On 2017/03/17 20:12:42, piman wrote:
> >
> https://codereview.chromium.org/2757953002/diff/1/content/
> browser/renderer_host/delegated_frame_host.cc
> > File content/browser/renderer_host/delegated_frame_host.cc (right):
> >
> >
> https://codereview.chromium.org/2757953002/diff/1/content/
> browser/renderer_host/delegated_frame_host.cc#newcode518
> > content/browser/renderer_host/delegated_frame_host.cc:518: void
> > DelegatedFrameHost::OnBadFrameReceived() {
> > The callback mechanism seems extremely unfortunate:
> > 1- you end up with a deep stack which makes it hard to reason about what
> you
> can
> > and can't do (e.g. can you destroy things?)
> > 2- it makes it difficult to do things in response. For example in
> > SwapDelegatedFrame we would want to avoid processing any further, if the
> data
> is
> > inconsistent. As is, we would still do a bunch of stuff with that
> inconsistent
> > data (everything after SubmitCompositorFrame, including
> SetShowPrimarySurface,
> > updating current_surface_size_/current_scale_factor_, calling
> CheckResizeLocks,
> > UpdateGutters etc.)
> > 3- the callback may come too late. We may have done things (everything
> before
> > SubmitCompositorFrame) that we may want to undo, or something. It's not
> very
> > robust to refactoring
> >
> > How about instead a bool Surface::CheckConsistentFrame(const
> CompositorFrame&)
> -
> > that can be forwarded through the call chain
> > (CompositorFrameSinkSupport->SurfaceFactory->Surface) and also
> DCHECK'ed in
> > Surface::QueueFrame? We could call that one early and just early out the
> > SwapDelegatedFrame (with or without a ReceivedBadMessage).
>
> What if DelegatedFrameHost instead listened to SurfaceObserver::
> OnSurfaceCreated
> to update gutter/surfacelayer? This better mimics the end state behavior
> when
> the display compositor moves out of process and CompositorFrames do not
> arrive
> directly?
>

What's wrong with validating messages before doing anything with them?



>
> https://codereview.chromium.org/2757953002/
>

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

[Fady Samuel, 2017-03-17 20:46:44]

Thinking out loud.

The validation would ultimately be in the gpu process. The Gpu process can't
kill clients, and so it needs to signal back to some privileged process [display
compositor host: window server / browser] to kill the client. Setting a layer in
browser UI will ultimately not happen on the same call stack as
CompositorFrameSinkSupport::SubmitCompositorFrame because they are different
processes. With surface sync, we will do that immediately, and let the display
compositor do the synchronization. Without, we wait until the display compositor
receives a CompositorFrame from the child with a given surface ID.

1. Parent allocates surface ID for child.
2. Parent tells child about surface ID.
3. Child submits a CompositorFrame with new Surface ID and uses it for
subsequent frames.
4. Parent assumes a certain SurfaceInfo as its primary SurfaceInfo.
5. DisplayCompositor receives frame from child, and forwards it along to the
parent to use as the fallback SurfaceInfo.

With Surface sync, we need to validate size/device scale factor in surface
aggregator of the child. That is something I am probably not enforcing yet.

We can, in addition, validate every CompositorFrame the child submits to the
display compositor to make sure the invariant is maintained throughout the
lifetime of the surface. I suppose we can be lazy and only do this in
SurfaceAggregator? The parent submits its expectations in a SurfaceDrawQuad and
if they are violated during SurfaceAggregation, then we don't show the child?

WDYT?


On 2017/03/17 20:21:18, piman wrote:
> On Fri, Mar 17, 2017 at 1:17 PM, <mailto:fsamuel@chromium.org> wrote:
> 
> > On 2017/03/17 20:12:42, piman wrote:
> > >
> > https://codereview.chromium.org/2757953002/diff/1/content/
> > browser/renderer_host/delegated_frame_host.cc
> > > File content/browser/renderer_host/delegated_frame_host.cc (right):
> > >
> > >
> > https://codereview.chromium.org/2757953002/diff/1/content/
> > browser/renderer_host/delegated_frame_host.cc#newcode518
> > > content/browser/renderer_host/delegated_frame_host.cc:518: void
> > > DelegatedFrameHost::OnBadFrameReceived() {
> > > The callback mechanism seems extremely unfortunate:
> > > 1- you end up with a deep stack which makes it hard to reason about what
> > you
> > can
> > > and can't do (e.g. can you destroy things?)
> > > 2- it makes it difficult to do things in response. For example in
> > > SwapDelegatedFrame we would want to avoid processing any further, if the
> > data
> > is
> > > inconsistent. As is, we would still do a bunch of stuff with that
> > inconsistent
> > > data (everything after SubmitCompositorFrame, including
> > SetShowPrimarySurface,
> > > updating current_surface_size_/current_scale_factor_, calling
> > CheckResizeLocks,
> > > UpdateGutters etc.)
> > > 3- the callback may come too late. We may have done things (everything
> > before
> > > SubmitCompositorFrame) that we may want to undo, or something. It's not
> > very
> > > robust to refactoring
> > >
> > > How about instead a bool Surface::CheckConsistentFrame(const
> > CompositorFrame&)
> > -
> > > that can be forwarded through the call chain
> > > (CompositorFrameSinkSupport->SurfaceFactory->Surface) and also
> > DCHECK'ed in
> > > Surface::QueueFrame? We could call that one early and just early out the
> > > SwapDelegatedFrame (with or without a ReceivedBadMessage).
> >
> > What if DelegatedFrameHost instead listened to SurfaceObserver::
> > OnSurfaceCreated
> > to update gutter/surfacelayer? This better mimics the end state behavior
> > when
> > the display compositor moves out of process and CompositorFrames do not
> > arrive
> > directly?
> >
> 
> What's wrong with validating messages before doing anything with them?
> 
> 
> 
> >
> > https://codereview.chromium.org/2757953002/
> >
> 
> -- 
> 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.

[piman, 2017-03-17 21:37:23]

On Fri, Mar 17, 2017 at 1:46 PM, <fsamuel@chromium.org> wrote:

> Thinking out loud.
>
> The validation would ultimately be in the gpu process. The Gpu process
> can't
> kill clients, and so it needs to signal back to some privileged process
> [display
> compositor host: window server / browser] to kill the client.
>

I'm not sure killing the client is critical here, especially in that model.
Arguably I'm not sure we should let the GPU process tell the browser to
kill arbitrary clients (if we consider the GPU process somewhat untrusted).
But it's a separate concern.
The important part is to refuse bad messages as early as possible, to avoid
propagating unvalidated (possibly malicious) data. Any class that
unvalidated data touches needs to be resilient (i.e. hardened and tested)
for the full range of values, not only the legitimate ones, it increases
the long-term burden. So that's why it's preferable to early-reject
messages with bad values so that we don't have to worry about it any more -
in particular, the closest to the IPC, the better. Ideally the
ParamTraits/StructTraits can do that (for "syntactic" validation - e.g.
enums, valid range, etc.). In this particular case, we need
cross-validation (data sent with an earlier IPC needs to match data sent
with a later IPC), so we can't do that at the StructTraits/ParamTraits
level, but ideally we can do that as close to the IPC as possible, that is,
at the function entry.
That way, Surface/SurfaceFactory don't need to concern themselves with
security properties, they just provide the tools to ensure consistency -
it's up to the IPC-related class (e.g. RWHVAura, delegating to DFH here) to
ensure it.


> Setting a layer in
> browser UI will ultimately not happen on the same call stack as
> CompositorFrameSinkSupport::SubmitCompositorFrame because they are
> different
> processes. With surface sync, we will do that immediately, and let the
> display
> compositor do the synchronization. Without, we wait until the display
> compositor
> receives a CompositorFrame from the child with a given surface ID.
>
> 1. Parent allocates surface ID for child.
> 2. Parent tells child about surface ID.
> 3. Child submits a CompositorFrame with new Surface ID and uses it for
> subsequent frames.
> 4. Parent assumes a certain SurfaceInfo as its primary SurfaceInfo.
> 5. DisplayCompositor receives frame from child, and forwards it along to
> the
> parent to use as the fallback SurfaceInfo.
>
> With Surface sync, we need to validate size/device scale factor in surface
> aggregator of the child. That is something I am probably not enforcing yet.
>
> We can, in addition, validate every CompositorFrame the child submits to
> the
> display compositor to make sure the invariant is maintained throughout the
> lifetime of the surface. I suppose we can be lazy and only do this in
> SurfaceAggregator? The parent submits its expectations in a
> SurfaceDrawQuad and
> if they are violated during SurfaceAggregation, then we don't show the
> child?
>
> WDYT?
>

I think there's 2 separate things here:
1- is the child correctly creating a new local id when the size/DSF changes?
  -> this is something we can easily check upon receipt of a
CompositorFrame when the local id is reused, and discard the message, since
this is obviously a child doing something wrong. This is true regardless of
parenting relationships. That way we can correctly reason about e.g. damage
rects.
2- does the child's surface match the expectation for the parent?
  -> this is something we only know at aggregation in the end state. But
now we know that any check is valid regardless of whether a new frame has
been submitted or not, as long as the local id didn't change. Either the
child's surface always matches the parent's expectation, or it never does.

Antoine


>
>
> On 2017/03/17 20:21:18, piman wrote:
>
> > On Fri, Mar 17, 2017 at 1:17 PM, <mailto:fsamuel@chromium.org> wrote:
> >
> > > On 2017/03/17 20:12:42, piman wrote:
> > > >
> > > https://codereview.chromium.org/2757953002/diff/1/content/
> > > browser/renderer_host/delegated_frame_host.cc
> > > > File content/browser/renderer_host/delegated_frame_host.cc (right):
> > > >
> > > >
> > > https://codereview.chromium.org/2757953002/diff/1/content/
> > > browser/renderer_host/delegated_frame_host.cc#newcode518
> > > > content/browser/renderer_host/delegated_frame_host.cc:518: void
> > > > DelegatedFrameHost::OnBadFrameReceived() {
> > > > The callback mechanism seems extremely unfortunate:
> > > > 1- you end up with a deep stack which makes it hard to reason about
> what
> > > you
> > > can
> > > > and can't do (e.g. can you destroy things?)
> > > > 2- it makes it difficult to do things in response. For example in
> > > > SwapDelegatedFrame we would want to avoid processing any further, if
> the
> > > data
> > > is
> > > > inconsistent. As is, we would still do a bunch of stuff with that
> > > inconsistent
> > > > data (everything after SubmitCompositorFrame, including
> > > SetShowPrimarySurface,
> > > > updating current_surface_size_/current_scale_factor_, calling
> > > CheckResizeLocks,
> > > > UpdateGutters etc.)
> > > > 3- the callback may come too late. We may have done things
> (everything
> > > before
> > > > SubmitCompositorFrame) that we may want to undo, or something. It's
> not
> > > very
> > > > robust to refactoring
> > > >
> > > > How about instead a bool Surface::CheckConsistentFrame(const
> > > CompositorFrame&)
> > > -
> > > > that can be forwarded through the call chain
> > > > (CompositorFrameSinkSupport->SurfaceFactory->Surface) and also
> > > DCHECK'ed in
> > > > Surface::QueueFrame? We could call that one early and just early out
> the
> > > > SwapDelegatedFrame (with or without a ReceivedBadMessage).
> > >
> > > What if DelegatedFrameHost instead listened to SurfaceObserver::
> > > OnSurfaceCreated
> > > to update gutter/surfacelayer? This better mimics the end state
> behavior
> > > when
> > > the display compositor moves out of process and CompositorFrames do not
> > > arrive
> > > directly?
> > >
> >
> > What's wrong with validating messages before doing anything with them?
> >
> >
> >
> > >
> > > https://codereview.chromium.org/2757953002/
> > >
> >
> > --
> > 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.
>
>
>
> https://codereview.chromium.org/2757953002/
>

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

[Saman Sami, 2017-05-23 23:13:37]

Description was changed from

==========
DisplayCompositor should enforce invariant that frame size and device scale
factor are fixed
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
DisplayCompositor should enforce invariant that frame size and device scale
factor are fixed
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========