Closed Bug 1034616 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files)

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?
Flags: needinfo?(jorendorff)
Flags: needinfo?(jimb)
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.
Flags: needinfo?(jimb)
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
Flags: needinfo?(jorendorff)
(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.
(In reply to Jim Blandy :jimb from comment #3)
> That skips the rest of the test, though.

... all of it, in fact.
> 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...
Blocks: 966452
(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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Okay, I understand now. Yes, jorendorff's solution seems fine.
Attachment #8451774 - Flags: review?(jorendorff) → review+
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.
Also, looks like I need the same change to Frame-onPop-star-generators-02.js.  I went ahead and just added that.
This is fixed.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.