1051791 - Uncaught exception in OS X 10.6 mochitest-4 is not making the job fail

Status: RESOLVED INCOMPLETE

(Testing :: Mochitest, defect)

Description

[Ed Morley [:emorley]]

I've just noticed that there are failures within OS X 10.6 mochitest-4 jobs that are not turning the run orange.

Bug 1051783 is filed for fixing the failure itself, this bug is for fixing the harness so that the job is actually marked as a failure.

Calling this a regression from bug 886570 until proven otherwise :-)

Example logs...
opt: https://tbpl.mozilla.org/php/getParsedLog.php?id=44078966&tree=Mozilla-Inbound
debug: https://tbpl.mozilla.org/php/getParsedLog.php?id=44077183&tree=Mozilla-Inbound

Comment 1

[Ed Morley [:emorley]]

22:10:56     INFO -  [POINTERLOCK] Starting file_retargetMouseEvents.html
22:10:56     INFO -  [974] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /builds/slave/m-in-osx64-d-00000000000000000/build/dom/events/ContentEventHandler.cpp, line 110
22:10:56     INFO -  [974] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /builds/slave/m-in-osx64-d-00000000000000000/build/dom/events/ContentEventHandler.cpp, line 110
22:10:56     INFO -  [974] WARNING: Should have pointer locked element, but didn't.: file /builds/slave/m-in-osx64-d-00000000000000000/build/dom/events/EventStateManager.cpp, line 3638
22:10:56     INFO -  [974] WARNING: Should have pointer locked element, but didn't.: file /builds/slave/m-in-osx64-d-00000000000000000/build/dom/events/EventStateManager.cpp, line 3638
22:10:56     INFO -  476 INFO TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | uncaught exception - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendMouseEvent] at chrome://specialpowers/content/specialpowersAPI.js:150
22:10:56     INFO -  JavaScript error: chrome://specialpowers/content/specialpowersAPI.js, line 150: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendMouseEvent]

Comment 2

[Ahmed Kachkach [:akachkach]]

Here's the code path that seems to be executed:
http://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#1313

I don't understand why this isn't taken into account in the summary.

