+ folks who probably know things about these tests. Anyone have cycles to help look into this?

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. These failures therefore imply that somewhere in the pipeline there are incorrect ordering assumptions being made.

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

I believe the VR test should be getting removed anyways shortly. +offenwanger@ who's been working on the XR layout tests a lot more than I have.

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

commit 27378e0364b705643275c861f4ba64b70572dff1
Author: Andrey Kosyakov <caseq@chromium.org>
Date: Thu Aug 09 16:27:24 2018

Deflake inspector-protocol/input/dispatchKeyEvent-focus.js

The keystrokes injected via Input.dispatchKeyEvent through the browser are
racing against Runtime.evaluate in the renderer, so we need to wait for
the async completion of the last dispatchKeyEvent before validating the
tests results with evaluate.

Bug:  872081 
Change-Id: Ic5ef41feba6a877c76c05de2e09e019a826756a1
Reviewed-on: https://chromium-review.googlesource.com/1168200
Reviewed-by: Andrey Lushnikov <lushnikov@chromium.org>

[modify] https://crrev.com/27378e0364b705643275c861f4ba64b70572dff1/third_party/WebKit/LayoutTests/inspector-protocol/input/dispatchKeyEvent-focus.js

In the http/tests/event-timing/event-timing-onloadthenobserve.html failure, we have the following script:

* Inject a click on a button and make the main thread busy for 300 ms.
* Wait: promise + setTimeout(0).
* Then, start an observer of clicks.
* Another round of inject a click, main thread busy, and wait.

After the Mojo CL, there seems to be some sort of race condition between the first click event and the observer start. I could deflake this but it seems like undesirable behavior? I think that the injection call happens on the main thread whereas the browser needs to handle the click, so maybe this does not imply that we're delaying real input. But it's not super clear to me why the first click is no longer happening before the observer all the time: the browser must have had enough time to notify the renderer about the click before the main thread arrives to the wait period.

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

commit 4af14ca3c99bd7edbf100e05db93564b7107cc62
Author: Andrii Shyshkalov <tandrii@chromium.org>
Date: Thu Aug 09 19:09:51 2018

Revert "Deflake inspector-protocol/input/dispatchKeyEvent-focus.js"

This reverts commit 27378e0364b705643275c861f4ba64b70572dff1.

Reason for revert: Due to Gerrit outage  http://crbug.com/872722 , we are reverting this CL. Please, re-land it after all clear is given. If you have questions, please ask on the bug. Sorry for the inconvenience.

Original change's description:
> Deflake inspector-protocol/input/dispatchKeyEvent-focus.js
> 
> The keystrokes injected via Input.dispatchKeyEvent through the browser are
> racing against Runtime.evaluate in the renderer, so we need to wait for
> the async completion of the last dispatchKeyEvent before validating the
> tests results with evaluate.
> 
> Bug:  872081 
> Change-Id: Ic5ef41feba6a877c76c05de2e09e019a826756a1
> Reviewed-on: https://chromium-review.googlesource.com/1168200
> Reviewed-by: Andrey Lushnikov <lushnikov@chromium.org>

TBR=dgozman@chromium.org,lushnikov@chromium.org,caseq@chromium.org

Change-Id: I6f7e8e41eab2c756875103cae24576c37784baf3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  872081 
Reviewed-on: https://chromium-review.googlesource.com/1169789
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>

[modify] https://crrev.com/4af14ca3c99bd7edbf100e05db93564b7107cc62/third_party/WebKit/LayoutTests/inspector-protocol/input/dispatchKeyEvent-focus.js

I believe the following sequence of events is relevant to the failure:

1. A synthetic click is requested through chrome.gpuBenchmarking, which sends an IPC to the browser.
2. The 300ms blocking wait is started on the renderer's main thread.
3. The browser sends two separate IPCs to the renderer: mouse down, mouse up.
4. The renderer becomes aware of the first IPC's arrival and a task is scheduled to begin processing messages on the WidgetInputHandler interface (bound to WidgetInputHandlerImpl) on the main thread.
5. The renderer main thread finishes waiting and resolves its clickAndBlockMain promise. This schedules a task to run the promise continuation on the main thread.
6. The task posted by #4 runs, reading and dispatching the mouse-down message, and posting another task to dispatch the mouse-up message
7. The task posted from #5 runs, starting the observer, requesting another synthetic click, and then blocking for another 300ms.
8. The renderer finishes waiting and resolves its promise.
9. The first mouse-up dispatch task is finally run
10. The second mouse-down and mouse-up dispatch tasks are run.

The only relevant change in behavior caused by the Mojo CL is at #6. Before the CL, #6 will effectively always (like 99.999% of the time) dispatch both mouse-down and mouse-up messages within a single task execution, since by the time the renderer actually processes the posted dispatch task from #4, both messages will have arrived on the pipe. This is however not a guarantee made by Mojo (the second message could in fact take arbitrarily long to arrive) and so it is definitely incorrect for the system to rely on that assumption.

AFAICT then, the issue is that the first click isn't fully realized until after the observer is started.

+Navid

A simple hack which will make the test work would be to chain two wait() promises after the first clickAndBlockMain, rather than only the one that's chained now.

This still relies on the implementation detail that a click simulation happens in two IPCs, and that both IPCs will necessarily be received and dispatched within two scheduler round-trips. While almost always true in practice, this simply is not a guarantee made by any part of the system.

Is there a better way to write this test?

