Open Bug 1776965 Opened 4 years ago Updated 3 days ago

Intermittent gfx/layers/apz/test/mochitest/test_group_touchevents-5.html | single tracking bug

Categories

(Core :: Panning and Zooming, defect, P3)

Product:
Core
Component:
Panning and Zooming
Type:
defect

Tracking

()

People

(Reporter: jmaher, Unassigned)

References

Details

(Keywords: intermittent-failure, intermittent-testcase, leave-open)

Attachments

(1 file, 2 obsolete files)

Bug 1776965 - Skip helper_touch_drag_root_scrollbar.html on linux. r=botond
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 1776965

Severity: normal → S3

The recent spike of failures seem to all be the new subtest added in bug 1724759.

Probably it's due to bug 1806400. I innocently used a touchstart event listener in the test. :/

This started failing more often after Bug 1724759 landed here. When I retriggered a job that failed on this test, all the retriggers were green as it can be seen here, that is why we didn't back out that revision.
Hi Hiroyuki! Should we disable the test until you can make a fix for this? or should we leave it like this because you plan to fix this soon?
Thank you!

Flags: needinfo?(hikezoe.birchill)

I am going to fix the new intermittent in bug 1873591.

Flags: needinfo?(hikezoe.birchill)
See Also: → 1873591

Failure rate dropped since https://bugzilla.mozilla.org/show_bug.cgi?id=1873591#c5.

Whiteboard: [stockwell disable-recommended]
No longer depends on: 1672736
Whiteboard: [stockwell disable-recommended]
Whiteboard: [stockwell disable-recommended]

