+fserb@ - Any chance you might have some cycles to help look into this. These are some of the failures that are blocking the scheduling change in Mojo.

And just to clarify, these failures appear when applying this CL[1]. The CL does not break any guarantees made by Mojo bindings, but it can affect the timing of arbitrary events. The failures therefore strongly imply that incorrect ordering assumptions being made somewhere, and this has been the case for all other blocking bugs which have been fixed so far.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1145692

Taking a look at it.

It seems that original CL breaks copyImageAtAndCapturePixelsAsyncThen... this is the cause of all copy-image and commit-copyimage test failures (not sure about the printing ones yet).

The problem is:
https://cs.chromium.org/chromium/src/content/shell/test_runner/pixel_dump.cc?dr=CSs&g=0&l=181

It has to do with how this function communicates with MockClipboardHost.

It calls GetSequenceNumber first (a mojo sync API)
then it calls web_frame->CopyImageAt (this eventually calls a non-sync API: CommitWrite)
and then it calls GetSequenceNumber again, expecting a different number.

The new CL makes CommitWrite happen AFTER GetSequenceNumber. Which seems to be OK mojo-wise, but clearly breaks the code logic.

I'm cc-ing some people who may know this code better, meanwhile I'll try to see if there's an easy way to refactor this function.

This is the proposed solution to the above problem:
https://chromium-review.googlesource.com/c/chromium/src/+/1176078

Still needs to investigate whatever is left after this lands.
rockot, please rerun everything once this lands and update this bug.

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

commit be5a7b1b933239a6430120b4a2a616384c7784bf
Author: Fernando Serboncini <fserb@chromium.org>
Date: Wed Aug 15 20:53:31 2018

Fix CopyImageAtAndCapturePixel for new Mojo sequencer

The old function assumed that a async mojo calls between two sync calls
would happen in sequence. This is not true at all.

Bug: 872076
Change-Id: I9b7f78be76352c6133638665c60b156902fff1eb
Reviewed-on: https://chromium-review.googlesource.com/1176078
Commit-Queue: Fernando Serboncini <fserb@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583386}
[modify] https://crrev.com/be5a7b1b933239a6430120b4a2a616384c7784bf/content/shell/test_runner/pixel_dump.cc
[modify] https://crrev.com/be5a7b1b933239a6430120b4a2a616384c7784bf/third_party/WebKit/LayoutTests/TestExpectations
[delete] https://crrev.com/e516ef4da00cf8d3934d2c02a47d0ca860fabe97/third_party/WebKit/LayoutTests/compositing/copy-image-when-no-image-exists.html
[delete] https://crrev.com/e516ef4da00cf8d3934d2c02a47d0ca860fabe97/third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-commit-copyImage.html
[add] https://crrev.com/be5a7b1b933239a6430120b4a2a616384c7784bf/third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-copyImage.html

Just to update this, some of the tests appear to have been fixed, probably by the CL in comment #7:

  compositing/webgl/webgl-copy-image.html
  virtual/threaded/compositing/webgl/webgl-copy-image.html

It also appears that one of the tests has been removed or possibly renamed (virtual/gpu/fast/canvas/OffscreenCanvas-commit-copyImage.html).

These other tests remain broken:

  fast/canvas/OffscreenCanvas-placeholder-image-source.html
  printing/offscreencanvas-2d-printing.html
  printing/offscreencanvas-webgl-printing.html
  virtual/layout_ng_experimental/printing/offscreencanvas-2d-printing.html
  virtual/threaded/printing/offscreencanvas-2d-printing.html
  virtual/threaded/printing/offscreencanvas-webgl-printing.html

I have a fix for OffscreenCanvas-placeholder-image-source.html here: https://chromium-review.googlesource.com/1292717

It looks like it just has incorrect (and kind of arbitrary) expectations about when it can assume the placeholder canvas has been updated. I don't see why there would be any guarantee for example that it happens within two scheduling iterations. I changed it to just wait until it observes the correct contents. If anything breaks the test will time out.

The 2d-printing and webgl-printing tests look like exactly the same issue, but the TestRunner machinery used for them is pretty gnarly. There doesn't appear to be anything to observe in between waitUntilDone and notifyDone, but I can verify that if I do something stupid like increase the delay between them by even 1 millisecond, the test consistently passes.

fserb@ or lukasza@, any chance one of you could work out a way to fix those two tests?

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

commit a57c9e94bff6ac35768a226868381ebe3e5c5403
Author: Ken Rockot <rockot@chromium.org>
Date: Mon Oct 22 15:53:58 2018

Fix OffscreenCanvas-placeholder-image-source.html

This corrects the expectations of
fast/canvas/OffscreenCanvas-placeholder-image-source.html to align with
reality. There does not appear to be any kind of guarantee that the
placeholder canvas will be updated within the two scheduler iterations
the test was designed to wait for.

Instead the test now keeps checking the placeholder canvas until it's
updated, which still happens very quickly in practice. If something
breaks, the test will timeout.

