We can close the message pipe instead, like we do for bad surfaces?

We do already in Viz code, yes. The problem is the code above cannot live in Viz. 

Right now Viz doesn't track these properties, and I'd argue it probably shouldn't? Maybe in the short term we can track them in Viz (CompositorFrameSinkSupport)?

// static
RenderWidgetSurfaceProperties
RenderWidgetSurfaceProperties::FromCompositorFrame(
    const viz::CompositorFrame& frame) {
  RenderWidgetSurfaceProperties properties;
  properties.size = frame.size_in_pixels();
  properties.device_scale_factor = frame.device_scale_factor();
#ifdef OS_ANDROID
  properties.top_controls_height = frame.metadata.top_controls_height;
  properties.top_controls_shown_ratio = frame.metadata.top_controls_shown_ratio;
  properties.bottom_controls_height = frame.metadata.bottom_controls_height;
  properties.bottom_controls_shown_ratio =
      frame.metadata.bottom_controls_shown_ratio;
  properties.selection = frame.metadata.selection;
  properties.has_transparent_background =
      frame.render_pass_list.back()->has_transparent_background;
#endif
  return properties;
}

Once renderers' frames go directly to CFSS, we should remove this code because CFSS also performs the check for size and device scale factor, and we should just forget about the Android checks.

This change will be delayed.

Currently this allows us to crash/restart a renderer which is providing bad data.
Removing this now would lead to lots of work being done before Viz detects the error and stops processing it.

Until the rest of the SubmitCompositorFrame cleanup is done we need to keep this here.

So while this blocks 775030, it is effectively blocked by the rest of the sub-bugs listed there.

I talked about this with fsamuel and how this could work.

We can (and sort of do) check for surface invariant violations in CompositorFrameSinkSupport now. We don't check all of the properties for Android but those could be added.

CompositorFrameSinkSupport::SubmitCompositorFrame() returns false when there is a surface invariant violation. For code that still directly submits CompositorFrames via CompositorFrameSinkSupport the usual way to handle this to DCHECK on that return value. That crashes the browser process in debug mode but does nothing for release builds. We still want this behaviour.

We also want to keep the behaviour found in RenderWidgetHostImpl where if a renderer submits a bad CompositorFrame we kill the renderer process. The flow has to be something like (1) renderer submits CompositorFrame to viz (2) viz finds a surface invariant violation (3) viz tells host (browser) about surface invariant violation and (4) browser kills the renderer process. Viz can't directly tell the renderer do anything, since it's already misbehaving and might ignore requests.

We also don't want the behaviour where viz closes the mojom::CompositorFrameSink message pipe on surface invariant violations. This is fine for the a renderer that will be killed shortly but isn't fine if something in the browser process (like ui::Compositor) does the same. That would end up making the browser unresponsive with no indication of why.

We talked about doing something like the following:
(1) CompositorFrameSinkSupport tells FrameSinkManagerImpl about surface invariant violation.
(2) FrameSinkManagerImpl tells mojom::FrameSinkManagerClient about FrameSinkId + invariant violation.
(3) HostFrameSinkManager finds HostFrameSinkClient for FrameSinkId and calls HostFrameSinkClient::OnSurfaceInvariantsViolation().
(4) The implementation of HostFrameSinkClient can do something appropriate, either kill the renderer process or just a NOTREACHED() for things like the ui::Compositor.

Since Viz is not a trusted process, is it appropriate to let it kill arbitrary clients?

> We don't check all of the properties for Android but those could be added.

I think Viz shouldn't check properties for Android. The only properties that seem appropriate to check in Viz are size and device scale factor IMO.

> Since Viz is not a trusted process, is it appropriate to let it kill arbitrary clients?

Viz wouldn't be able to kill arbitrary clients, it can only report the client submitted a CompositorFrame that violated some invariant. The browser process can choose to kill a client based on that invariant violation.

> I think Viz shouldn't check properties for Android. The only properties that seem appropriate to check in Viz are size and device scale factor IMO.

We should look at the original justification for adding those checks on Android. It would be easier if they weren't required.

> Viz wouldn't be able to kill arbitrary clients, it can only report the client submitted a CompositorFrame that violated some invariant. The browser process can choose to kill a client based on that invariant violation.

I think my argument is still valid. Viz is not killing the client directly, but it's still killing it. The browser should not trust the signal coming from Viz that could result in killing renderers. 

> We should look at the original justification for adding those checks on Android. It would be easier if they weren't required.

These checks should be client side imo. Any two embedder/embedee can have their own approach for allocating new ids and Viz doesn't have to be aware of it or verify it. 

fsamuel@ mentioned possibly addressing this this sprint.

Another flavor of this has come up recently: I was adding code to prohibit CopyOutputRequests from renderers from executing. Basically, for the non-VIZ case, I was doing the check in RWHostImpl::SubmitCompositorFrame(), right next to the surface invariants check; and this would allow RWHostImpl to kill a render process if it tries to make any copy requests.

For VIZ, currently, if there is a copy request or surface invariant violation, all that will happen is the mojo pipe will close. Then, the renderer will just re-request a new CompositorFrameSink. In other words, the renderer that sent the bad message gets to live; and that's bad.

My initial knee-jerk solution to all this was to add a "wrapper" in RWHostImpl in RequestCompositorFrameSink(). This would allow RWHostImpl to monitor for violations with each call to SubmitCompositorFrame() and kill the renderer before even forwarding the request to the compositor in the VIZ process. This solution is great in terms of having the browser process make the decision to instantly kill the renderer. However, it has a major downside: All mojo calls have to hop from renderer→browser→VIZ processes. (Without the wrapper, the pipe can be set up as a direct renderer→VIZ process hop.)

However, maybe the extra hop is warranted for security reasons? I just hate to think it requires a minimum of two context switches for every call to SubmitCompositorFrame(). On 2-core or single-core machines, that would likely add a lot of latency.

BTW--Comment #13 is in regards to: https://chromium-review.googlesource.com/c/chromium/src/+/890339

I don't believe it is *necessary* for a misbehaving renderer to be killed. It makes it harder to carry out attacks undetected, but it is not what ensures security in itself. So I don't think we need to hop to the browser.

That being said, if we wanted to, having viz signal the browser that a renderer is misbehaving isn't really much of a problem, I think. A compromized viz could do a lot of bad things to that renderer anyway, like dropping its connection (rendering it useless) and displaying arbitrary content on the screen.

Yea, I don't expect viz => browser for invariants violations and other errors to be much of an issue because they ought to be relatively uncommon.

Are there any security implications if we don't kill the renderer? I can't think of any. My understanding is that we are only using surface invariants violation to find bugs, so can we just DCHECK (or CHECK) client side when the mojo connection is closed due to surface invariants violation? That would be a lot simpler.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2cd75532d445dd15f64d50c3794b049ef5201847

commit 2cd75532d445dd15f64d50c3794b049ef5201847
Author: Fady Samuel <fsamuel@chromium.org>
Date: Thu Feb 08 01:10:10 2018

Viz: Remove DCHECK_IS_ON around HostFrameSinkManager::SetFrameSinkDebugLabel

Now that site isolation is turned on, we would like to be able to identify which
clients are violating surface invariants. We can report the FrameSink debug
label in crash logs if they are used in release builds.

Bug:  771354 ,  672962 
TBR: samans@chromium.org, boliu@chromium.org, sadrul@chromium.org
Change-Id: I9b2df6acf6489d08fddf7a673d885571d59a616e
Reviewed-on: https://chromium-review.googlesource.com/907742
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535231}
[modify] https://crrev.com/2cd75532d445dd15f64d50c3794b049ef5201847/android_webview/browser/hardware_renderer.cc
[modify] https://crrev.com/2cd75532d445dd15f64d50c3794b049ef5201847/content/browser/renderer_host/compositor_impl_android.cc
[modify] https://crrev.com/2cd75532d445dd15f64d50c3794b049ef5201847/content/browser/renderer_host/delegated_frame_host.cc
[modify] https://crrev.com/2cd75532d445dd15f64d50c3794b049ef5201847/content/browser/renderer_host/offscreen_canvas_surface_impl.cc
[modify] https://crrev.com/2cd75532d445dd15f64d50c3794b049ef5201847/content/browser/renderer_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/2cd75532d445dd15f64d50c3794b049ef5201847/services/ui/ws/server_window.cc
[modify] https://crrev.com/2cd75532d445dd15f64d50c3794b049ef5201847/ui/android/delegated_frame_host_android.cc
[modify] https://crrev.com/2cd75532d445dd15f64d50c3794b049ef5201847/ui/aura/local/layer_tree_frame_sink_local.cc
[modify] https://crrev.com/2cd75532d445dd15f64d50c3794b049ef5201847/ui/compositor/compositor.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/483530c1094f0ad6fe36663262b484be300f472e

commit 483530c1094f0ad6fe36663262b484be300f472e
Author: Yuri Wiitala <miu@chromium.org>
Date: Thu Feb 08 07:17:34 2018

Layer CopyOutputRequests execute in VIZ service.

Implements the final missing pieces allowing CopyOutputRequests made on
Layers to propagate and execute in the VIZ display compositor:

1. Added missing mojo fields and struct traits for viz::CopyOutputRequest
and viz::RenderPass.

2. Added extra transform code to cc::EffectTree::TakeCRsAndTransform()
to account for the scaling ratio, not just the source area.

3. Updated DelegatedFrameHost::CopyFromCompositingSurface() to use the
new scaling APIs, enabling testing and otherwise proving that all "the
plumbing" is in-place.

4. Updated RenderWidgetHostViewBrowserTests so that they now work with
--enabled-features=VizDisplayCompositor. (And, disabled dead tests I did
not feel we should bother updating; to be removed in a later change.)

5. Added privilege checks to make sure CopyOutputRequests cannot be made
from unprivileged clients. Added new RWHostImpl browser test to confirm.

Testing (all with and without --enable-features=VizDisplayCompositor):

1. All RenderWidgetHostViewBrowserTest content_browsertests pass.

