Closed Bug 1258896 Opened 8 years ago Closed 8 years ago

Intermittent test_mousecapture.xhtml | selection scroll position after timer - got 180, expected 140

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- unaffected
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: KWierso, Assigned: kats)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Looks like this has been failing intermittently since it landed.
Blocks: 1254091
Component: DOM → Event Handling
Flags: needinfo?(gijskruitbosch+bugs)
Summary: Intermittent ASAN test_mousecapture.xhtml | selection scroll position after timer - got 180, expected 140 → Intermittent test_mousecapture.xhtml | selection scroll position after timer - got 180, expected 140
Version: 47 Branch → unspecified
(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> Looks like this has been failing intermittently since it landed.

And was failing intermittently before - it's a straight conversion from the original xul test, for which see bug 1235139.

I'll see if I can make time to have a look anyway, but it isn't new, particularly...
See Also: → 1235139
There seems to be some issue with the scroll events here. We wait for 6 scroll events, and then check that the scrollY of the window we're scrolling is set to what we expect (ie (6 + 1) * 20 = 140). Sometimes it's higher. I haven't seen it ever being lower. I tried moving the event listener addition a bit earlier in case we were racing with the events and are just missing some, but that didn't help, so I suspected involvement of apz.

According to :kats, making scroll events line up with vsync and potentially coalescing was bug 1209970, which roughly lines up with when bug 1235139 got filed, especially considering that was around Christmas time, so being out by 3-4 days doesn't seem like a big deal. So I suspect (and it's not much more than that at the moment) that that stuff is involved.

This seems to happen more often on e10s, but it does and did happen on non-e10s as well. I can reproduce fairly easily on my mac with:

./mach mochitest toolkit/content/tests/mochitest/test_mousecapture.xhtml --run-until-failure

where it usually fails within about 5 cycles (of the 30 which is the default for run-until-failure), but ymmv.

With permission from :kats, punting the needinfo over to them in the hope they can figure out what's going on.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bugmail.mozilla)
(In reply to :Gijs Kruitbosch from comment #11)
> According to :kats, making scroll events line up with vsync and potentially
> coalescing was bug 1209970, which roughly lines up with when bug 1235139 got

I was able to reproduce locally on Linux with logging and confirmed that this is the problem. In the case I saw, two scrolls got coalesced into a single scroll event dispatch and so the test waited for an extra scroll event than it should have. I'll see if I can modify the test to fix this while maintaining the intent of the test.
Assignee: nobody → bugmail.mozilla
Flags: needinfo?(bugmail.mozilla)
So it looks like the selection autoscroll runs every 30ms [1] - sometimes we get two of those within a single vsync and so this problem happens. If that timer delay were a pref I'd just make it large enough during the test that the probability of this happening drops to zero. However I don't want to introduce a new pref just for this.

Another option is stop listening for scroll events in the test, and use another 30ms setTimeout so that (in theory) they get interleaved perfectly. However that seems like a bad solution for various reasons.

I think the best thing to do here is to just make the test a little fuzzier and allow a range of 20-pixel increments as valid, rather than expecting exactly 140 px to have scrolled.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp?rev=d2b9a2c891fc#3626
Attachment #8749258 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8749258 [details]
MozReview Request: Bug 1258896 - Make the scroll position check in test_mousecapture a little fuzzier to allow for scroll event coalescing. r?Gijs

https://reviewboard.mozilla.org/r/50823/#review47515

rs=me

Thanks for diving into this and figuring out exactly why it was broken!
https://hg.mozilla.org/mozilla-central/rev/298128fa79bf
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.