Closed Bug 1137952 Opened 9 years ago Closed 9 years ago

Intermittent test_bug607464.html | Pixel scrolling should have finished after one refresh driver iteration. We shouldn't be scrolling smoothly, even though the pref is set. - got 0, expected 15

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Version:
39 Branch
Platform:
x86_64
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla39
Tracking Flags:
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed
firefox-esr31 --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: KWierso, Assigned: kats)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

Patch
1.30 KB, patch
mstange
: review+
Details | Diff | Splinter Review 
12:46:23 INFO - 244 INFO TEST-START | dom/events/test/test_bug605242.html
12:46:23 INFO - 245 INFO TEST-OK | dom/events/test/test_bug605242.html | took 46ms
12:46:23 INFO - 246 INFO TEST-START | dom/events/test/test_bug607464.html
12:46:23 INFO - 247 INFO must wait for load
12:46:23 INFO - 248 INFO must wait for focus
12:46:23 INFO - 249 INFO TEST-UNEXPECTED-FAIL | dom/events/test/test_bug607464.html | Pixel scrolling should have finished after one refresh driver iteration. We shouldn't be scrolling smoothly, even though the pref is set. - got 0, expected 15
12:46:23 INFO - 250 INFO MEMORY STAT vsize after test: 3423588352
12:46:23 INFO - 251 INFO MEMORY STAT residentFast after test: 390557696
12:46:23 INFO - 252 INFO MEMORY STAT heapAllocated after test: 151905120
12:46:23 INFO - 253 INFO TEST-OK | dom/events/test/test_bug607464.html | took 398ms
12:46:23 INFO - 254 INFO TEST-START | dom/events/test/test_bug613634.html
Any ideas what might be going on here?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
Presumably someone broke something?  This test hasn't changed since bug 1056851, which landed a week before the failures started, right?
Flags: needinfo?(bzbarsky)
I strongly suspect bug 1137203 given timing of when it hit the various branches (which of course we wouldn't have caught on b2g-inbound since 10.8 tests don't run there).

BTW, why are we landing GFX patches on b-i?!?!?
Flags: needinfo?(bugs) → needinfo?(bugmail.mozilla)
Bug 1137203 backed out - we'll find out soon enough.
I can reproduce this locally, and as far as I can tell it seems to be a pre-existing race condition in the code. It may very well have been exposed by my patches on bug 1137203, or by something else, who knows. The test is for bug 607464, which tested that wheel scroll events of type PIXEL don't trigger smooth scrolling. However, when I instrument the code I see that now these wheel scroll events are in fact triggering SMOOTH_MSD scrolling. Sometimes the scrolling finishes before the test checks for the scroll position, and sometimes it doesn't, resulting in the intermittent failure.

Either the code is correct and we are supposed to use SMOOTH_MSD for PIXEL wheel events, in which case the test is wrong. Or the code is wrong and we should be using an INSTANT scroll in this case. Kip, Markus - do you know which of the two is applicable here?
Flags: needinfo?(mstange)
Flags: needinfo?(kgilbert)
Flags: needinfo?(bugmail.mozilla)
Sorry, I was wrong - I misread some of the output. It's not doing SMOOTH_MSD scrolling after all, it's doing NORMAL scrolling which uses an mAsyncScroll thingy. Maybe it's just the async-ness of the scrolling that isn't properly detected by the test.
Flags: needinfo?(kgilbert)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #69)
> Maybe it's just the async-ness of the scrolling that isn't properly
> detected by the test.

This is very much possible. Somebody needs to find out what kinds of async-ness scrolling uses and how the test can wait for it reliably.
I think these things all used to go through the refresh driver - the test was apparently fairly stable for a few years. So it looks like one step in the async-ness chain has changed to something else.
Flags: needinfo?(mstange)
Attached patch Patch — — Splinter Review 
Doh, this looks like a typo. This patch seems to fix it for me locally, and I did a try push for 10.8 to verify. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ca7df4e9aae
Assignee: nobody → bugmail.mozilla
Attachment #8571487 - Flags: review?(mstange)
Comment on attachment 8571487 [details] [diff] [review]
Patch

This again! Argh!

Nice job catching it. I didn't realize this test was opening new windows, or I would have suggested for looking something like it, because I've wasted a whole day on a very similar problem before (see the last paragraph of bug 1112339 comment 0).
Attachment #8571487 - Flags: review?(mstange) → review+
Try build is still pending and I don't know if it'll ever get scheduled, so I pushed to inbound anyway. Even if it doesn't fix the problem it's still a correct change.

https://hg.mozilla.org/integration/mozilla-inbound/rev/98f395293242
The try push from comment 71 is green, which indicates that this was the root cause of the M-2 failure that got bug 1137203 backed out. I've relanded those now, but this patch will need uplifting to b2g34 and b2g37 so that bug 1137203 can also get uplifted. I intend to do that with a=testonly once this merges to m-c.
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-master: --- → affected
https://hg.mozilla.org/mozilla-central/rev/98f395293242
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
I was going to land this on beta and let it merge, but oh well.
status-firefox37: --- → affected
status-firefox38: --- → affected
status-firefox-esr31: --- → fixed
Assignee: bugmail.mozilla → cpearce
Assignee: cpearce → bugmail.mozilla
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: