1034616 - Frame-onPop-generators-02.js depends on js_ReportUncaughtException sometimes just swallowing the exception instead

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Consider js/src/jit-test/tests/debug/Frame-onPop-generators-02.js

This test does

        throw "fit";

insdide a debugger callback.  The caller then ends up calling js::Debugger::handleUncaughtExceptionHelper which calls JS_ReportPendingException, which calls js_ReportUncaughtException.

The thrown thing is not an error, so we get no JSErrorReport, so we end up taking the JS_ReportErrorNumber.  But that lands us in ReportError, which does this:

    if (!JS_IsRunning(cx) || !js_ErrorToException(cx, message, reportp, callback, userRef)) {
        if (message)
            CallErrorReporter(cx, message, reportp);

and since we're in fact running script on cx (the script we're debugging) we end up doing js_ErrorToException and set a pending exception on the cx instead of calling the error reporter.  Then we go ahead and unwind to js_ReportUncaughtException which just goes and does JS_ClearPendingException (this is after its JS_ReportErrorNumber call).  So this exception is never reported.  In particular, if you run the test there is no console output.

If I change the test to throw a |new Error()| instead of a string, or if I check in the patch in bug 966452, which actually makes js_ReportUncaughtException always report, this test starts failing, because it starts printing an uncaght exception message.  What is the right way to fix this test?

Comment 1

[Jim Blandy :jimb]

Attachment: In some JS Debugger tests, don't assume that non-Error instances thrown from debugger hooks get eaten.

Something like the attached should be right. I don't know what the exception value looks like by the time that top level sees it; perhaps 'assertThrowsValue(..., "fit");' is what's needed?  But it's going to be something like that.

Comment 2

[Jason Orendorff [:jorendorff]]

The exception here is caught and reported; it isn't thrown to the calling script.

bz, adding this line at the top of the test worked for me:

// |jit-test| error: fit

Comment 3

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #2)
> The exception here is caught and reported; it isn't thrown to the calling
> script.
> 
> bz, adding this line at the top of the test worked for me:
> 
> // |jit-test| error: fit

That skips the rest of the test, though.

Comment 4

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #3)
> That skips the rest of the test, though.

... all of it, in fact.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I don't know what the exception value looks like by the time that top level sees it

Top level never sees it.  Various debugger functions have the general structure where they do:

  return handleUncaughtException(stuff);

This will report the exception before returning...

Comment 6

[Jason Orendorff [:jorendorff]]

(In reply to Jim Blandy :jimb from comment #3)
> > // |jit-test| error: fit
> 
> That skips the rest of the test, though.

Normally, but not in this case. The debugger calls Debugger::handleUncaughtException, and since there is no uncaughtExceptionHook installed, it just does:

    JS_ReportPendingException(cx);
    cx->clearPendingException();
    ...
    return JSTRAP_ERROR;

So this is propagated to the debuggee as an uncatchable error. But the way the test is written, after unwinding, control returns to the debugger, which receives null as the completion value. So the rest of the test does run.

So why is there output about an uncaught exception on stderr? It's because JS_ReportPendingException called the shell's JSErrorHandler, which prints to stderr. We could fix this either by adding a do-nothing uncaughtExceptionHook, or by instructing the test harness to ignore the output.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix Frame-onPop-generators-02.js test to pass even if we start reporting the exceptions we're supposed to be reporting but fail to report right now

Comment 8

[Jim Blandy :jimb]

Okay, I understand now. Yes, jorendorff's solution seems fine.

Comment 9

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f0e44cd5fbd

Comment 10

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=43505203&tree=Mozilla-Inbound

Comment 11

[Boris Zbarsky [:bzbarsky]]

Argh.  It looks like the "error" annotation _requires_ the test to produce an error to pass.  So I'll need to land this in a single push with bug 966452.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Also, looks like I need the same change to Frame-onPop-star-generators-02.js.  I went ahead and just added that.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I checked this in as part of https://hg.mozilla.org/integration/mozilla-inbound/rev/a2f5fa870c8a

Comment 14

[Boris Zbarsky [:bzbarsky]]

This is fixed.