Closed Bug 1115092 Opened 7 years ago Closed 7 years ago

test_imestate.html fails when run as a standalone directory

Categories

(Core :: Plug-ins, defect)

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 926830

People

(Reporter: vaibhav1994, Unassigned)

References

Details

(Keywords: regression)

Attachments

(1 file)

in bug 1110982, we are looking to enable tests where we run a fresh browser instance per directory.  Usually what happens is that a few tests fail because they accidentally depend on the state of the browser from an earlier test.

In the try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1a5fa04f057a

In linux opt/debug oth chunk, this test fails:
 3546 INFO TEST-UNEXPECTED-FAIL | widget/tests/test_imestate.html | runPluginTest: unexpected enabled state when plugin has focus - got 0, expected 3 

 3548 INFO TEST-UNEXPECTED-FAIL | widget/tests/test_imestate.html | runPluginTest: unexpected enabled state when plugin has focus #2 - got 0, expected 3
Blocks: 1110982
I can reproduce this locally by running:
./mach mochitest-chrome widget/tests


local is a current inbound tree for linux64 opt build.

Here is a clip from my console:
570 INFO TEST-PASS | widget/tests/test_imestate.html | Testing of normal contents: designMode editor, The latest MozIMEFocus* event must be MozIMEFocusIn 
571 INFO TEST-PASS | widget/tests/test_imestate.html | Testing of normal contents: designMode editor, wrong enabled state 
572 INFO TEST-PASS | widget/tests/test_imestate.html | Testing of normal contents: designMode editor, MozIMEFocusOut event must be fired after IME state is updated 
573 INFO TEST-PASS | widget/tests/test_imestate.html | runPluginTest: unexpected enabled state when no element has focus 
574 INFO TEST-UNEXPECTED-FAIL | widget/tests/test_imestate.html | runPluginTest: unexpected enabled state when plugin has focus - got 0, expected 3
575 INFO TEST-PASS | widget/tests/test_imestate.html | runPluginTest: unexpected enabled state when plugin has focus 
576 INFO TEST-UNEXPECTED-FAIL | widget/tests/test_imestate.html | runPluginTest: unexpected enabled state when plugin has focus #2 - got 0, expected 3
577 INFO TEST-PASS | widget/tests/test_imestate.html | runPluginTest: unexpected enabled state when plugin is removed from tree 
578 INFO TEST-PASS | widget/tests/test_imestate.html | runPluginTest: unexpected enabled state when input[type=text] has focus 
579 INFO TEST-PASS | widget/tests/test_imestate.html | Testing [contentEditable="true"]: unexpected enabled state when no element has focus 
580 INFO TEST-PASS | widget/tests/test_imestate.html | Testing [contentEditable="true"]: input[type=text], MozIMEFocusOut event must be fired with expected IME state if the state isn't being changed 

Masayuki, can you take a look at this?
Flags: needinfo?(masayuki)
Hmm, I already have a lot of jobs in my queue... So, I cannot work on this soon.
Flags: needinfo?(masayuki)
Masayuki, it has been a couple of weeks, is there any time you have in the next week or two to look into this?
Flags: needinfo?(masayuki)
I think no. The test must pass only when it runs alone. So, another test running before it has a bug may have a bug. Therefore, I'm not sure if it's my fault...
Flags: needinfo?(masayuki)
actually running test_imestate.html by itself:  ./mach mochitest-chrome widget/tests/test_imestate.html, yields a failure:
TEST-INFO | Main app process: exit 0

runtests.py | Application ran for: 0:00:07.828421
zombiecheck | Reading PID log: /tmp/tmpLrepPIpidlog
Stopping web server
Stopping web socket server
Stopping ssltunnel
WARNING | leakcheck | refcount logging is off, so leaks can't be detected!
runtests.py | Running tests: end.
The following tests failed:
3048 INFO TEST-UNEXPECTED-FAIL | widget/tests/test_imestate.html | runPluginTest: unexpected enabled state when plugin has focus - got 0, expected 3
3049 INFO TEST-UNEXPECTED-FAIL | widget/tests/test_imestate.html | runPluginTest: unexpected enabled state when plugin has focus #2 - got 0, expected 3
3050 INFO TEST-START | Shutdown
3051 INFO Passed:  3500
3052 INFO Failed:  2
3053 INFO Todo:    9
3054 INFO Slowest: 5177ms - chrome://mochitests/content/chrome/widget/tests/test_imestate.html
3055 INFO SimpleTest FINISHED
3056 INFO TEST-INFO | Ran 1 Loops
3057 INFO SimpleTest FINISHED
SUITE-END | took 9s