(In reply to Intermittent Failures Robot from comment #95)

23 failures in 742 pushes (0.031 failures/push) were associated with this bug yesterday.
(snip)
For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1776965&startday=2024-03-25&endday=2024-03-25&tree=all

21 of 23 failures are helper_bug1669625.html, it started from https://treeherder.mozilla.org/jobs?repo=autoland&selectedJob=452111883&revision=19d905446a32ebc5b281e61c8ee49718ee784a25 which landed just before bug 1806400, so it's definitely unrelated to bug 1806400.

That's being said, there's one failure on helper_bug1806400-3.html on a TV run on Windows 11 unfortunately.

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

21 of 23 failures are helper_bug1669625.html

That test file was recently touched in https://phabricator.services.mozilla.com/D205427, though it looks to me like the change should be a no-op.

It looks to me that the helper_bug1669625.html failure has started from bug 1886371. I did re-trigger M-1 task on the revision.

CCing Jamie.

See Also: → 1886371

Oops, I CC-ed a different person. :/

Unfortunately it looks the failure is perma.

I'm guessing this should be resolved by the fix in bug 1887584.

Thanks Jamie. Indeed there's no new helper_bug1669625.html failure.

The subtest that's causing the intermittents here seems to be helper_touch_drag_root_scrollbar.html.

Flags: needinfo?(drobertson)
Flags: needinfo?(drobertson)
Keywords: leave-open

(In reply to Botond Ballo [:botond] from comment #111)

The subtest that's causing the intermittents here seems to be helper_touch_drag_root_scrollbar.html.

The failure rate has been fluctuating between 21-30 over the past 6 weeks, occasionally spiking above like this most recent week.

Dan, should we consider disabling the subtest on Linux, which is where almost all the failures are, until we investigate the underlying reason for the intermittent?

Flags: needinfo?(drobertson)

Skip helper_touch_drag_root_scrollbar.html in test_group_touchevents-5.html on
linux until the cause of the intermittent failure is fixed.

Thanks for the ping!

Flags: needinfo?(drobertson)
Pushed by drobertson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/df4f3ebe5c54 Skip helper_touch_drag_root_scrollbar.html on linux. r=botond

The recent spike of failures is all helper_bug1669625.html, something must have regressed it; would be interesting to figure out what.

Filed Bug 1963520 for this issue, which is caused by Bug 1957314.

See Also: → 1963520

The recent spike seems to be caused by the recent changes in Bug 2038491, as it can be seen here.
Hi Stephen! Could you please take a look?

Thank you!

Flags: needinfo?(spohl.mozilla.bugs)

helper_bug1669625.html verifies that scrolling alone does not cause extra display list building. That relies on no mouse being positioned over the content, so that scroll-driven PresShell::SynthesizeMouseMove calls are no-ops. Since bug 2038491, a fresh top-level PresShell can claim an orphaned static last-mouse position from a torn-down PresShell on the same widget and seed its own mLastMousePointerId. After that, every scroll-driven SynthesizeMouseMove dispatches a synthetic eMouseMove, which can land in the refresh-driver tick the subtest is observing and produce display list building it attributes to the scroll, causing intermittent failures.

Disable layout.reflow.synthMouseMove for this subtest so synthetic mouse moves are suppressed entirely. The subtest exercises scroll-induced painting, not synthetic mouse moves, so this removes the confound without weakening what it checks.

Thanks for the ping, Simona. I believe this spike is indeed caused by bug 2038491, which can now cause a scroll-driven synthetic mouse move to perturb the "helper_bug1669625.html" subtest, and I have put up a candidate fix in D303600 that disables synthetic mouse moves for that subtest. I have pushed two try runs to confirm this fixes the spike before requesting review:

a baseline without the fix: https://treeherder.mozilla.org/jobs?repo=try&landoInstance=lando-prod-2025&landoCommitID=52117
a push with the proposed fix: https://treeherder.mozilla.org/jobs?repo=try&landoInstance=lando-prod-2025&landoCommitID=52116

I will follow up here once the results are in. Keeping n-i set.

My wild guess is that dropping this button element fixes the failure?

Thanks, Hiroyuki. The synthetic mouse move introduced by bug 2038491 appears to be dispatched on scroll, and your suggestion that its hover restyle on the button is what produces the display list building would explain the failure. I have dispatched a third try run that drops the button element while leaving layout.reflow.synthMouseMove enabled, so we can tell whether removing the hover target alone is enough or whether dispatching the synthetic move triggers the rebuild on its own:

https://treeherder.mozilla.org/jobs?repo=try&landoInstance=lando-prod-2025&landoCommitID=52118

I will report back once all three try runs are in, and if your button-removal approach holds up it would be the more surgical fix, so I would lean toward it.

If dropping the button didn't fix the failure, we should back out bug 2038491 for now. Given that while scrolling is being driven by APZ, if there's unnecessary paint happens on the main-thread, it means it breaks what APZ does, right?

If the button-removal run does not come back green, I am on board with backing out bug 2038491 while we rethink the approach.

One nuance so we are reasoning about the same thing: a synthetic mouse move on scroll, and any hover restyle it causes, is pre-existing behavior whenever the pointer is actually over the scrolling content. PresShell::SynthesizeMouseMove(aFromScroll=true) has always re-dispatched at the last mouse position so that :hover and the cursor track content scrolling under a stationary pointer, and helper_bug1669625.html avoids that today by running with no mouse over the content. What bug 2038491 changed is that a freshly navigated PresShell now claims the orphaned last-mouse position and seeds mLastMousePointerId from it, so the per-scroll synthetic move fires even when no mouse is over this document. That is both what breaks the test's invariant and, as you note, a real per-scroll cost for any page right after a navigation.

That said, I think the most likely correct fix may be more targeted than either the test change or a permanent backout. Bug 2038491 only needs a single synthetic mouse move right after the navigation to bring the cursor and :hover up to date. It does not need to leave mLastMousePointerId set such that every subsequent scroll re-synthesizes. If we dispatch once after the navigation and do not re-arm the per-scroll synthesis, we should fix the original cursor bug without adding main-thread paints during APZ scrolling. I would like to confirm that against the try data before committing to it.

The honest blocker right now is that the try server has been rejecting pushes for the last several hours (abandoned transaction on hg.mozilla.org/try), so none of the three runs, including the button-removal one, have actually started yet. I have them queued to push automatically as soon as try recovers and will report back as soon as results are in.

Bug 2038491 lets a freshly navigated root PresShell recover the last-mouse position recorded before the navigation (on the same widget) so cursor and :hover restyles take effect without requiring a real mouse move first. The problem is that it then treats that recovered position exactly like a pointer that is currently and verifiably over the document: it seeds mLastMousePointerId persistently, so every subsequent scroll-driven PresShell::SynthesizeMouseMove(aFromScroll=true) also dispatches a synthetic eMouseMove. While APZ drives the scroll that forces main-thread display list building on every scroll for a position the document never actually received an event for, which intermittently fails helper_bug1669625.html (bug 1776965) and, more generally, adds unnecessary main-thread work during async scrolling for any page right after a navigation.

The underlying issue is that a recovered position is best-effort: it is inherited rather than confirmed by an event delivered to this document, and there is no live confirmation the pointer is still there. Such a position is appropriate for the discrete, low-frequency cursor/:hover update bug 2038491 needs (navigation- and restyle-driven synthesis, aFromScroll == false), but not for high-frequency, APZ-driven per-scroll synthesis. Track whether the current last-mouse position was recovered (emulated) rather than set by a real event over the document, and skip scroll-driven synthesis in that case. As soon as a real mouse event arrives the position is confirmed, the flag is cleared, and scrolling resumes updating :hover / cursor exactly as before.

Attachment #9591812 - Attachment is obsolete: true

With try back up I was able to get data on this, and I think we have a clearer picture now.

Confirming the failing subtest first: at 20 rebuilds, the baseline (current central) reproduces the failure as test_group_touchevents-5.html | helper_bug1669625.html | "Document element didn't get display list", with the failures concentrated on the Windows 11 25H2 ASan config Simona linked. So this does appear to be helper_bug1669625.html, as Hiroyuki suspected.

On the button idea: I tried dropping the button element, and unfortunately it does not appear to be sufficient on its own. With the button removed but synthetic mouse moves still enabled, the subtest still failed 4 times with the same signature. That suggests the display list building comes from dispatching the synthetic mouse move itself during the scroll's refresh tick, rather than from a :hover restyle on the button. As a cross-check, disabling layout.reflow.synthMouseMove entirely for the subtest was green, which is consistent with the synthetic move being the trigger.

Rather than disable synthetic mouse moves wholesale or back out bug 2038491, I went with a more targeted fix layered on top of bug 2038491. After a navigation we recover the last-mouse position from before the navigation, which is best-effort: it is inherited rather than confirmed by an event delivered to the new document. That recovered position is appropriate for the discrete cursor and :hover update bug 2038491 needs (navigation- and restyle-driven synthesis), but it should not keep driving a synthetic mouse move on every APZ-driven scroll, which appears to be where the unnecessary main-thread work comes from. The patch tracks whether the current last-mouse position is such a recovered position and skips only the scroll-driven synthesis in that case. As soon as a real mouse event arrives, the position is confirmed and scrolling resumes updating :hover and cursor as before.

Try runs:

The fix is a follow-up on top of bug 2038491 and should not require backing it out. It is up for review now as D303633.

Flags: needinfo?(spohl.mozilla.bugs)

Hey :spohl, you should open a new bug and attach the patch there.

And I am afraid I am not familiar with synthesize mouse move stuff, so I don't quite understand why a paint is triggered even though there's nothing :hover/:cursor styles? Can you clarify it? If the synthesize mouse move stuff forcibly trigger a paint, I think we should fix it rather than doing a band-aid to the special case where bug 2038491 introduced?

I'd expect the answer in the new bug.

Flags: needinfo?(spohl.mozilla.bugs)
Attachment #9591883 - Attachment description: Bug 1776965: Only treat a navigation-recovered last-mouse position as a real pointer for discrete cursor/:hover updates, not for per-scroll synthesis. r=masayuki,hiro → WIP: Bug 1776965: Only treat a navigation-recovered last-mouse position as a real pointer for discrete cursor/:hover updates, not for per-scroll synthesis. r=masayuki,hiro
See Also: → 2043935

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

Hey :spohl, you should open a new bug and attach the patch there.

I was going back-and-forth on this and ultimately decided to post it here, since the try runs showed no further failures. But it is true that this intermittent existed before the changes in bug 2038491, so it is reasonable to assume that it would return to the frequency of intermittents prior to bug 2038491 and whatever followup patch we land. I have opened bug 2043935 for further discussion.

Flags: needinfo?(spohl.mozilla.bugs)

Comment on attachment 9591883 [details]
WIP: Bug 1776965: Only treat a navigation-recovered last-mouse position as a real pointer for discrete cursor/:hover updates, not for per-scroll synthesis. r=masayuki,hiro

Revision D303633 was moved to bug 2043935. Setting attachment 9591883 [details] to obsolete.

Attachment #9591883 - Attachment is obsolete: true
Depends on: 2043935
Whiteboard: [stockwell disable-recommended]
Whiteboard: [stockwell disable-recommended]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: