Other than unbreaking headless and taking a fresher snapshot, moving CopyOutputRequests to the new surface has another important benefit. If primary is available and fallback != primary and the CopyOutputRequest is in the fallback surface, then we draw both fallback and primary surface. It's better to always send the CopyOutputRequest to the surface that will be presented on screen. Here is one way to do it:

- CompositorFrameSinkSupportL::RequestCopyOfSurface will stash the CopyOutputRequest and not send it to any particular Surface.

- In SurfaceAggregator::PreWalkTree, we already notify SurfaceManager that a surface will be drawn. We can notify the surface too.
https://cs.chromium.org/chromium/src/components/viz/service/display/surface_aggregator.cc?rcl=3457fe6c8dbd3da7a32778bbb167b0528431da07&l=848
 
- If a Surface finds out it's being drawn, it takes any stashed CopyOutputRequests from its CompositorFrameSinkSupport.

- It is possible that no surface of a CompositorFrameSink will be drawn on screen but we still want CopyOutputRequests to be processed. Here in SurfaceAggregator::CopyUndrawnSurfaces, also check for CopyOutputRequests in CompositorFrameSinkSupport and not just in the active frame. https://cs.chromium.org/chromium/src/components/viz/service/display/surface_aggregator.cc?rcl=3457fe6c8dbd3da7a32778bbb167b0528431da07&l=1104 Also modify Surface::TakeCopyOutputRequests to return the stashed CopyOutputRequests in addition to the ones it already has in the active surface.

As I said in the email thread, before Yi's CL the CopyOutputRequests would always go through the browser's compositor and because of the way surface sync works the CopyOutputRequest would be executed on the primary surface when available, so we're not really doing anything controversial here, we're just restoring the old behaviour.

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

commit 99f8d9ac643c71a84b6b7958c7a64749ebb85db1
Author: Saman Sami <samans@chromium.org>
Date: Fri Mar 02 15:48:15 2018

viz: Always move CopyOutputRequests do the surface being drawn

Currently CopyOutputRequests given to CompositorFrameSinkSupport end up in whatever
surface happens to be the last activated surface at the time. This could result in
several bad scenarios such as CopyOutputRequests being dropped, handled late,
applied on an old surface taking a stale snapshot, or force SurfaceAggregator to
draw two surfaces of one frame sink at the same time.  Always move CopyOutputRequests
given to CompositorFrameSinkSupport to the surface that SurfaceAggregator will draw.

Bug:  817364 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I2a4dffdb17c3b76260ab40a456604c1a73ca1c07
Reviewed-on: https://chromium-review.googlesource.com/943940
Commit-Queue: Saman Sami <samans@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540525}
[modify] https://crrev.com/99f8d9ac643c71a84b6b7958c7a64749ebb85db1/components/viz/service/display/surface_aggregator.cc
[modify] https://crrev.com/99f8d9ac643c71a84b6b7958c7a64749ebb85db1/components/viz/service/frame_sinks/compositor_frame_sink_support.cc
[modify] https://crrev.com/99f8d9ac643c71a84b6b7958c7a64749ebb85db1/components/viz/service/frame_sinks/compositor_frame_sink_support.h
[modify] https://crrev.com/99f8d9ac643c71a84b6b7958c7a64749ebb85db1/components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc
[modify] https://crrev.com/99f8d9ac643c71a84b6b7958c7a64749ebb85db1/components/viz/service/surfaces/surface.cc
[modify] https://crrev.com/99f8d9ac643c71a84b6b7958c7a64749ebb85db1/components/viz/service/surfaces/surface.h
[modify] https://crrev.com/99f8d9ac643c71a84b6b7958c7a64749ebb85db1/components/viz/service/surfaces/surface_client.h
[modify] https://crrev.com/99f8d9ac643c71a84b6b7958c7a64749ebb85db1/components/viz/service/surfaces/surface_unittest.cc

saman: Is this fixed?

Let me try running headless tests.

Thanks for the patch, Saman!

It might take a little more to make headless tests pass. For example, I don't think that viz passes on --run-all-compositor-stages-before-draw to the DisplayScheduler (as wait_for_all_surfaces_before_draw) in GpuDisplayProvider yet. I'm happy to look into it on Monday.

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

commit 33be507c236e6667c88e480195e5448a61ffb436
Author: Saman Sami <samans@chromium.org>
Date: Fri Mar 02 22:33:08 2018

headless: Revert the workaround code added for  crbug.com/817364 

This bug is fixed, so remove the workaround code.


Bug:  817364 
Change-Id: I2f62a77920c0ac869867ed96f7d50f53c535ddad
Reviewed-on: https://chromium-review.googlesource.com/946863
Reviewed-by: Eric Seckler <eseckler@chromium.org>
Commit-Queue: Eric Seckler <eseckler@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540662}
[modify] https://crrev.com/33be507c236e6667c88e480195e5448a61ffb436/headless/lib/headless_web_contents_browsertest.cc

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

commit 7c9c1f8265094ca9e587e95caecd0598cf9f3234
Author: Eric Seckler <eseckler@chromium.org>
Date: Mon Mar 05 12:09:11 2018

headless: Remove work-arounds for  bug 817364  from CompController tests

 crbug.com/817364  is fixed, so we don't need to take double screenshots
anymore.

Bug:  817364 
Change-Id: Ie5e467d21858dcec1f8fb52d4ff52df7c2edd472
Reviewed-on: https://chromium-review.googlesource.com/948484
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Commit-Queue: Eric Seckler <eseckler@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540803}
[modify] https://crrev.com/7c9c1f8265094ca9e587e95caecd0598cf9f3234/headless/public/util/compositor_controller_browsertest.cc