1175585 - Write full-stack mochitest for wheel transactions

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Comment 1

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1175585 - Full-stack mochitest for wheel transactions. r=kats

Bug 1175585 - Generalize scrollWheelOver() so it's usable by other APZ tests. r=kats

Comment 2

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1175585 - Full-stack mochitest for wheel transactions. r=kats

Bug 1175585 - Full-stack mochitest for wheel transactions. r=kats

Comment 3

[Botond Ballo [:botond]]

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.

Comment 4

[Botond Ballo [:botond]]

Will push to Try when Try is open.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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)

Comment 6

[Botond Ballo [:botond]]

Enabled chaos mode and pushed to Try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=714a9aae5599

Comment 7

[Kartikaya Gupta (email:[e-mail])]

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

https://reviewboard.mozilla.org/r/11625/#review10087

Comment 8

[Kartikaya Gupta (email:[e-mail])]

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

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Out of curiosity what does this cover that test_wheeltransaction.xul doesn't?

Comment 10

[Botond Ballo [:botond]]

(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.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

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

Comment 12

[Botond Ballo [:botond]]

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

Comment 13

[Botond Ballo [:botond]]

Addressed review comments. Asking for review on my implementation of this check:

(In reply to Kartikaya Gupta (email:[e-mail]) 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 14

[Kartikaya Gupta (email:[e-mail])]

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

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89b4c0b06ff3
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b56c3d0e379

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

These made some apz tests permafail, so backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2f952a21d83c


https://treeherder.mozilla.org/logviewer.html#?job_id=10977193&repo=mozilla-inbound

Comment 17

[Botond Ballo [:botond]]

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...

Comment 18

[Botond Ballo [: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

Comment 19

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 21

[Botond Ballo [:botond]]

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

Comment 22

[Botond Ballo [:botond]]

(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

Comment 23

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 24

[Botond Ballo [:botond]]

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

Comment 25

[Botond Ballo [:botond]]

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 26

[Botond Ballo [:botond]]

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 27

[Botond Ballo [:botond]]

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 28

[Botond Ballo [:botond]]

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.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

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)

Comment 30

[Botond Ballo [:botond]]

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 31

[Botond Ballo [:botond]]

Updated to address review comments.

Try pushes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d2fd3df191c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d160d8129c2a

Comment 32

[Botond Ballo [:botond]]

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.

Comment 33

[Botond Ballo [:botond]]

Now that bug 1177018 has landed, this is ready to land too.

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48608a2a19b1

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/48608a2a19b1