I notice that chrome.gpuBenchmarking.pointerActionSequence accepts a callback, probably to be called once inputs are received. So I think if I just wrap the click in a promise  then that'll enforce the order I want.

It kind of looks like that callback fires as soon as the browser acknowledges the synthetic input request, rather than after the synthesized event is fully processed by the renderer.

Is there any reason the test can't simply register an onclick listener on the test button and proceed with the observer stuff once that fires?

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

commit 8b90885e60134c34176df2dd1834a94dbd6b73b9
Author: Andrey Kosyakov <caseq@chromium.org>
Date: Thu Aug 09 23:33:36 2018

Reland "Deflake inspector-protocol/input/dispatchKeyEvent-focus.js"

This reverts commit 4af14ca3c99bd7edbf100e05db93564b7107cc62.

Reason for revert: relanding once the outage is over.

Original change's description:
> Revert "Deflake inspector-protocol/input/dispatchKeyEvent-focus.js"
> 
> This reverts commit 27378e0364b705643275c861f4ba64b70572dff1.
> 
> Reason for revert: Due to Gerrit outage  http://crbug.com/872722 , we are reverting this CL. Please, re-land it after all clear is given. If you have questions, please ask on the bug. Sorry for the inconvenience.
> 
> Original change's description:
> > Deflake inspector-protocol/input/dispatchKeyEvent-focus.js
> > 
> > The keystrokes injected via Input.dispatchKeyEvent through the browser are
> > racing against Runtime.evaluate in the renderer, so we need to wait for
> > the async completion of the last dispatchKeyEvent before validating the
> > tests results with evaluate.
> > 
> > Bug:  872081 
> > Change-Id: Ic5ef41feba6a877c76c05de2e09e019a826756a1
> > Reviewed-on: https://chromium-review.googlesource.com/1168200
> > Reviewed-by: Andrey Lushnikov <lushnikov@chromium.org>
> 
> TBR=dgozman@chromium.org,lushnikov@chromium.org,caseq@chromium.org
> 
> Change-Id: I6f7e8e41eab2c756875103cae24576c37784baf3
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug:  872081 
> Reviewed-on: https://chromium-review.googlesource.com/1169789
> Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>

TBR=dgozman@chromium.org,lushnikov@chromium.org,caseq@chromium.org,tandrii@chromium.org

Change-Id: Ia8e5d3e56d923c31451474969afad7450e8e734d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  872081 
Reviewed-on: https://chromium-review.googlesource.com/1170007
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Commit-Queue: Andrey Kosyakov <caseq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581956}
[modify] https://crrev.com/8b90885e60134c34176df2dd1834a94dbd6b73b9/third_party/WebKit/LayoutTests/inspector-protocol/input/dispatchKeyEvent-focus.js

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

commit 6edf709c841f879bdfee2f1b2d9aab913c08b992
Author: Andrey Kosyakov <caseq@chromium.org>
Date: Fri Aug 10 02:02:39 2018

Fix http/tests/inspector-protocol/network/multiple-headers.js to use same-origin request

We accidentally used a CORS fetch() to a resource that dit not allow CORS access in the test,
expecting Network.loadingFinished to happen after the resource is loaded. However, with
per-message dispatch tasks (https://chromium-review.googlesource.com/c/chromium/src/+/1145692),
the request is cancelled before we receive loadingFinished, so we now get loadingFailed
with net::ERR_ABORTED instead.

Bug:  872081 
Change-Id: I9cfd6c8ebc49f1e835916fb87f4d6cea0e4262b1
Reviewed-on: https://chromium-review.googlesource.com/1168185
Commit-Queue: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582014}
[delete] https://crrev.com/6b625f83dc9eeef06a86e7af2f5b08b4a4d5baee/third_party/WebKit/LayoutTests/flag-specific/site-per-process/http/tests/inspector-protocol/network/multiple-headers-expected.txt
[modify] https://crrev.com/6edf709c841f879bdfee2f1b2d9aab913c08b992/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/multiple-headers-expected.txt
[modify] https://crrev.com/6edf709c841f879bdfee2f1b2d9aab913c08b992/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/multiple-headers.js

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

commit 38786b89186766fc2f1045144b412a32b0a0f2e8
Author: Ken Rockot <rockot@chromium.org>
Date: Fri Aug 10 15:48:25 2018

Fix http/tests/event-timing/event-timing-onloadthenobserve.html

Changes this test to properly wait for the first synthetic click
event to fire before proceeding with the rest of the test and validating
its expectations.

Bug:  872081 
Change-Id: I0954ef7deff8415600effbc79cca7402a1b51583
Reviewed-on: https://chromium-review.googlesource.com/1170173
Commit-Queue: Ken Rockot <rockot@chromium.org>
Reviewed-by: Nicolás Peña Moreno <npm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582184}
[modify] https://crrev.com/38786b89186766fc2f1045144b412a32b0a0f2e8/third_party/WebKit/LayoutTests/http/tests/event-timing/event-timing-onloadthenobserve.html
[modify] https://crrev.com/38786b89186766fc2f1045144b412a32b0a0f2e8/third_party/WebKit/LayoutTests/http/tests/event-timing/resources/event-timing-support.js

All that remains here is the VR test. Any word on whether we can just delete that test now offenwanger@?

It's getting removed as part of https://chromium-review.googlesource.com/c/chromium/src/+/1159468, so I think it's fine to remove now.

Great, thanks! There are still quite a few other blockers, so deleting the
test isn't urgent. I think we can just wait for that CL to land.