This fails standalone or as a directory.  This implies that something in a previous directory is causing this to pass.  If there is no time to work on this in the short term, we can disable this test until there is time.
Hmm, that's odd. The test (runPluginTest()) was added by bug 531810 which is fixed at Gecko 2.0 (Firefox 4). So, OOPP is already enabled. I guess something of a way to test plugins has been changed?
Okay, probably, this is a regression of bug 899080. It's not fix this test by https://bugzilla.mozilla.org/attachment.cgi?id=784894&action=diff
Blocks: 899080
Assignee: nobody → masayuki
Component: Event Handling → Plug-ins
OS: Linux → All
Version: unspecified → Trunk
Status: NEW → ASSIGNED
Attached patch PatchSplinter Review
This should have been added by bug 899080.
Attachment #8545181 - Flags: review?(gfritzsche)
And I guess one or more tests outside of /widget/tests are disabling ClickToPlay and not restoring the pref. It's another side of this bug.
is there a pref we should be looking for related to clicktoplay?
Comment on attachment 8545181 [details] [diff] [review]
Patch

We should be using setTestPluginEnabledState() (or similar) here so we properly reset the enabledState after the test and to not explicitly repeat that code in every single test.
Attachment #8545181 - Flags: review?(gfritzsche)
(In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> Comment on attachment 8545181 [details] [diff] [review]
> Patch
> 
> We should be using setTestPluginEnabledState() (or similar) here so we
> properly reset the enabledState after the test and to not explicitly repeat
> that code in every single test.

I don't understand the code. I just added what you added to other tests in the bug. This is really your regression. Could you take this if you don't like your approach of bug 899080?
Flags: needinfo?(gfritzsche)
Keywords: regression
Just because i didn't do the right thing in all the tests for bug 899080 doesn't make it proper code - see bug 926830.

Also, i don't have time for this for at least the next two weeks.
Flags: needinfo?(gfritzsche)
(In reply to Georg Fritzsche [:gfritzsche] from comment #13)
> Just because i didn't do the right thing in all the tests for bug 899080
> doesn't make it proper code - see bug 926830.

Bug 926830 is now completely left... And the reason why you gave up to fix that is the mysterious memory leak. So, if we would take similar approach in this bug, wouldn't it cause same trouble?

> Also, i don't have time for this for at least the next two weeks.

Me too. I'd like you to r+ at least for now.
Flags: needinfo?(gfritzsche)
The leak issues are not necessarily coming up when doing the proper approach from there for widget/tests only (or even this test only). They might be only with the whole patch stack... or even already fixed by another bug.
The issue in comment 0 should be fixed by the proper setTestPluginEnabledState(); i don't think we should introduce more places where we insert stale code into the tree.
Flags: needinfo?(gfritzsche)
(In reply to Georg Fritzsche [:gfritzsche] from comment #15)
> The issue in comment 0 should be fixed by the proper
> setTestPluginEnabledState(); i don't think we should introduce more places
> where we insert stale code into the tree.

... stale state.
Okay, then, please take this even if you can fix this two or more weeks later. If I'll have much time for this, I'll steal this again.

(In reply to Joel Maher (:jmaher) from comment #5)
> If there is no time to work on
> this in the short term, we can disable this test until there is time.

If you need to disable the test, please do not disable whole of test_imestate.html because it's one of the most important test for IME users. If you will do that, just comment out the call of runPluginTest().
Assignee: masayuki → gfritzsche
Status: ASSIGNED → NEW
Flags: needinfo?(jmaher)
Please don't assigns things to me that i don't currently have time for.
Assignee: gfritzsche → nobody
FWIW, i don't think doing the proper thing here is really more effort than adding the code in the proposed patch, hence i don't understand the back-and-forth here.
alright, there is a widgets patch in bug 926830 which covers all but two test cases (https://bugzilla.mozilla.org/attachment.cgi?id=8377135),  This was landed back in February 2014 and backed out for memory leaks, I have modified that patch and added support to the two test cases (one of them being test_imestate.html).

This I will push up to try, do a bunch of retriggers and get it landed if all goes well.  It is my understanding that bug 926830 will be closed once the widgets/tests/ directory patch is landed.  In the last 11 months a lot has changed in our test frameworks and product, there is a good chance this won't be leaking anymore.

If there is something I am overlooking in going this route, please speak up.
Flags: needinfo?(jmaher)
That sounds good, thanks.
Note though that there are more patches on bug 926830 then the one for widget/tests. They should all land to close bug 926830.
However, we can just land the patch for widget/tests here as needed.
My understanding is that all the patches in bug 926830 landed and only the widgets/tests one was backed out.  Either way, if this sticks, we can rebaseline the others and work to land them as well!
I have a patch up for review on bug 926830, it makes sense to just mark this as a duplicate of that :)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 926830
You need to log in before you can comment on or make changes to this bug.