1852243 - Enable "test.events.async.enabled" for all web-platform-tests

Status: RESOLVED WONTFIX

(Testing :: web-platform-tests, task, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The work to get widget events dispatched with Marionette and Remote Agent is not trivial. All the necessary work will be tracked on the meta bug 1773393.

As discovered after some experiments and discussions we probably can have a short term solution for the interop 2023 requirements by not only setting the test.events.async.enabled preference to true for the specific tests but for all the web-platform tests. Right now the related tests that require this preference to be set only pass in our own CI but fail upstream due to the missing meta data settings.

To actually get them running and passing upstream the preference can be added to the default profile of Firefox for wpt tests, which is used in both places.

I've pushed an initial try build with such a change and it shows a couple of issues:

1. Most of the keyboard related tests in Wdspec are failing because dispatching the event in the parent process let Firefox itself handle some keypresses and that inappropriately opens eg. a sidebar, the library etc.

2. Some Wdspec tests still fail because we see crashes due to the IPC routing of the event to the parent process. Here we definitely need bug 1849972 fixed first, and we have to check that the preference to disable all security is getting set as well.

3. A lot of wpt tests fail or unexpectedly pass which need further investigation once 1) has been solved.

To actually get 1) solved we can add more preferences under test.events.async.enabled to individually disable certain events from being dispatched to the parent process. Those preference could look like test.events.async.key.enabled.

Hopefully this will work out all fine.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Actually we do not necessarily need bug 1853120 immediately when we force off event specific async preferences. I should soon have some patches will will allow some more wpt tests as the known ones to pass, and keeps async events off for wdspec.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

With the following patch we enable async events for all wpt but disable for wdspec:
https://treeherder.mozilla.org/jobs?repo=try&revision=ff8128ab46cf18c269ce9316b6cc7778b4e38e2c

Sadly as you can see we still have quite a lot of failures because all the events are dispatched at the widget level. We could potentially disable async events for key and mouse because those shouldn't be needed for the interop 2023 related tests? If I'm wrong please let me know.

Also enabling touch async events causes problems with Marionette but it looks like we need that to get the interop 2023 related wpt tests working because they use touch and not wheel for the scrolling checks.

Here two more try builds:

• https://treeherder.mozilla.org/jobs?repo=try&revision=eb63d1a2b23b1c65e4bf18264ca36d317de13971 (only touch and wheel as async)
• https://treeherder.mozilla.org/jobs?repo=try&revision=dd7a8e0141cb4ffe26c9f58e7fd2a2207e9a01a5 (only wheel as async)

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1852243 - [dom] Add specific preferences for all available async event types.

The preference "test.events.async.enabled" controls the usage of
widget events in general, but Marionette doesn't support all of
them yet. To get widget events enabled for web-platform tests
those unsupported event types need to still synthesize the event.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1852243 - [wpt] Enable async touch and wheel events for wpt but not wdspec tests.

Depends on D188296

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I've uploaded WIP patches which enable all the async events for wpt, but not wdspec. That is causing quite a lot of failures as mentioned above. In case you want to test more focused for wheel only please add the relevant new preferences to the user.js of the wpt default profile.

Having only wheel async events enabled (see last try build) seems to be pretty stable with just 2 intermittent failures. I've triggered more wpt jobs on other platforms as well now. Lets see how it will look there.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #2)

With the following patch we enable async events for all wpt but disable for wdspec:
https://treeherder.mozilla.org/jobs?repo=try&revision=ff8128ab46cf18c269ce9316b6cc7778b4e38e2c

Sadly as you can see we still have quite a lot of failures because all the events are dispatched at the widget level. We could potentially disable async events for key and mouse because those shouldn't be needed for the interop 2023 related tests? If I'm wrong please let me know.

That's right.

Also enabling touch async events causes problems with Marionette but it looks like we need that to get the interop 2023 related wpt tests working because they use touch and not wheel for the scrolling checks.

Here two more try builds:

• https://treeherder.mozilla.org/jobs?repo=try&revision=eb63d1a2b23b1c65e4bf18264ca36d317de13971 (only touch and wheel as async)
• https://treeherder.mozilla.org/jobs?repo=try&revision=dd7a8e0141cb4ffe26c9f58e7fd2a2207e9a01a5 (only wheel as async)

This only wheel result looks promising! (Note that as Botond mentioned in bug 1773393 comment 36 the failure on overscroll-behavior.html might be a bug in Gecko, so we can handle it separately).

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)

(Note that as Botond mentioned in bug 1773393 comment 36 the failure on overscroll-behavior.html might be a bug in Gecko, so we can handle it separately).

I debugged further why overscroll-behavior.html fails with test.events.async.enabled:true and test.events.async.mouse.enabled:false.

The reason is that with test.events.async.mouse.enabled:false any mouse events are not handled by APZ, thus we use the same scroll container (APZC) as a target of any wheel events. I suppose that's the reason why the test originally uses a mouse click event.

Setting mousewheel.transaction.timeout=0 explicitly fixes the failure, but I am not sure whether it causes any side effects on other tests.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)

