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

RESOLVED INCOMPLETE

Status

defect
RESOLVED INCOMPLETE
5 years ago
Last year

People

(Reporter: emorley, Assigned: martijn.martijn)

Tracking

(Blocks 2 bugs)

Trunk
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

5 years ago
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
Reporter

Comment 1

5 years ago
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]
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.
@ahal: Any idea what might cause this? Can you confirm this was before structured logging landed?
Flags: needinfo?(ahalberstadt)
Yeah, changeset c682a6b343ba is push #194785 while the structured logging landing was push #194804. So it isn't caused by that.
Flags: needinfo?(ahalberstadt)
Reporter

Updated

5 years ago
No longer blocks: 886570
Assignee

Comment 5

5 years ago
(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.
Assignee: nobody → martijn.martijn
Assignee

Comment 6

5 years ago
Posted patch 1051791.diff (obsolete) — Splinter Review
This would make this test fail.
Assignee

Comment 7

5 years ago
Posted patch 1051791_2.diff (obsolete) — Splinter Review
This would catch errors for all popup windows.
Assignee

Comment 8

5 years ago
Given bug 744125, I'm surprised this didn't give more failures.
Blocks: 744125
Assignee

Comment 9

5 years ago
(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
Assignee

Comment 10

5 years ago
Posted patch 1051791_v3.diff (obsolete) — Splinter Review
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
Attachment #8474593 - Attachment is obsolete: true
Assignee

Comment 11

5 years ago
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.
Attachment #8474581 - Attachment is obsolete: true
Attachment #8474653 - Attachment is obsolete: true
Attachment #8475164 - Flags: review?(jmaher)
Assignee

Comment 12

5 years ago
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 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?
Attachment #8475164 - Flags: review?(jmaher) → review-
Assignee

Comment 14

5 years ago
(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.
Reporter

Comment 15

Last year
Mass-closing old bugs I filed that have not had recent activity/no longer affect me.
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.