Bug: 872076
Change-Id: Idd1a3018e63f5ad2df540d642f5abb17b80410d5
Reviewed-on: https://chromium-review.googlesource.com/c/1292717
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#601591}
[modify] https://crrev.com/a57c9e94bff6ac35768a226868381ebe3e5c5403/third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-placeholder-image-source.html

Any feedback or insight here would be appreciated. This will soon be the singular set of tests blocking the Mojo change from landing.

I'm having a difficult time making sense of what the intent of these tests are. Specifically I'm not sure what exactly test_runner::CapturePixelsForPrinting is supposed to capture, or why the result of capturing a frame with an offscreen canvas should match the result of capturing a frame with a normal canvas.

Per offline discussion with fserb@, going to disable the remaining tests for now to unblock the Mojo change. Thanks!

I am not familiar with the printing/offscreencanvas-*-printing.html tests and only a bit familiar with the layout test harness code for capturing an image.  The fact that these tests fail under virtual/threaded and virtual/layout_ng_experimental makes me think that something (the test?  the test harness?) needs to wait for compositing to actually happen before grabbing the pixels.  I don't really know much about it though... :-/

+danakj@ for any thoughts and threaded-compositing aspect of the failures
+vmpstr@ who might be more familiar with how pixel dumps are done (e.g. r553885)

threaded sounds like a red herring, the same tests fail outside of virtual/threaded/

Linking to parent bug. Taking a quick look at one of the WebGL tests.

https://chromium-review.googlesource.com/1297602 makes one of the failing OffscreenCanvas + WebGL printing tests pass reliably with https://chromium-review.googlesource.com/1145692 applied, but I'm not sure it's a principled fix.

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

commit a0e43753f591bc5b91760da04e89de86717d988b
Author: Ken Rockot <rockot@google.com>
Date: Wed Oct 24 02:06:17 2018

Temporarily disable some offscreen canvas tests

See bug for context.

Bug: 872076
Change-Id: I6aa25842174facb56f3e74fa88d99caf4d1d9680
Reviewed-on: https://chromium-review.googlesource.com/c/1297061
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#602217}
[modify] https://crrev.com/a0e43753f591bc5b91760da04e89de86717d988b/third_party/WebKit/LayoutTests/TestExpectations

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

commit a0e43753f591bc5b91760da04e89de86717d988b
Author: Ken Rockot <rockot@google.com>
Date: Wed Oct 24 02:06:17 2018

Temporarily disable some offscreen canvas tests

See bug for context.

Bug: 872076
Change-Id: I6aa25842174facb56f3e74fa88d99caf4d1d9680
Reviewed-on: https://chromium-review.googlesource.com/c/1297061
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#602217}
[modify] https://crrev.com/a0e43753f591bc5b91760da04e89de86717d988b/third_party/WebKit/LayoutTests/TestExpectations

All tests mentioned at the top currently pass for me on master:

  compositing/webgl/webgl-copy-image.html
  fast/canvas/OffscreenCanvas-placeholder-image-source.html
  printing/offscreencanvas-2d-printing.html
  printing/offscreencanvas-webgl-printing.html
  virtual/gpu/fast/canvas/OffscreenCanvas-commit-copyImage.html
  virtual/layout_ng_experimental/printing/offscreencanvas-2d-printing.html
  virtual/threaded/compositing/webgl/webgl-copy-image.html
  virtual/threaded/printing/offscreencanvas-2d-printing.html
  virtual/threaded/printing/offscreencanvas-webgl-printing.html

Yes, master is not the issue, the issue is when https://chromium-review.googlesource.com/c/chromium/src/+/1145692 is applied.

@rockot Ah, thanks. I did not realize that

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/11cacaac140000

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/11cacaac140000

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

commit 084d28e8d4c458f639feb87bf76d2d67c92c3b13
Author: Aaron Krajeski <aaronhk@google.com>
Date: Mon Jan 07 20:46:11 2019

Use promises to remove race conditions from some canvas tests

Changes introduced here create flakiness in a few tests:
https://chromium-review.googlesource.com/c/chromium/src/+/1145692
This should fix that. Things are still SLIGHTLY flaky for the webGL
test (~1 failure in 20).

Bug: 872076
Change-Id: I52ee9b7e67f718dfabb89f21627abfd4ead4d605
Reviewed-on: https://chromium-review.googlesource.com/c/1342801
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Commit-Queue: Aaron Krajeski <aaronhk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620458}
[modify] https://crrev.com/084d28e8d4c458f639feb87bf76d2d67c92c3b13/third_party/blink/web_tests/TestExpectations
[modify] https://crrev.com/084d28e8d4c458f639feb87bf76d2d67c92c3b13/third_party/blink/web_tests/printing/offscreencanvas-2d-printing.html
[modify] https://crrev.com/084d28e8d4c458f639feb87bf76d2d67c92c3b13/third_party/blink/web_tests/printing/offscreencanvas-webgl-printing.html