2. On CrOS Ash desktop, tested CTRL-F5 (full screenshot) and
SHIFT-CTRL-F5 (partial screenshot).

3. On Linux, confirmed chrome.captureVisibleTab() screenshot API works
via the common/extensions/docs/examples/api/tabs/screenshot demo
extension.

Bug:  806375 ,  806366 ,  759310 ,  771354 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I98ad6cd90f332318dbdea9449e5d9c3a31679114
Reviewed-on: https://chromium-review.googlesource.com/890339
Commit-Queue: Yuri Wiitala <miu@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535325}
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/cc/trees/property_tree.cc
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/components/viz/common/quads/compositor_frame.cc
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/components/viz/common/quads/compositor_frame.h
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/components/viz/common/quads/render_pass.h
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/components/viz/service/display/surface_aggregator_unittest.cc
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/components/viz/service/frame_sinks/compositor_frame_sink_impl.cc
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/components/viz/service/frame_sinks/compositor_frame_sink_support.cc
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/components/viz/service/frame_sinks/compositor_frame_sink_support.h
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/components/viz/service/frame_sinks/root_compositor_frame_sink_impl.cc
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/components/viz/test/test_layer_tree_frame_sink.cc
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/content/browser/bad_message.h
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/content/browser/renderer_host/delegated_frame_host.cc
[add] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/content/browser/renderer_host/render_widget_host_browsertest.cc
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/content/browser/renderer_host/render_widget_host_view_browsertest.cc
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/content/test/BUILD.gn
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/services/viz/public/cpp/compositing/copy_output_request_struct_traits.cc
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/services/viz/public/cpp/compositing/copy_output_request_struct_traits.h
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/services/viz/public/cpp/compositing/render_pass_struct_traits.cc
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/services/viz/public/cpp/compositing/render_pass_struct_traits.h
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/services/viz/public/cpp/compositing/struct_traits_unittest.cc
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/services/viz/public/interfaces/compositing/copy_output_request.mojom
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/services/viz/public/interfaces/compositing/render_pass.mojom
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/testing/buildbot/filters/viz.content_browsertests.filter
[modify] https://crrev.com/483530c1094f0ad6fe36663262b484be300f472e/tools/metrics/histograms/enums.xml

At this point, I don't see a huge amount of value to this. Assigning to mohsen@. Please just delete this block of code.

Is this still an issue? It seems that RWH_SURFACE_INVARIANTS_VIOLATION is not used anymore (removed in https://crrev.com/c/926913).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/51a81d39fc6d3db54b04174afda85b826c50fc99

commit 51a81d39fc6d3db54b04174afda85b826c50fc99
Author: Mohsen Izadi <mohsen@chromium.org>
Date: Tue Sep 11 18:18:50 2018

Remove surface invariants violation check in RenderWidgetHostImpl

RenderWidgetHostImpl::SubmitCompositorFrame() checks for surface
invariants violation and records Compositing.SurfaceInvariantsViolations
histogram in case of violation. However, CompositorFrameSinkSupport
already checks for violations and records appropriate histogram values
(Compositing.CompositorFrameSinkSupport.SubmitResult).

So, the check and histogram in RenderWidgetHostImpl is not very useful
and can be removed.

BUG= 771354 

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I24a9e8078be17175e588383259237f27ae8c28b2
Reviewed-on: https://chromium-review.googlesource.com/1217706
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Mohsen Izadi <mohsen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590406}
[modify] https://crrev.com/51a81d39fc6d3db54b04174afda85b826c50fc99/components/viz/common/features.cc
[modify] https://crrev.com/51a81d39fc6d3db54b04174afda85b826c50fc99/components/viz/common/features.h
[modify] https://crrev.com/51a81d39fc6d3db54b04174afda85b826c50fc99/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/51a81d39fc6d3db54b04174afda85b826c50fc99/content/browser/renderer_host/render_widget_host_impl.h
[modify] https://crrev.com/51a81d39fc6d3db54b04174afda85b826c50fc99/tools/metrics/histograms/histograms.xml

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/257348f17429e146468c96a9ceddb6753a2cae39

commit 257348f17429e146468c96a9ceddb6753a2cae39
Author: Mohsen Izadi <mohsen@chromium.org>
Date: Wed Sep 12 14:53:26 2018

Remove copy request check in RenderWidgetHostImpl

This is already checked in CompositorFrameSinkSupport for both OOP-D and
non-OOP-D cases.

BUG= 771354 

Change-Id: I2723afdfc58f03cdad6cd40a6bdf6e108b5f4cc1
Reviewed-on: https://chromium-review.googlesource.com/1219913
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590679}
[modify] https://crrev.com/257348f17429e146468c96a9ceddb6753a2cae39/content/browser/renderer_host/input/input_router_impl.h
[modify] https://crrev.com/257348f17429e146468c96a9ceddb6753a2cae39/content/browser/renderer_host/render_widget_host_browsertest.cc
[modify] https://crrev.com/257348f17429e146468c96a9ceddb6753a2cae39/content/browser/renderer_host/render_widget_host_impl.cc