+++ This bug was initially created as a clone of Bug #887799 +++ Bah humbug. The fix for bug 887799 didn't take into account that the 'error' argument to do_throw() isn't always an object. I suppose I really ought to write a couple of unit tests for this; in the mean time I'll request feedback instead of review...

FYI, http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/selftest.py has some harness unit tests.

And fix a couple of lowercase errors. Error objects have fileName while Stack objects have filename ;-(

Comment on attachment 774616 [details] [diff] [review] Handle non-object argument to do_throw() Review of attachment 774616 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for adding the selftests! ::: testing/xpcshell/selftest.py @@ +512,5 @@ > + self.assertInLog("TEST-UNEXPECTED-FAIL") > + self.assertInLog("error.js") > + self.assertInLog("Error object") > + self.assertInLog("ERROR STACK") > + self.assertNotInLog("TEST-PASS") Seems like the only thing you're not testing here is something that generates a legit Error object, like maybe a syntax error?

Added another test case as Ted suggested, and then flew into a duplicate-code-removing berserker rage. This passes clean on a Try build, except for pre-existing test failures caused by bug 887493; we weren't seeing those failures in the logs because do_throw was blowing up and not reporting them to the xpcshell harness.

I'm working on some changes to the way xpcshell does output for my summer project. I'd like to base these changes on the changes in this patch. When will it be feasible to land this?

The patch still applies cleanly to central, and I noticed its last dependency was resolved this morning. Pushed to try here: https://tbpl.mozilla.org/?tree=Try&rev=364fbc1eefb9

I apologise for the delayed response; I'm on the road right now. As you'll have noticed from your try build, fixing this bug exposes a whole bunch of failures that turn the tree orange, so based on an IRC conversation the sherrifs would prefer not to land the patch as is. As far as I can tell, almost all (if not all) of the test failures are caused by new JavaScript warnings triggering a listener in the devtools test harness, introduced by bug 749258. I've copied relevant people from that bug for advice. Aside from going through and actually fixing all the new JS warnings, is there another approach to cleaning up the failures? I'd like to get this landed soon so that even more hidden failures don't creep into the tree.

IRC conversation: Chris has offered to take a look at the devtools xpcshell tests that fail; fix the easy problems; and flag the rest for me to look at. For the latter, if the test is really borked, we'll disable it (or the failing portion) temporarily and file a follow-up bug; and it's not, I'll handle the more involved fix. Then we can land this. Tests that aren't doing their job are a big deal, so if a change to head.js can reveal problems we weren't aware of, we should get that cleaned up toot sweet.

These seemed like obvious fixes, and bring the number of test files that fail down from 101 to these 5 on my machine: toolkit/devtools/server/tests/unit/test_stepping-06.js toolkit/devtools/server/tests/unit/test_sourcemaps-07.js toolkit/devtools/server/tests/unit/test_blackboxing-05.js toolkit/devtools/server/tests/unit/test_blackboxing-04.js toolkit/devtools/apps/tests/unit/test_webappsActor.js

Comment on attachment 787048 [details] [diff] [review] devtools_tests.patch Review of attachment 787048 [details] [diff] [review]: ----------------------------------------------------------------- Thanks very much for this! f=me, with some comments addressed: ::: toolkit/devtools/server/actors/script.js @@ +344,5 @@ > let globalDebugObject; > try { > + if (this.dbg) { > + globalDebugObject = this.dbg.addDebuggee(aGlobal); > + } Was the problem that this.dbg was undefined or null? If so, then that's a serious bug, and this change would just conceal it. We shouldn't make this change; instead, we should open a separate bug for that. ::: toolkit/devtools/server/protocol.js @@ +870,5 @@ > let frozenSpec = desc.value._methodSpec; > let spec = {}; > spec.name = frozenSpec.name || name; > + spec.request = Request(object.merge({type: spec.name}, frozenSpec.request || undefined)); > + spec.response = Response(frozenSpec.response || undefined); So... this silences the warning because the reference now occurs in a conditional? Amazing. ::: toolkit/devtools/server/tests/unit/test_breakpoint-17.js @@ +106,5 @@ > return void finishClient(gClient); > } > > do_check_true(false, "Should never get here"); > + return undefined; Could you, instead, change the 'return void BLAH;' statements earlier to this: BLAH; return; So that we have no 'return VAL' statements in the function at all? ::: toolkit/devtools/server/tests/unit/test_protocol_children.js @@ +243,5 @@ > }), > > clearTemporaryChildren: protocol.custom(function() { > if (!this._temporaryHolder) { > + return undefined; This should return "resolve(undefined)".

(In reply to Chris Manchester [:chmanchester] from comment #10) > toolkit/devtools/server/tests/unit/test_sourcemaps-07.js This one is caused by bug 897031. > toolkit/devtools/server/tests/unit/test_stepping-06.js > toolkit/devtools/server/tests/unit/test_blackboxing-05.js > toolkit/devtools/server/tests/unit/test_blackboxing-04.js Fixed these. > toolkit/devtools/apps/tests/unit/test_webappsActor.js All the xpcshell tests under toolkit/devtools/apps fail for me for some reason...

Here are the fixes for the tests mentioned in the prior comment.

(In reply to Chris Manchester [:chmanchester] from comment #10) > toolkit/devtools/apps/tests/unit/test_webappsActor.js These tests are mysterious to me. They reference a global variable APP_ORIGIN, whose definition I'm unable to find at all. Alex, how are these supposed to work?

> ::: toolkit/devtools/server/actors/script.js > @@ +344,5 @@ > > let globalDebugObject; > > try { > > + if (this.dbg) { > > + globalDebugObject = this.dbg.addDebuggee(aGlobal); > > + } > > Was the problem that this.dbg was undefined or null? If so, then that's a > serious bug, and this change would just conceal it. We shouldn't make this > change; instead, we should open a separate bug for that. > The warning is about this.dbg being undefined. I looked at this a bit and don't know that it's a real bug: it looks like this.dbg is always set in _initDebugger. I think this should either stay this way, or define this.dbg to be null in the constructor. Does that make sense?

I don't exactly know why the webapps xpcshell test is being run, but it should not. They have been disabled as following moz.build doesn't register the unit folder: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/apps/tests/moz.build You should be able to safely ignore the test_webappsActor.js exception. I've been pulled away from fixing this test, but I meant to fix it in bug 893848, testing stuff on b2g appears to be really challenging...

(In reply to Alexandre Poirot (:ochameau) from comment #16) > I don't exactly know why the webapps xpcshell test is being run, but it > should not. > They have been disabled as following moz.build doesn't register the unit > folder: > http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/apps/tests/ > moz.build However, it does look like they are enabled here: http://hg.mozilla.org/mozilla-central/annotate/fd4cf30428b0/toolkit/devtools/apps/tests/Makefile.in#l11 (and that hasn't been ported to the new-style in moz.build). If they are really meant to be disabled why not use skip-if = true in the xpcshell.ini file with a reference to the bug?

Long story, they have been disabled by mistake when switching to the new moz.build behavior (the is a bug to fix that. That will land when the test is going to be fixed) and then silently regressed and can be considered as disabled without having any bug where we explicitely disabled them. Please, do not hesitate to add skip-if if it can help landing this patch.

(In reply to Chris Manchester [:chmanchester] from comment #15) > > ::: toolkit/devtools/server/actors/script.js > > @@ +344,5 @@ > > > let globalDebugObject; > > > try { > > > + if (this.dbg) { > > > + globalDebugObject = this.dbg.addDebuggee(aGlobal); > > > + } > > > > Was the problem that this.dbg was undefined or null? If so, then that's a > > serious bug, and this change would just conceal it. We shouldn't make this > > change; instead, we should open a separate bug for that. > > > > The warning is about this.dbg being undefined. I looked at this a bit and > don't know that it's a real bug: it looks like this.dbg is always set in > _initDebugger. I think this should either stay this way, or define this.dbg > to be null in the constructor. Does that make sense? We expect this.dbg to be defined and non-null before addDebuggee is called. If you were ever seeing a null this.dbg in our tests, there's something we need to investigate to find the real bug.

Standard8 pointed me to bug 902934, which looks like a related failure. Could someone familiar with the source maps code see where a similar fix might be needed? I couldn't tell from the test file where appinfo is involved.

I think I saw some errors in your try build from comment 7 which were "gApp is not defined". This is coming from here: http://hg.mozilla.org/mozilla-central/annotate/264e54846d4a/toolkit/mozapps/extensions/nsBlocklistService.js#l69 xpcshell doesn't generally have an nsIXULAppInfo instance defined, hence the blocklist service won't be able to find it - although I must admit I was getting inconsistent results locally and couldn't figure out why. I believe you can hit this if your tests happen to touch the blocklist service in some remote way.

This is a try run with the fixes to tests to see where we are, marking toolkit/devtools/server/tests/unit/test_sourcemaps-07.js as a known failure: https://tbpl.mozilla.org/?tree=Try&rev=376398d9060c

Regarding getting an nsIXULAppInfo from xpcshell tests: https://developer.mozilla.org/en-US/docs/Using_nsIXULAppInfo#Getting_nsIXULAppInfo_in_xpcshell_tests

Comment on attachment 787619 [details] [diff] [review] Fix strict mode warnings in devtools tests uncovered by fixing bug 89055; Review of attachment 787619 [details] [diff] [review]: ----------------------------------------------------------------- r=me, but the change to ThreadAgent.prototype.addDebuggee must be removed. ::: toolkit/devtools/server/actors/script.js @@ +344,5 @@ > let globalDebugObject; > try { > + if (this.dbg) { > + globalDebugObject = this.dbg.addDebuggee(aGlobal); > + } As I said before, this change shouldn't go in. This hides the symptom without addressing the cause.

(In reply to Jim Blandy :jimb from comment #25) > Comment on attachment 787619 [details] [diff] [review] > Fix strict mode warnings in devtools tests uncovered by fixing bug 89055; > > Review of attachment 787619 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me, but the change to ThreadAgent.prototype.addDebuggee must be removed. > > ::: toolkit/devtools/server/actors/script.js > @@ +344,5 @@ > > let globalDebugObject; > > try { > > + if (this.dbg) { > > + globalDebugObject = this.dbg.addDebuggee(aGlobal); > > + } > > As I said before, this change shouldn't go in. This hides the symptom > without addressing the cause. I think I tracked down the cause. Reported in bug 903801.

(In reply to Jim Blandy :jimb from comment #12) > (In reply to Chris Manchester [:chmanchester] from comment #10) > > toolkit/devtools/server/tests/unit/test_sourcemaps-07.js Nick and I tracked this one down. Reported in bug 904808.

The group of fixes on try: https://tbpl.mozilla.org/?tree=Try&rev=7257e3174422

Clearing my request for information as Jim already handled this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b9c5de825c6 https://hg.mozilla.org/integration/mozilla-inbound/rev/acd6f7fec0e1 https://hg.mozilla.org/integration/mozilla-inbound/rev/135c0ebda47b

https://hg.mozilla.org/mozilla-central/rev/5b9c5de825c6 https://hg.mozilla.org/mozilla-central/rev/acd6f7fec0e1 https://hg.mozilla.org/mozilla-central/rev/135c0ebda47b