The reason is that with test.events.async.mouse.enabled:false any mouse events are not handled by APZ, thus we use the same scroll container (APZC) as a target of any wheel events. I suppose that's the reason why the test originally uses a mouse click event.

Setting mousewheel.transaction.timeout=0 explicitly fixes the failure, but I am not sure whether it causes any side effects on other tests.

This indeed fixes the test for me. Thanks!

I've compared the results between wheel only and wheel/touch aysnc events. There is actually not such a big difference and as such it might be good to get touch async events enabled as well. As such I've run tests locally and updated the expectations. Hereby I noticed that running various touch related tests in sequence have still a timeout issue while just passing when running on their own. It turns out that the context menu that gets opened by one of the touch tests actually are not getting closed at the end of the test, and cause following tests to timeout.

I pushed two try builds for all platforms:

• https://treeherder.mozilla.org/jobs?repo=try&revision=b7a0a3ba7b5840df75674c796b742d5fd21c9c62 (async wheel and touch events)
• https://treeherder.mozilla.org/jobs?repo=try&revision=26fcd32f6ea5c4f103aa1e6daf882c962ff21d7a (same with transaction timeout set to 0)

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

There doesn't seem to be a big difference with the transaction timeout set to 0. As such I would suggest that try to use it.

Nevertheless nearly each failing job shows a YOU ARE LEAKING THE WORLD warning, which we probably need to investigate and fix? Hereby I would need some help.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The leaks that can be seen look similar to what Dan saw over on bug 1816473. Maybe it's related and it will be fixed once the patch re-landed. For now I'm going to add it as dependency.

Comment 11

[Dan Robertson (:dlrobertson)]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #10)

The leaks that can be seen look similar to what Dan saw over on bug 1816473. Maybe it's related and it will be fixed once the patch re-landed. For now I'm going to add it as dependency.

Sounds good! The leak does look suspiciously similar.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

With the leak fixed I pushed a new try build with quite a few meta data updates for pointerevent tests:
https://treeherder.mozilla.org/jobs?repo=try&revision=4167cf25fcfcc8ab50338b455a117425e0d6612a

We still have some unexpected pass / fail left to fix. Maybe the next try will be more green. But please note that for some the status went from FAIL to NOTRUN or TIMEOUT but I hope that this would be fine and actually uncovers a problem with the tests themselves?

Also it looks like we have a case were we still leak the world. Hiro or Dan could one of you maybe have a look?

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Another permanent reproducible leak with the attached patches is at:
https://treeherder.mozilla.org/logviewer?job_id=430369579&repo=try&lineNumber=10225

Comment 14

[Hiroyuki Ikezoe (:hiro)]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #12)

With the leak fixed I pushed a new try build with quite a few meta data updates for pointerevent tests:
https://treeherder.mozilla.org/jobs?repo=try&revision=4167cf25fcfcc8ab50338b455a117425e0d6612a

We still have some unexpected pass / fail left to fix. Maybe the next try will be more green. But please note that for some the status went from
FAIL to NOTRUN or TIMEOUT but I hope that this would be fine and actually uncovers a problem with the tests themselves?

Yeah I think so.

Also it looks like we have a case were we still leak the world. Hiro or Dan could one of you maybe have a look?

The leak seems to happen on tests in browsing-the-web/. There are already a bunch of leaks so we can just amend it to align with the additional leaks?

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #13)

Another permanent reproducible leak with the attached patches is at:
https://treeherder.mozilla.org/logviewer?job_id=430369579&repo=try&lineNumber=10225

Pointerevents tests also look very leaky to me.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Some good news here first, with the latest pushes to try for debug builds I haven't seen leaks. So they are at least not happening all the time now which is great. Given the feedback from Hiro I've now updated all the metadata to reflect the current state of touch and scroll related tests when run with async events. I just pushed a new try build that hopefully might be the last one before I can update the revisions on Phabricator and can ask for review.

https://treeherder.mozilla.org/jobs?repo=try&revision=7f72b823ea1d0f00bdd4edd7b32fd693c7784831

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Also note that before I commented comment 14 I actually run wpt in browsing-the-web/ locally on my Linux box with the Henrik's changes a couple of times, at that time I saw a bunch of leak objects there, but I didn't see the same atom leaks in comment 12. So,

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #15)

Some good news here first, with the latest pushes to try for debug builds I haven't seen leaks. So they are at least not happening all the time now which is great.

This sounds plausible.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1852243 - [wpt] Enable async touch and wheel events for wpt but not wdspec tests.

Depends on D188296

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c818afe5e31e
[dom] Add specific preferences for all available async event types. r=hiro
https://hg.mozilla.org/integration/autoland/rev/b10876321a92
[wpt] Enable async touch and wheel events for wpt but not wdspec tests. r=jgraham,hiro

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Failed to create upstream wpt PR due to merge conflicts. This requires fixup from a wpt sync admin.

Comment 20

[Cristina Horotan [:chorotan]]

Backed out 2 changesets (bug 1852243) for causing multiple wpt failures

Backout: https://hg.mozilla.org/integration/autoland/rev/6775478c0aeaa458b618501a06615e4a7d5e6399