Btw, the example logs are from before we landed structured logging ("TEST-END" instead of "TEST-OK", memory info that isn't buffered, etc.) so I don't think this is a regression from that patch.

Comment 3

[Ahmed Kachkach [:akachkach]]

@ahal: Any idea what might cause this? Can you confirm this was before structured logging landed?

Comment 4

[Andrew Halberstadt [:ahal]]

Yeah, changeset c682a6b343ba is push #194785 while the structured logging landing was push #194804. So it isn't caused by that.

Comment 5

[Martijn Wargers (dead)]

(In reply to Ahmed Kachkach [:akachkach] from comment #2)
> Here's the code path that seems to be executed:
> http://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/
> SimpleTest/SimpleTest.js#1313
> 
> I don't understand why this isn't taken into account in the summary.

This is because file_retargetMouseEvents.html is opened in a popup window and it's using SimpleTest.js, but is using it's own ok/is/etc to retarget those to the opener window. But it forgets about the window.onerror case.
http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/pointerlock/file_retargetMouseEvents.html?force=1
http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/pointerlock/pointerlock_utils.js?force=1
In this case, pointerlock_utils.js could add a window.onerror function and retarget it to the opener window.

But in general, I wish that tests wouldn't use SimpleTest.js in popup windows, I think it would be better to use opener.SimpleTest.is and so on.
SimpleTest.js should have some method to track all errors generated in popup windows, but I don't know if that's possible.

Comment 6

[Martijn Wargers (dead)]

Attachment: 1051791.diff

This would make this test fail.

Comment 7

[Martijn Wargers (dead)]

Attachment: 1051791_2.diff

This would catch errors for all popup windows.

Comment 8

[Martijn Wargers (dead)]

Given bug 744125, I'm surprised this didn't give more failures.

Comment 9

[Martijn Wargers (dead)]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #7)
> Created attachment 8474593 [details] [diff] [review]
> 1051791_2.diff
> 
> This would catch errors for all popup windows.

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=91c2977b3fa4

Comment 10

[Martijn Wargers (dead)]

Attachment: 1051791_v3.diff

This should fix all the failures I saw in the previous try run, pushed this to try:
https://tbpl.mozilla.org/?tree=Try&rev=b136e443c5bc

Comment 11

[Martijn Wargers (dead)]

Attachment: 1051791_v4.diff

Now with a test.
There were no failures in the try run that were caused by the patch.
I tried to support nested popups, but I get some weird errors, so for now, I focused on single popups.
This:
+      SpecialPowers.wrap(win)
was necessary, because there is are tests like docshell/test/navigation/test_opener.html that open windows with enhanced privileges.

Comment 12

[Martijn Wargers (dead)]

Oh, this if call, btw, is because I can't seem to overload window.open in chrome documents:
+if (window.location.href.indexOf('chrome://') != 0) {
So that means chrome/browser-chrome mochitests don't have this onerror handling.
But those errors might already been handled in some other way, not sure.

Comment 13

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

Comment on attachment 8475164 [details] [diff] [review]
1051791_v4.diff

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

a few concerns.

::: testing/mochitest/tests/Harness_sanity/test_bug1051791.html
@@ +18,5 @@
> +  }
> +}
> +function endtest() {
> +  is(win.closed, true, "Popup window should be closed");
> +  is(SimpleTest._tests.length, 2, "2 tests should have been run");

what 2 tests, win.closed=true and uncaught exception?

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +1301,5 @@
>  var info = SimpleTest.info;
>  
>  var gOldOnError = window.onerror;
> +
> +SimpleTest.simpletestOnerror = function(errorMsg, url, lineNumber) {

s/simpletestOnerror/simpletest_onerror/

@@ +1353,5 @@
> +  var gOldWindowOpen = window.open;
> +  window.open = function(aUrl, aName, aFeatures) {
> +    var win = gOldWindowOpen(aUrl, aName, aFeatures);
> +    if (win) {
> +      gOldOnError = SpecialPowers.wrap(win).onerror;

shouldn't we be avoiding .wrap(win)?

@@ +1356,5 @@
> +    if (win) {
> +      gOldOnError = SpecialPowers.wrap(win).onerror;
> +      SpecialPowers.wrap(win).onerror = function(errorMsg, url, lineNumber) {
> +        SimpleTest.simpletestOnerror(errorMsg, url, lineNumber);
> +        win.close();

why do we do a win.close() here and not above?

Comment 14

[Martijn Wargers (dead)]

(In reply to Joel Maher (:jmaher) from comment #13)
> Comment on attachment 8475164 [details] [diff] [review]
> 1051791_v4.diff
> 
> Review of attachment 8475164 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> a few concerns.
> 
> ::: testing/mochitest/tests/Harness_sanity/test_bug1051791.html
> @@ +18,5 @@
> > +  }
> > +}
> > +function endtest() {
> > +  is(win.closed, true, "Popup window should be closed");
> > +  is(SimpleTest._tests.length, 2, "2 tests should have been run");
> 
> what 2 tests, win.closed=true and uncaught exception?

Indeed.

> @@ +1353,5 @@
> > +  var gOldWindowOpen = window.open;
> > +  window.open = function(aUrl, aName, aFeatures) {
> > +    var win = gOldWindowOpen(aUrl, aName, aFeatures);
> > +    if (win) {
> > +      gOldOnError = SpecialPowers.wrap(win).onerror;
> 
> shouldn't we be avoiding .wrap(win)?

SpecialPowers.wrap(win) was necessary here, because there is are tests like docshell/test/navigation/test_opener.html that open windows with enhanced privileges.

> @@ +1356,5 @@
> > +    if (win) {
> > +      gOldOnError = SpecialPowers.wrap(win).onerror;
> > +      SpecialPowers.wrap(win).onerror = function(errorMsg, url, lineNumber) {
> > +        SimpleTest.simpletestOnerror(errorMsg, url, lineNumber);
> > +        win.close();
> 
> why do we do a win.close() here and not above?

You're right, I'll change that.

Comment 15

[Ed Morley [:emorley]]

Mass-closing old bugs I filed that have not had recent activity/no longer affect me.