Last Comment Bug 550481 - Exceptions from the do_execute_soon callback should be logged and fail the test
: Exceptions from the do_execute_soon callback should be logged and fail the test
Status: RESOLVED FIXED
:
Product: Testing
Classification: Components
Component: XPCShell Harness (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Dave Townsend [:mossop]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-05 08:17 PST by Dave Townsend [:mossop]
Modified: 2010-03-09 11:13 PST (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch rev 1 (4.17 KB, patch)
2010-03-05 12:43 PST, Dave Townsend [:mossop]
no flags Details | Diff | Splinter Review
patch rev 1 (5.58 KB, patch)
2010-03-05 14:01 PST, Dave Townsend [:mossop]
ted: review+
Details | Diff | Splinter Review

Description Dave Townsend [:mossop] 2010-03-05 08:17:47 PST
Currently if the callback for do_execute_soon throws an exception other than the do_check_ tests nothing is displayed and the test hangs (because do_test_finished won't get called). Instead we should catch any exception, log it and end the test.
Comment 1 Dave Townsend [:mossop] 2010-03-05 12:43:36 PST
Created attachment 430709 [details] [diff] [review]
patch rev 1

This catches any exception, for those that weren't triggered by do_throw in the first place we log the exception and any available stack (it's only available for pure JS exceptions, XPCOM exceptions won't have any stack info).

This also adds an automatic do_test_pending and do_test_finished saving the caller from doing so (though there is no harm in them still doing so).

Ran this through the try server and everything still passes. If we're lucky this might show more info for some of the intermittent failures in xpcshell tests, but worse case it still makes debugging these tests much easier.
Comment 2 Dave Townsend [:mossop] 2010-03-05 12:49:43 PST
Comment on attachment 430709 [details] [diff] [review]
patch rev 1

Actually let me change this a bit
Comment 3 Dave Townsend [:mossop] 2010-03-05 14:01:54 PST
Created attachment 430729 [details] [diff] [review]
patch rev 1

Nicer version that does the stack trace for exceptions from run_test too.
Comment 4 Ted Mielczarek [:ted.mielczarek] 2010-03-08 05:03:13 PST
Comment on attachment 430729 [details] [diff] [review]
patch rev 1

>diff --git a/testing/xpcshell/example/unit/test_execute_soon.js b/testing/xpcshell/example/unit/test_execute_soon.js
>new file mode 100644
>--- /dev/null
>+++ b/testing/xpcshell/example/unit/test_execute_soon.js
>@@ -0,0 +1,52 @@
>+/* vim:set ts=2 sw=2 sts=2 et: */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1

It's acceptable to license test files in the Public Domain nowadays:
https://wiki.mozilla.org/License_Policy#Acceptable_Licenses


>diff --git a/testing/xpcshell/head.js b/testing/xpcshell/head.js
>--- a/testing/xpcshell/head.js
>+++ b/testing/xpcshell/head.js
>@@ -187,22 +206,42 @@ function _load_files(aFiles) {
> 
> function do_timeout(delay, func) {
>   var timer = Components.classes["@mozilla.org/timer;1"]
>                         .createInstance(Components.interfaces.nsITimer);
>   timer.initWithCallback(new _TimerCallback(func, timer), delay, timer.TYPE_ONE_SHOT);
> }
> 
> function do_execute_soon(callback) {
>+  do_test_pending();
>   var tm = Components.classes["@mozilla.org/thread-manager;1"]
>                      .getService(Components.interfaces.nsIThreadManager);
> 
>   tm.mainThread.dispatch({
>     run: function() {
>-      callback();
>+      try {
>+        callback();
>+      } catch (e) {
>+        // Print exception, but not do_throw() result.
>+        // Hopefully, this won't mask other NS_ERROR_ABORTs.
>+        if (!_quit || e != Components.results.NS_ERROR_ABORT) {
>+          dump("TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | " + e);

Hm, do you really want the !_quit here? I admit, I find that if statement hard to follow even in the execute_test() bit you copied it from.

This is good stuff, should help diagnose random orange in these cases. Also the extra do_test_{pending,cleanup} pair is a nice touch.
Comment 5 Dave Townsend [:mossop] 2010-03-08 10:18:23 PST
(In reply to comment #4)
> (From update of attachment 430729 [details] [diff] [review])
> >diff --git a/testing/xpcshell/example/unit/test_execute_soon.js b/testing/xpcshell/example/unit/test_execute_soon.js
> >new file mode 100644
> >--- /dev/null
> >+++ b/testing/xpcshell/example/unit/test_execute_soon.js
> >@@ -0,0 +1,52 @@
> >+/* vim:set ts=2 sw=2 sts=2 et: */
> >+/* ***** BEGIN LICENSE BLOCK *****
> >+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
> 
> It's acceptable to license test files in the Public Domain nowadays:
> https://wiki.mozilla.org/License_Policy#Acceptable_Licenses

Fine by me, I'll update it.

> >+        // Print exception, but not do_throw() result.
> >+        // Hopefully, this won't mask other NS_ERROR_ABORTs.
> >+        if (!_quit || e != Components.results.NS_ERROR_ABORT) {
> >+          dump("TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | " + e);
> 
> Hm, do you really want the !_quit here? I admit, I find that if statement hard
> to follow even in the execute_test() bit you copied it from.

I think yes. If _quit is false then no test fail has been caught by a do_check_... test so we certainly should log the exception. Likewise if e is not NS_ERROR_ABORT then this exception did not come from a do_check_... fail (could be an exception that happened after such a test fail) so we should log it. The only ones we don't log are those where _quit is true and e is NS_ERROR_ABORT in which case the exception is likely just the one from the do_check_... failure and has already been logged, though as the comment suggests it isn't impossible for us to be hiding other exceptions there.
Comment 6 Dave Townsend [:mossop] 2010-03-09 11:13:28 PST
http://hg.mozilla.org/mozilla-central/rev/29cb33290f7e

Note You need to log in before you can comment on or make changes to this bug.