Closed Bug 1251905 Opened 4 years ago Closed 4 years ago

Enable test_continuous_wheel_events.html with e10s

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: martijn.martijn, Assigned: mccr8)

References

(Blocks 1 open bug, )

Details

(Whiteboard: btpp-active)

Attachments

(6 files, 4 obsolete files)

5.78 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
2.10 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
4.57 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
14.42 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
1.54 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
1.09 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
I was trying to convert test_continuous_wheel_events.html to use pushPrefEnv, but the problem is I'm already getting failures beforehand and afterwards, with the patch applied, I get even more failures (and passes)

Without patch:
Passed: 5356
Failed: 127

With patch:
Passed: 5372
Failed: 143

I guess the failures come from:
http://mxr.mozilla.org/mozilla-central/source/dom/events/test/mochitest.ini#150
 skip-if = buildapp == 'b2g' || e10s # b2g(5535 passed, 108 failed - more tests running than desktop) b2g-debug(5535 passed, 108 failed - more tests running than desktop) b2g-desktop(5535 passed, 108 failed - more tests running than desktop)

I guess nowadays Nightly debug builds run with e10s enabled.

Masayuki, you wrote this test, do you know what's going on here?
I don't understand why this test is failing for me.
Attachment #8724510 - Flags: review?(masayuki)
I'm not sure. Do the DOM events be fired synchronously even in e10s mode since it's testing on chrome file?