Failure push:
https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=b10876321a92f99061494df52d2d9269fb022d38

A few failure logs:
https://treeherder.mozilla.org/logviewer?job_id=431487938&repo=autoland&lineNumber=11626
https://treeherder.mozilla.org/logviewer?job_id=431487101&repo=autoland&lineNumber=2414
https://treeherder.mozilla.org/logviewer?job_id=431484156&repo=autoland&lineNumber=7656
https://treeherder.mozilla.org/logviewer?job_id=431484116&repo=autoland&lineNumber=3077
https://treeherder.mozilla.org/logviewer?job_id=431484182&repo=autoland&lineNumber=7210

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

With the latest update of the patch I actually made a mistake and did NOT disable async events for key and especially mouse events. The latter actually cause all the bustage.

Here a new try build with the fix:
https://treeherder.mozilla.org/jobs?repo=try&revision=6a5b0ef3b532eb9d96e53c2920a3f566afab2ca4

If it is all fine I'll land it tomorrow morning.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5c1b9223818
[dom] Add specific preferences for all available async event types. r=hiro
https://hg.mozilla.org/integration/autoland/rev/6ffbaf3b4ea8
[wpt] Enable async touch and wheel events for wpt but not wdspec tests. r=jgraham,hiro

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As discussed we will bump this bug up to 5 points.

Comment 24

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/42388 for changes under testing/web-platform/tests

Comment 25

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/e5c1b9223818
https://hg.mozilla.org/mozilla-central/rev/6ffbaf3b4ea8

Comment 26

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 27

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1852243 - [wpt] Fix infrastructure test multiTouchPointsTwoTouchStarts.html.

This test fails with async touch events enabled upstream and
needs to be fixed by temporarily using a larger delta for the
pointermove action until bug 1857912 is fixed.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a063766cd8ad
[wpt] Fix infrastructure test multiTouchPointsTwoTouchStarts.html. r=jgraham

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hi Hiro, Botond, and Dan. With the enabling of async touch and wheel events we still have quite a lot of intermittent failing tests that temporarily got marked in the metadata, or already filed a new bug for (see regressions list). Nevertheless the list of those metadata changes is huge and not something I potentially should dig through myself. Is there someone from the DOM team who can check why we still have timeouts or failures for certain tests and why these are that flaky? If we should have a meeting please let me know and we can schedule one. Thanks a lot.

Please note that you don't see the correct results on wpt.fyi yet. The related upstream PR is still not merged and needs the patch as just pushed to autoland.

Comment 31

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by jgraham

Comment 32

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/a063766cd8ad

Comment 33

[Hiroyuki Ikezoe (:hiro)]

Are the intermittent failures fixed by disabling async touch events? If so, we should disable it first.

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #33)

Are the intermittent failures fixed by disabling async touch events? If so, we should disable it first.

This is all hard to tell given that I tried to update the metadata status for all the affected tests to have lesser entries as well. So it might be that or the async touch. But we could try to disable async touch if that is fine by adding the pref test.events.async.touch.enabled=false to all of the affected tests. Note that we cannot simply turn of async touch events globally for wpt because it's now baked into wptrunner.

Comment 35

[Hiroyuki Ikezoe (:hiro)]

Yeah, that's fine for now. Given that we just needed native wheel events for bug 1689546 (it's one of interop-2023 tests).

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If you need to disable the async event dispatching one (or some) of them, you should disable all of them because synthesizing different kind of events may cause DOM events fired with unexpected order. That may make the developers waste their time when they hit the issue within writing a new test.

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: backout patch for 6ffbaf3b4ea8

After discussion with Masayuki we agreed on to backout this change due to causing problems in event handling for a lot of tests.

Revisions that need to backed out:
https://hg.mozilla.org/mozilla-central/rev/a063766cd8ad
https://hg.mozilla.org/mozilla-central/rev/6ffbaf3b4ea8
https://hg.mozilla.org/mozilla-central/rev/e5c1b9223818

Please note that for 6ffbaf3b4ea8 there is a merge conflict. As such I've added a backout patch as attachment to this bug.

Sheriffs can you please backout? Thanks!

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eadebae5a9c9
Backed out changeset a063766cd8ad causing problems in event handling for a lot of tests.
https://hg.mozilla.org/integration/autoland/rev/5a54a47faf1e
Backed out changeset e5c1b9223818 causing problems in event handling for a lot of tests.
https://hg.mozilla.org/integration/autoland/rev/0d4d9e3562d1
Revert changeset 6ffbaf3b4ea8 - [wpt] Enable async touch and wheel events for wpt but not wdspec tests. r=jgraham,hiro CLOSED TREE

Comment 39

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/42478 for changes under testing/web-platform/tests

Comment 40

[Sandor Molnar[:smolnar]]

Backed out the requested changesets

Backout link

Comment 41

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/0d4d9e3562d1

Comment 42

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 43

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As it turned out we cannot incrementally enable async events step by step but have to do it for all the event types at once. That means that we need a proper implementation in Marionette / Remote Agent, which actually uses the widget event dispatching in the parent process. This work will be done as part of bug 1851812 and bug 1866500.