890555 - Handle non-object arguments to xpcshell head.js do_throw()

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[:Irving Reid (No longer working on Firefox)]

+++ 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...

Comment 1

[(not currently active) Ted Mielczarek]

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

Comment 3

[:Irving Reid (No longer working on Firefox)]

Attachment: Handle non-object argument to do_throw()

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

Comment 4

[(not currently active) Ted Mielczarek]

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?

Comment 5

[:Irving Reid (No longer working on Firefox)]

Attachment: Handle non-object argument to do_throw(), with test cases

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.

Comment 6

[Chris Manchester (limited bugmail, email directly)]

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?

Comment 7

[Chris Manchester (limited bugmail, email directly)]

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

Comment 8

[:Irving Reid (No longer working on Firefox)]

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.

Comment 9

[Jim Blandy :jimb]

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.

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Attachment: devtools_tests.patch

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 11

[Jim Blandy :jimb]

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)".

Comment 12

[Jim Blandy :jimb]

(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...

Comment 13

[Jim Blandy :jimb]

Attachment: Fix silent bugs in some toolkit/devtools/server xpcshell tests.

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

Comment 14

[Jim Blandy :jimb]

(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?

Comment 15

[Chris Manchester (limited bugmail, email directly)]

> ::: 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?

Comment 16

[Alexandre Poirot [:ochameau]]

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...

Comment 17

[Mark Banner (:standard8)]

(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?

Comment 18

[Alexandre Poirot [:ochameau]]

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.

Comment 19

[Dave Camp (:dcamp)]

(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.

Comment 20

[Chris Manchester (limited bugmail, email directly)]

Attachment: Fix strict mode warnings in devtools tests uncovered by fixing bug 89055;

Comment 21

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 22

[Mark Banner (:standard8)]

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.

Comment 23

[Chris Manchester (limited bugmail, email directly)]

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

Comment 24

[Gregory Szorc [:gps]]

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

Comment 25

[Jim Blandy :jimb]

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.

Comment 26

[Chris Manchester (limited bugmail, email directly)]

(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.

Comment 27

[Chris Manchester (limited bugmail, email directly)]

(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.

Comment 28

[Chris Manchester (limited bugmail, email directly)]

The group of fixes on try:

https://tbpl.mozilla.org/?tree=Try&rev=7257e3174422

Comment 29

[Panos Astithas (he/him) [:past] (please ni?)]

Clearing my request for information as Jim already handled this.

Comment 30

[Gregory Szorc [:gps]]

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

Comment 31

[Ed Morley [:emorley]]

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