Closed Bug 1785615 Opened 3 years ago Closed 2 years ago

Intermittent /html/semantics/forms/textfieldselection/select-event.html | single tracking bug

Categories

(Core :: DOM: Core & HTML, defect, P3)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
111 Branch
Tracking Flags:
Tracking Status
firefox111 --- fixed

People

(Reporter: jmaher, Assigned: jjaschke)

References

Details

(Keywords: intermittent-failure, intermittent-testcase, Whiteboard: [stockwell unknown], [wptsync upstream])

Attachments

(1 file)

Bug 1785615: Fixed flaky wpt by changing how to wait for events to be dispatched. r=masayuki
48 bytes, text/x-phabricator-request
Details | Review
No description provided.

Additional information about this bug failures and frequency patterns can be found by running: ./mach test-info failure-report --bug 1785615

Hello Jan, this has been a frequent failure for weeks. Could you please spend time investigating this? Thank you.

Flags: needinfo?(jjaschke)

I could resolve the issues by giving the tests a bit more time, i.e. adding a(nother) await waitForRender(); before the assert statement. However, I'm not sure if this is the actual solution to the problem or if I'm just fighting symptoms here and there's actually an issue in how events are dispatched.

:masayuki, what's your opinion on this?

Flags: needinfo?(jjaschke) → needinfo?(masayuki)

waitForRender waits 2 animation frames, so it should flush any asynchronous dispatching, I think. However, if dispatching select event requires 2 animation frames, another waitForRender call could solve this, however, it may cause web-compat issues in real web apps.

One animation frame must be required for dispatch select event, but what's for the other? For notifying selection listener? Or setting Selection in the editor?

jjaschke: Do you try to investigate it? (The editor module must work synchronously, so, Selection or nsFrameSelection may use asynchronous handling before adding select event dispatcher into the queue.)

Flags: needinfo?(masayuki) → needinfo?(jjaschke)

Currently, AsyncEventDispatcher::PostDOMEvent() schedules the event dispatching with current thread event queue if it runs in the main thread.

Therefore, asynchronous events are not guaranteed to be dispatched in a couple of animation frames because the event queue may be busy. However, a lot of tests use await new Promise(resolve => requestAnimationFrame(() => requestAnimiationFrame(resolve)); to wait flushing something. This should be true for tests of web apps.

On the other hand, Chrome schedules that in next animation frame:

Therefore, my opinion is, we should change AsyncEventDispatcher::PostDOMEvent use nsRefreshDriver::AddEarlyRunner (like IMEContentObserver) if it's available.

Of course, it's risky change, but I believe that it's the best ideal approach. So, we could have the following options:

  1. Add optional argument to PostEvent which callers allow to use refresh driver
  2. Make PostEvent always use refresh driver if available

smaug: What do you think?

Flags: needinfo?(jjaschke) → needinfo?(smaug)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away ~1/4) from comment #30)

Therefore, asynchronous events are not guaranteed to be dispatched in a couple of animation frames because the event queue may be busy. However, a lot of tests use await new Promise(resolve => requestAnimationFrame(() => requestAnimiationFrame(resolve)); to wait flushing something. This should be true for tests of web apps.

The spec does not require such thing, maybe there should be a stabler helper function in WPT?

AsyncEventDispatcher::PostDOMEvent in general shouldn't use refreshdriver. PostDOMEvent is used when the specs say something like "queue a task to fire event".

The spec says "If the previous steps caused the selection of the text control to be modified (in either extent or direction), then queue an element task on the user interaction task source given the element to fire an event named select at the element, with the bubbles attribute initialized to true."

Isn't this a bug in the test?

Flags: needinfo?(smaug)

Thank you, then, I think that this kind of tests need to use setTimeout(..., 0) instead of requestAnimationFrame even though eslint does not allow to do it in the basic rules.

jjaschke: Could you take a look? If you use setTimeout, you need to touch https://searchfox.org/mozilla-central/source/testing/web-platform/tests/lint.ignore to make it allow to use setTimeout in the test for allowing it to wait running a macro task. I guess that it works in this case.

(In reply to Kagami [:saschanaz] from comment #32)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away ~1/4) from comment #30)

Therefore, asynchronous events are not guaranteed to be dispatched in a couple of animation frames because the event queue may be busy. However, a lot of tests use await new Promise(resolve => requestAnimationFrame(() => requestAnimiationFrame(resolve)); to wait flushing something. This should be true for tests of web apps.

The spec does not require such thing, maybe there should be a stabler helper function in WPT?

Yeah, I hope so...

(In reply to Olli Pettay [:smaug][bugs@pettay.fi] from comment #33)

AsyncEventDispatcher::PostDOMEvent in general shouldn't use refreshdriver. PostDOMEvent is used when the specs say something like "queue a task to fire event".

Isn't this a bug in the test?

Basically, yes, but I'm afraid about that this incompatibility makes web developers give up to test their product with Firefox in their CI.

Replacing the waitForRender() function with this seems to fix the issue. Is this what you meant?

function waitForRender() {
  return new Promise(resolve => setTimeout(resolve, 0));
}

FWIW, Git Blame tells me that this test used to use setTimeout(..., 200) earlier on and was changed to rAF to speed it up (Bug 1771177).

(In reply to Jan Jaeschke [:jjaschke] from comment #35)

Replacing the waitForRender() function with this seems to fix the issue. Is this what you meant?

Yes, it is.

FWIW, Git Blame tells me that this test used to use setTimeout(..., 200) earlier on and was changed to rAF to speed it up (Bug 1771177).

Yeah, waiting 0.2 sec seems too slow. If setTimout(..., 0) makes the test run too long, we need to split the test.

Dispatching of asynchronous events depend on how busy the event queue is,
therefore it cannot be guaranteed that events have finished in two animation frames.
Changing the waitForRender() function to use setTimeout(...,0) fixes this.

Assignee: nobody → jjaschke
Status: NEW → ASSIGNED
Pushed by jjaschke@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/10fdecbd330a Fixed flaky wpt by changing how to wait for events to be dispatched. r=masayuki
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/38015 for changes under testing/web-platform/tests
Whiteboard: [stockwell unknown] → [stockwell unknown], [wptsync upstream]

https://hg.mozilla.org/mozilla-central/rev/10fdecbd330a

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox111: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 111 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: