dom/base/test/chrome/test_nsITextInputProcessor.xul | runCallbackTests(aForTests=false): window.moveBy(0, 10) should cause a notification got 2, expected 1

RESOLVED FIXED in Firefox 56

Status

()

Core
DOM
P2
normal
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: sparky, Assigned: sparky)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

11 months ago
This bug happens when we we migrate from ubuntu 12.04 to ubuntu 16.04 on mochitest-chrome. Here's a push with the failure itself: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25808fe7b96cc01b646184f227870ccd3551c440&filter-searchStr=c1

It fails because two 'notify-position-change' events are received at once (or before the check). You can see this through the debugging that was done here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3ec309477df5162cfcec5267adebcf03d7a0673&filter-searchStr=c1
(Assignee)

Comment 1

11 months ago
I believe that we are having this problem because 'callContinueTest' is true and since we are looking for only one notification, we hit this code: https://dxr.mozilla.org/mozilla-central/source/dom/base/test/chrome/window_nsITextInputProcessor.xul#3940

And this part says, "We didn't have enough notifications last time we checked, so wait for some more." However, it got the one notification that it needed already but the callContinueTest flag wasn't changed so it runs the callBack function a second time with the same notification from the looks of it. It's quite possible that this is not the case though, so I'm going to add some output and see what aTIP and TIP look like.

Here's a patch with a quick-fix for this: https://hg.mozilla.org/try/rev/f0b01aa3f62aebc155761b4c3d9ba2e59d7c12f2#l1.116

It fixes the test by checking first if there are more than one notification, if there aren't reduce the expected_notifications. Otherwise, look through the notifications and make sure that we are only looking at 'notify-position-change' events, if it is anything else, it will be dumped by reducing the expected_notifications value.

I'm also running a test right now to see what a passing tests output looks like: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7299cbb8679d20bd2b281e2fa00639692dc986c

Comment 2

11 months ago
Hi Greg, looks like you've been working on a fix for this, should I assign this bug to you?
Flags: needinfo?(gmierz2)
(Assignee)

Comment 3

11 months ago
Hi Hsin-Yi, sounds good to me.
Flags: needinfo?(gmierz2)
(Assignee)

Comment 4

11 months ago
I was wrong to assume that the callContinueTest value was causing the problem, especially since everything else works. I ran a test to see if we get any notifications before the window.moveBy call and we definitely do. Have a look at this run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93341e928ba1d3af731f950bbbf0040e42ac2124

Essentially, what seems to be happening is that we are getting a notification after the input.focus() call, and it is received/overwritten to notify-position-change after the window.moveby call.

I'm trying to fix the timeout that's happening right now in this push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=19ab6b0204293a3c22436f7406b5a5ffa608203f
(Assignee)

Comment 5

11 months ago
Figured it out...the other sub-tests are interfering with the test that is failing. I removed all the other tests which run before it and found that the input wasn't focused. I added the line 'input.focus()' just before the window.moveby tests and everything worked fine after that without any notification issues. So, in short, there are way too many calls to 'input.focus()'.

Another thing, this single test runs almost 16,000 sub-tests so it would be a good idea to break it down.
Assignee: nobody → gmierz2
Priority: -- → P2
Comment hidden (mozreview-request)

Comment 8

11 months ago
mozreview-review
Comment on attachment 8878899 [details]
Bug 1372924 - Handle input focus notifications at the right times in test_nsITextInputProcessor.xul.

https://reviewboard.mozilla.org/r/150138/#review155050
Attachment #8878899 - Flags: review?(jmaher) → review+

Comment 9

11 months ago
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/19ab596ae624
Handle input focus notifications at the right times in test_nsITextInputProcessor.xul. r=jmaher

Comment 10

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/19ab596ae624
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.