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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Greg Mierzwinski [:sparky]]

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

Comment 1

[Greg Mierzwinski [:sparky]]

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

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Greg, looks like you've been working on a fix for this, should I assign this bug to you?

Comment 3

[Greg Mierzwinski [:sparky]]

Hi Hsin-Yi, sounds good to me.

Comment 4

[Greg Mierzwinski [:sparky]]

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

Comment 5

[Greg Mierzwinski [:sparky]]

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.

Comment 6

[Greg Mierzwinski [:sparky]]

Attachment: Bug 1372924 - Handle input focus notifications at the right times in test_nsITextInputProcessor.xul.

This patch prevents a part of the test from creating a notify-focus event at random times later in the test by taking care of it as soon as it can be received. It also removes an unecessary focus call.

Review commit: https://reviewboard.mozilla.org/r/150138/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150138/

Comment 7

[Greg Mierzwinski [:sparky]]

On Ubuntu 16: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c041857b1b070070326b1cd71e1abdbbc3f340e

On Ubuntu 12: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bede219f13807f8deb987fee540b718cbcab9d01

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19ab596ae624
Handle input focus notifications at the right times in test_nsITextInputProcessor.xul. r=jmaher

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/19ab596ae624