If NOT so, I guess that we should wait to finish each test until receiving a wheel event in system event group (really complicated event behavior, see https://developer.mozilla.org/en-US/docs/Web/Events/wheel#The_event_order_with_legacy_mouse_scroll_events and check SpecialPowers.addSystemEventListener()).

Otherwise, I have no idea for now...
It tursn out it was a MacOSX debug build, which doesn't have e10s enabled by default.
It makes me wonder why this test is failing for me locally, where it apparently is passing on Treeherder.
Ok, I get the failures also on an optimized MacOSX build.
I'm testing on MacOSX10.11, could it have something to do with that?
Attached patch 1251905_wheel.diff (obsolete) — Splinter Review
I put this on try to see if it doesn't fail there.
Attachment #8724510 - Attachment is obsolete: true
Attachment #8724510 - Flags: review?(masayuki)
I get the same problem with test_dom_wheel_event.html, btw. Also failures there.
When I run the test, it seems like the page gets zoomed in, although supposedly, the zoom level is at default (as well as the text zoom level).
Whiteboard: btpp-active
I've been working on fixing these tests, too, so we should coordinate.
(In reply to Andrew McCreight [:mccr8] from comment #7)
> I've been working on fixing these tests, too, so we should coordinate.

I meant, DOM event tests. Fortunately it looks like we haven't been looking at the same tests yet.

I also have problems with many of these tests failing locally on OSX. They run fine for me on Linux.
This might also need a similar treatment as in bug 1177651 in order for it to be able to work in e10s.
Blocks: 1218958
Is at least the pushPrefEnv stuff ready to be reviewed here? It is okay if the test doesn't work entirely with e10s.
Flags: needinfo?(martijn.martijn)
Blocks: 1257932
I have some patches locally to do this now, so I can just take up this bug.
I'll take this bug. I hope you don't mind.

There aren't any major issues with enabling this test, so I'll just land it all in this bug.
Assignee: martijn.martijn → continuation
No longer blocks: 1257932
Flags: needinfo?(martijn.martijn)
Summary: Convert test_continuous_wheel_events.html to use pushPrefEnv → Enable test_continuous_wheel_events.html with e10s
Duplicate of this bug: 1257932
try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=17df63b5c30f

I still need to do another run, because we don't run mochitest-plain with e10s on OSX or Windows...
(In reply to Andrew McCreight [:mccr8] from comment #15)
> I still need to do another run, because we don't run mochitest-plain with e10s on OSX or Windows...

Sorry, I meant with debug. We do for opt.
This enables later changes.

OSX and Windows try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b581da69e94

I'm waiting for a few OSX retriggers but it looks good so far.
Attachment #8733507 - Flags: review?(masayuki)
sendWheelAndPaint() doesn't properly handle legacy wheel events, so I changed the test to continue to use synthesizeWheel directly. However, this causes some intermittent failures when running with e10s. These seem to be fixed by waiting for paint flushing before sending the wheel event, as is done in sendWheelAndPaint().

kats, does this (the part at the bottom of the patch) seem like enough of sendWheelAndPaint() to avoid APZ-related flakiness? Rather than just sticking the waitForAllPaintsFlushed() in, I could also make some variant of sendWheelAndPaint that somehow handles the extra legacy wheel events and use that.
Attachment #8733512 - Flags: feedback?(bugmail.mozilla)
Just the trivial enabling, once everything else is fixed.
Attachment #8733513 - Flags: review?(masayuki)
Attachment #8724816 - Attachment is obsolete: true
This version just changes it to "part 6".
Attachment #8733514 - Flags: review?(masayuki)
Attachment #8733513 - Attachment is obsolete: true
Attachment #8733513 - Flags: review?(masayuki)
Blocks: e10s-tests
tracking-e10s: --- → +
Comment on attachment 8733512 [details] [diff] [review]
part 5 - Make the synthesize wheel async.

Review of attachment 8733512 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/test/test_continuous_wheel_events.html
@@ +3203,5 @@
>  
> +    // Keep APZ from ignoring the wheel event by waiting until the paint
> +    // is done. See sendWheelAndPaint() for more details.
> +    if (winUtils.isMozAfterPaintPending) {
> +      yield window.waitForAllPaintsFlushed(continueTest);

To be absolutely safe I'd recommend doing this:

yield window.waitForAllPaintsFlushed(function() {
  flushApzRepaints(continueTest);
});

where flushApzRepaints is in gfx/layers/apz/test/mochitest/apz_test_utils.js. You can include that with a <script> tag or inline the function if needed.

This will wait for all the pending paints to be completed, then also do a round-trip to the compositor to make sure any APZ-schedule paints have been pushed back to the main thread, and then continue. We do this in a number of APZ tests as we've found that not doing so results in intermittent failures. grep for "flushApzRepaints" in gfx/layers/apz/test/mochitest to see examples.
Attachment #8733512 - Flags: feedback?(bugmail.mozilla) → feedback+
Attachment #8733506 - Flags: review?(masayuki) → review+
Attachment #8733507 - Flags: review?(masayuki) → review+
Attachment #8733509 - Flags: review?(masayuki) → review+
Attachment #8733510 - Flags: review?(masayuki) → review+
Attachment #8733514 - Flags: review?(masayuki) → review+
The mouse wheel related events are fired in following order:

1. wheel in the default event group
2. DOMMouseScroll in both event groups (maybe)
3. MozMousePixelScroll in both default event groups
4. wheel in the system event group
https://developer.mozilla.org/en-US/docs/Web/Events/wheel#The_event_order_with_legacy_mouse_scroll_events

Therefore, I guess that EventUtils.sendWheelAndPaint() should use (add|remove)SystemEventListener() instead of (add|remove)EventListener().
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js?rev=5e4d9f0ec8cf&mark=580-580,607-607#563
(In reply to Andrew McCreight [:mccr8] from comment #13)
> I'll take this bug. I hope you don't mind.

Sorry for my lack of reply, thanks for taking this!
The other issues that needed to be fixed were above my head anyway.
Depends on: 1260148
With bug 1260148, this is much simpler. Thanks for the explanation of how this works. I didn't realize that the various kinds of wheel events all fired in a specific order, one at a time. Some of the other wheel tests could be altered in a similar way rather than having to compute a count of the expected number of events that will fire.

There was one timeout in the 14 times I triggered this test, but hopefully that is just a fluke.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=94dd746efa49
Attachment #8735474 - Flags: review?(masayuki)
Attachment #8733512 - Attachment is obsolete: true
Comment on attachment 8735474 [details] [diff] [review]
part 5 - Make the synthesize wheel async.

Nice!
Attachment #8735474 - Flags: review?(masayuki) → review+
You need to log in before you can comment on or make changes to this bug.