Closed Bug 1175585 Opened 5 years ago Closed 4 years ago

Write full-stack mochitest for wheel transactions

Categories

(Core :: Panning and Zooming, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox41 --- affected
firefox43 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Bug 1175585 - Generalize scrollWheelOver() so it's usable by other APZ tests. r=kats
Attachment #8623959 - Flags: review?(bugmail.mozilla)
Bug 1175585 - Full-stack mochitest for wheel transactions. r=kats
Attachment #8623960 - Flags: review?(bugmail.mozilla)
Unlike test_layerization and like test_wheel_scroll, this test works with or without APZ, and I verified that it passes locally in both scenarios.
Will push to Try when Try is open.
I'll look at it tomorrow, but it would be great to make the test chaos-mode-enabled for the try push (and landing assuming it doesn't expose badness)
Attachment #8623959 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8623960 [details]
MozReview Request: Bug 1175585 - Full-stack mochitest for wheel transactions. r=kats

https://reviewboard.mozilla.org/r/11627/#review10089

::: gfx/layers/apz/test/test_wheel_transactions.html:27
(Diff revision 1)
> +      background: repeating-linear-gradient(#EEE, #EEE 100px, #DDD 100px, #DDD 200px);      

trailing whitespace (here and more below)

::: gfx/layers/apz/test/test_wheel_transactions.html:46
(Diff revision 1)
> +  // Scroll |outer| to the bottom. Note that this brings |inner|

It might be worth asserting this, by checking the target of the last wheel event. It should be #inner-content, I think. (That might change if we ever resolve bug 1168182)

::: gfx/layers/apz/test/test_wheel_transactions.html:51
(Diff revision 1)
> +  // Imemdiately after, scroll it back up a bit.

typo: "Immediately"

::: gfx/layers/apz/test/test_wheel_transactions.html:86
(Diff revision 1)
> +  // Wait for the transaction timeout to elaspe.

typo: elapse
Attachment #8623960 - Flags: review?(bugmail.mozilla) → review+
Out of curiosity what does this cover that test_wheeltransaction.xul doesn't?
(In reply to David Anderson [:dvander] from comment #9)
> Out of curiosity what does this cover that test_wheeltransaction.xul doesn't?

It synthesizes *native* events which pass through widget code and APZ (if enabled). So with APZ enabled, this tests APZ's implementation of wheel transactions, which is the point.
David raises a good point, actually. When getting the wheel tests passing with APZ enabled, we made some changes so that test_wheeltransaction.xul also exercises the APZ codepaths. In particular the line at [1] takes non-native wheel events and routes them back through the widget so they go through APZ. To be honest I wasn't really happy with that because it's inconsistent; I'd rather have sendWheelEvent just do a DOM dispatch and sendNativeWheelEvent do a widget-level dispatch (and the same for all the other event types). That would mean changing test_wheeltransaction.xul (and probably other tests) to use sendNativeWheelEvent instead of sendWheelEvent. That's probably a good idea regardless but we might have to do some other work to make that possible first.

Anyway, I think this test still adds value because the mechanism for synthesizing a native wheel event is different from what DOMWindowUtils does, and closer to what happens in the real world. It's probably not worth writing a full set of tests that duplicate test_wheeltransaction.xul using native wheel events though.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp?rev=536bd9910bc2#866
Attachment #8623960 - Flags: review+ → review?(bugmail.mozilla)
Comment on attachment 8623960 [details]
MozReview Request: Bug 1175585 - Full-stack mochitest for wheel transactions. r=kats

Bug 1175585 - Full-stack mochitest for wheel transactions. r=kats
Addressed review comments. Asking for review on my implementation of this check:

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> ::: gfx/layers/apz/test/test_wheel_transactions.html:46
> (Diff revision 1)
> > +  // Scroll |outer| to the bottom. Note that this brings |inner|
> 
> It might be worth asserting this, by checking the target of the last wheel
> event. It should be #inner-content, I think. (That might change if we ever
> resolve bug 1168182)
Comment on attachment 8623960 [details]
MozReview Request: Bug 1175585 - Full-stack mochitest for wheel transactions. r=kats

https://reviewboard.mozilla.org/r/11627/#review10205

Looks good!

::: gfx/layers/apz/test/test_wheel_transactions.html:46
(Diff revision 2)
> +  

trailing whitespace on lines 46 and 52
Attachment #8623960 - Flags: review?(bugmail.mozilla) → review+
The failing test is the one the patch adds, on Linux e10s. Retriggering the relevant job of the Try push from comment 6 (which was green) a few times shows 4/14 failures, so I guess I have an intermittent failure on my hands...
Flags: needinfo?(botond)
After much debugging, I found the cause of the intermittent failure.

The test relies on wheel events sent in succession without a delay between them being considered part of the same wheel transaction. That is, it relies on the wheel transaction timeout not firing between two such events (during the part of the test where the timeout is at its original value of 1500 ms).

This holds while the 1500 ms timeout value is respected. However, in chaos mode, timers sometimes randomly fire earlier than they're supposed to [1], causing wheel transactions to time out in places where the test doesn't expect them.

Kats, do you agree that this makes chaos mode inappropriate for this test? (I'm actually surprised that this timer behaviour in chaos mode doesn't mess things up much worse...)

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/TimerThread.cpp?rev=ce36885ea160#330
Flags: needinfo?(bugmail.mozilla)
If I'm reading that code correctly, it seems like it only fiddles with the microsecond values on timer intervals, which affects whether the task runs right away or sleeps for the minimum possible sleep amount first. It shouldn't cause timers to fire earlier than normal, certainly not by a millisecond order-of-magnitude, and certainly not by 1500ms. I suspect you'll see the same failure even with chaos mode disabled.

Fundamentally I think this a legitimate problem with the test because it assumes that the test will run fast enough to dispatch all the wheel events within a given timeframe, and random stalls in the test harness can cause that assumption to fail. We should either increase the transaction timeout for the first part of the test, or find some other way to compensate for this.

It might also be worth doing a try push with chaos mode disabled and a lot of retriggers to see if the issue still happens. It might be that I'm misreading the code and it's a bug in the chaos mode code. Either way it would be good to know what happens with chaos mode disabled.
Flags: needinfo?(bugmail.mozilla)
Oh wait, I did misread the code. Yeah that seems a little weird in that it can completely eat the requested delay. I do think there is still a problem with the test as I described above, but possible also a bug in the chaos mode code. If it works with chaos mode disabled, feel free to land that for now and we can talk to roc about the intention of that code.
Hmm.

During my local testing, the failure was definitely not caused by 1500 ms actually elapsing before the timeout - I instrumented the code, and only 50-100 ms would elapse between the timer being scheduled and the timeout.

However, the test is still intermittently failing on Try without chaos mode [1], indicating that there is some other issue. I will push again with more logging.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=adbf16b201c3
(In reply to Botond Ballo [:botond] from comment #21)
> However, the test is still intermittently failing on Try without chaos mode,
> indicating that there is some other issue.

After further investigation, it seems that the other issue is that sometimes, when synthesizing a mouse move event and waiting for it to arrive to Gecko, it never arrives. This can be seen in these Try pushes: [1] (synthesizing mouse moves), [2] (not synthesizing mouse moves).

My best guess for why this is happening, is that when multiple mouse move events are dispatched at the same location, then successive ones can sometimes be coalesced by the operating system. This can be worked around by only sending a mouse move event before a wheel event that actually is targeted at a new location than the previous one, rather than before every wheel event.

Note that the early wheel transaction timeouts due to chaos mode still is an issue, as demonstrated by the Try push [3] (no mouse move events but chaos mode enabled).

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=f683dc273e5a 
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=64623c084f64
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc7da2560823
No longer blocks: 1176402
Depends on: 1176402
Ok, that makes sense. Another option is to synthesize the native move, but not wait for the "mousemove" listener to be triggered - instead, just synthesize the wheel event right after without waiting. Since the wheel event will go in the queue after the move event the ordering is guaranteed, and even if the move event gets dropped the wheel event should still make it through.
Now that the issues in bug 1177018 have been sorted out, I cleaned up this test in preparation for relanding it.

Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=72bddf8dc8c1
The Try push in comment 24 had some failures. Here's an updated patch which passes:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0730ef1cabd3 (apz enabled)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=acb34c1da6a6 (apz disabled)

Posting updated patch for review. Note that this was already reviewed before (in comment 14). In the end the only change since that version is to remove the generation of mouse events altogether; they were needed in test_layerization to make sure that wheel events at different locations didn't get grouped into a transaction, but in this test all wheel events are generated at the same location.
Comment on attachment 8623959 [details]
MozReview Request: Bug 1175585 - Full-stack mochitest for wheel transactions. r=kats

Bug 1175585 - Full-stack mochitest for wheel transactions. r=kats
Attachment #8623959 - Attachment description: MozReview Request: Bug 1175585 - Generalize scrollWheelOver() so it's usable by other APZ tests. r=kats → MozReview Request: Bug 1175585 - Full-stack mochitest for wheel transactions. r=kats
Attachment #8623960 - Attachment is obsolete: true
Comment on attachment 8623959 [details]
MozReview Request: Bug 1175585 - Full-stack mochitest for wheel transactions. r=kats

Bug 1175585 - Full-stack mochitest for wheel transactions. r=kats
Comment on attachment 8623959 [details]
MozReview Request: Bug 1175585 - Full-stack mochitest for wheel transactions. r=kats

Updated to remove some commented-out lines I had meant to remove. Also MozReview carried the r+ when I didn't mean for it to.
Attachment #8623959 - Flags: review+ → review?(bugmail.mozilla)
https://reviewboard.mozilla.org/r/11623/#review13465

::: gfx/layers/apz/test/test_wheel_transactions.html:98
(Diff revision 4)
> +      yield scrollWheelOver(outer, -10);

indentation

::: gfx/layers/apz/test/test_wheel_transactions.html:99
(Diff revision 4)
> +  }

I'd feel better if each iteration of this loop checked that the scrollTop incremented. Otherwise the failure mode is that this goes into an infinite loop and burns CPU cycles until the test times out.

::: gfx/layers/apz/test/test_wheel_transactions.html:105
(Diff revision 4)
> +  while (outer.scrollTop < outer.scrollTopMax) {

Ditto here (both re the infinite loop and indentation)
Attachment #8623959 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8623959 [details]
MozReview Request: Bug 1175585 - Full-stack mochitest for wheel transactions. r=kats

Bug 1175585 - Full-stack mochitest for wheel transactions. r=kats
The patches I've been pushing to Try for this bug are on top of the patches for bug 1177018, so I'll wait for that to land before landing this.
Depends on: 1177018
Now that bug 1177018 has landed, this is ready to land too.
https://hg.mozilla.org/mozilla-central/rev/48608a2a19b1
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.