898122 - FormatFrame ignores JSAPI failures

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

A local debug build of fx-team tip fails many debugger xpcshell tests with:

Assertion failure: parent, at /Users/past/src/fx-team/js/src/jswrapper.cpp:29

This has been happening since at least a couple of days - maybe more, I've been mainly working with mochitests lately - and has been observed by me and mikeratcliffe in 3 different systems, Mac and Linux. The following command results in 65 test failures on my Mac:

mach xpcshell-test toolkit/devtools/server/tests/unit/

One of them is test_blackboxing-01.js, when the code is displaying the stack trace from a debugger statement (i.e. the stack trace is never displayed in full). This may be the case for all failures, an attempt to print the stack for a debugger statement from SpiderMonkey. Here is part of the output from this test:

0:00.47 TEST-PASS | undefined | [undefined : undefined] 2 == 2
0:00.47 DBG-SERVER: Packet 27 sent to "conn0.context1"
0:00.47 DBG-SERVER: Received packet 27: {
0:00.47   "to": "conn0.context1",
0:00.47   "type": "resume"
0:00.47 }
0:00.47 DBG-SERVER: Packet 28 sent from "conn0.context1"
0:00.48 ------------------------------------------------------------------------
0:00.48 Hit JavaScript "debugger" keyword. JS call stack...
0:00.48 Assertion failure: parent, at /Users/past/src/fx-team/js/src/jswrapper.cpp:29


What I really can't figure out is why this assertion doesn't appear in tbpl causing the tests to fail. I can reproduce it with either mach or make, and with either running a single test or the whole suite (mbrubeck suggested that there might be a difference, but sadly there was none).

This has been making it hard for me to write new xpcshell tests as I keep hitting this unrelated assertion, so I'd be grateful for any insight you can provide me. CCing bholley for his experience with wrappers, njn because "hg blame" says he recently touched the code near this assertion and jimb as our resident SpiderMonkey expert. I also asked ejpbruel about it, but he didn't have any ideas.

Comment 1

[Bobby Holley (:bholley)]

So, the issue here is that we end up calling into JS_NewGlobalObject with an exception already set on cx. So when we create a compartment, and enter it to create a global, we end up trying to wrap the pending exception into the global-less compartment, causing a crash.

The exception appears to be that same darn exception in nsExternalHelperAppService that I described in a recent comment:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/tests/unit/test_sourcemaps-04.js#14

I guess we need to figure out how to get that code to avoid calling into the JS engine after it throws. I'll take a look.

Comment 2

[Bobby Holley (:bholley)]

Ok, the nsExternalHelperAppService thing was a red herring. The problem is actually further up.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add some helpful assertions into the JS engine. v1

Things go wrong much earlier than the assertion we currently hit.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Properly squelch exceptions in FormatFrame. v1

Given that this is a debugging tool, we don't really want to just throw in the
towel if we fail to glean all possible information during stack introspection.
Clear the exception and note the failure to the stream.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 781894 [details] [diff] [review]
Part 1 - Add some helpful assertions into the JS engine. v1

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

::: js/src/jsapi.cpp
@@ +3346,5 @@
>      JS_THREADSAFE_ASSERT(cx->compartment() != cx->runtime()->atomsCompartment);
> +    // When we enter the newly-created compartment to create the global, we can't
> +    // have an exception on the cx, otherwise we'll try to wrap it and bork in
> +    // the wrap callbacks on a null global.
> +    JS_ASSERT(!cx->isExceptionPending());

In general, one should not be calling JSAPIs with an exception pending (it tends to indicate a bug) so I'd be fine w/o the comment.

Comment 6

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=064cf29170ea

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 781895 [details] [diff] [review]
Part 2 - Properly squelch exceptions in FormatFrame. v1

Nice

Comment 8

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=73e85b06c7c6

Comment 9

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3a974ffb8cf9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ef5f21a8fba1

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3a974ffb8cf9
https://hg.mozilla.org/mozilla-central/rev/ef5f21a8fba1