1227190 - Figure out how debugger is expected to interact with dontReportUncaught

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

There is a fundamental disconnect between the execution model bug 981187 is trying to create (engine never reports anything, embedding is responsible for all exception reporting) and the way debugger wants to work as described in bug 1172246 comment 22 (exceptions from debugger callbacks must be reported and then cleared so they don't affect debuggee execution).

This disconnect is somewhat artificial in the sense that it's only a problem because debugger is inside the engine, not the embedding.  Some theoretically possible solutions to the problem, none terribly great:

1)  Change debugger to not be part of the engine so it can use AutoJSAPI around
    its calls into the callbacks.  I doubt this is terribly viable.
2)  Move AutoJSAPI into the engine somehow... but the problem is that what
    debugger really wants to do is reset the error reporter around its nested
    script invocation and an engine-internal AutoJSAPI would know nothing about
    error reporters.
3)  Add a way for debugger to notify the embedding before/after it runs the
    callback, so the embedding can swap out the error reporter.
4)  Add a way to detect, in the error reporter, that the error report is coming
    from debugger so we can skip the assert and just let it through.  This could
    be built on top of callbacks as in #3, or via explicit flags on the
    JSErrorReport or something.

Other bright ideas?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Bobby pointed out that we have ScriptEnvironmentPreparer, which might work here.  I'm going to see if I can make it so.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Some notes to self:  The debugger code is weirdly interwingled in terms of its compartment handling.  In particular, it enters the debugger compartment, does some stuff, then in various places (I'm looking at parseResumptionValue and handleUncaughtException) exits the debugger compartment manually, instead of just unwinding to the AutoCompartment on the stack.

I guess there's a good reason to do that in parseResumptionValue because it's writing outparams...

The upshot is that if we want to use ScriptEnvironmentPreparer we need to make sure to do so with our current compartment on entry, not the debugger compartment.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Talked to jorendorff a bit.

The current setup, and the modification to use ScriptEnvironmentPreparer are both moderately broken in that they fire onerror and the like for debugger errors.

So new possible plan, in order:

1)  Introduce a new runtime callbacks that takes a JSContext and a HandleValue for the
    debugger to use for error reporting.
2)  Change Gecko so it sets up something for this callback.  In practice the something
    would look like the guts of AutoJSAPI::ReportException; I'll just refactor that code a
    bit.
3)  Change Debugger::handleUncaughtExceptionHelper to use this new callback instead of
    JS_ReportPendingException, suppressing the exception altogether if the callback is not
    set on the runtime.
4)  Change JSErrorReport flags to have a "from debugger" flag.
5)  Change the Gecko callback to set that flag on the JSErrorReport in the code from
    step 2.
6)  Change the actual error reporting infrastructure in Gecko to skip all the event firing
    if the "from debugger" flag is set, basically by following the same codepaths as
    JSREPORT_IS_WARNING.

Some questions:

* Is there really a need for the "from debugger" flag, or should we just set
  JSREPORT_WARNING in the debugger case?  This affects the actual formatting/presentation
  of the reports, I think.

* Is the result of the above something that matches what the devtools team wants to happen
  for debugger exceptions?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Bobby had another suggestion, which was to skip new runtime callbacks and basically change Debugger::handleUncaughtExceptionHelper to do the following, in pseudocode:

  if (exception pending) {
    steal exception into stack Rooted;
    PrepareScriptEnvironmentAndInvoke() a callback which just does JS_SetPendingException.
  }

This won't have the onerror problem I mention above (just like the current code hopefully doesn't, actually), for mainthread, because we would not find a window from the AES to fire on.  But on workers, we would need some additional work similar to what's described in comment 3.  All the worker stuff basically assumes that anything coming through is from the worker itself (not a bad assumption until worker debugging got added), so even getting it to not show up in the web page console will be a bit of a PITA.  :(  But we have that PITA with the approach from comment 3 too, so it's not additional pain.

Jason, opinions on doing things this way?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, and Eddy points out that in _some_ cases on workers we do the right thing: if aFireAtScope then we do something where we call ReportErrorToDebugger instead of doing the normal thing.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I filed bug 1227639 on the worker debugger bits.

Comment 7

[Jim Blandy :jimb]

Talking with bz on IRC, it seems that one sticking point is the call to JS_ReportPendingException from Debugger::handleUncaughtExceptionHelper here:

https://hg.mozilla.org/mozilla-central/file/d1cae7deae1a/js/src/vm/Debugger.cpp#l996

That is meant to serve as a third-level backstop, simply because it's a horrible thing to ever swallow an error message. We can satisfy that goal any way that makes sense for us; using JS_ReportPendingException there is not essential to the Debugger architecture at all.

Error handling in Debugger-based tools is supposed to work like this:

1) Debugger callbacks aren't supposed to throw in the first place; they're supposed to handle their errors in a way appropriate to that callback.

2) If a callback does throw, the tool is supposed to have set the uncaughtExceptionHook on their Debugger, to manage the tool's failure in some tool-appropriate fashion. Docs for that hook: https://hg.mozilla.org/mozilla-central/file/d1cae7deae1a/js/src/doc/Debugger/Debugger.md#l58

3) If even that doesn't work, then we simply call JS_ReportPendingException because we couldn't think of anything else to do.

In that conversation, I gathered that the weakness here was that embeddings shouldn't have to trust every Debugger-based tool to get this right, and need to be able to set JSRuntime-wide policies for how these errors get handled; but JS_ReportPendingException doesn't give the embedding the information or flexibility it needs to get this right.

As far as I'm concerned, that call to JS_ReportPendingException in Debugger::handleUncaughtExceptionHelper can be changed to do anything that is useful to embeddings. All I ask: please please please don't make us eat error messages! I don't want that kind of blood on my hands.</overdramatic>

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Make debugger error reporting play nice with the embedding taking ownership of error reporting

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 8691554 [details] [diff] [review]
Make debugger error reporting play nice with the embedding taking ownership of error reporting

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

::: js/src/vm/Debugger.cpp
@@ +978,5 @@
> +namespace {
> +class MOZ_STACK_CLASS ReportExceptionClosure : public ScriptEnvironmentPreparer::Closure
> +{
> +public:
> +    ReportExceptionClosure(RootedValue& exn)

explicit

@@ +986,5 @@
> +
> +    bool operator()(JSContext* cx) override
> +    {
> +        cx->setPendingException(exn_);
> +        return true;

This is weird. It's not OK to set an exception pending and then return true, so much so that we assert about it in some places. Does it work correctly if we return false here? That's definitely what it should be doing.

The code in js::PrepareScriptEnvironmentAndInvoke, similarly, strikes me as weird in the same way: it's not following error handling conventions, and there doesn't seem to be any reason why not...

Here's what it currently says (not part of your patch):
>     JSAutoCompartment ac(cx, scope);
>     bool ok = closure(cx);
> 
>     // NB: This does not affect Gecko, which has a prepareScriptEnvironment
>     // callback.
>     if (JS_IsExceptionPending(cx)) {
>         JS_ReportPendingException(cx);
>     }
> 
>     return ok;

This code should:

1.  MOZ_ASSERT(!cx->isExceptionPending()) on entry;
2.  after calling invoke(), MOZ_ASSERT_IF(ok, !cx->isExceptionPending());
3.  perform error handling only if (!ok), like all other code in our codebase;
4.  after the error-handling if-statement, MOZ_ASSERT(!cx->isExceptionPending());
5.  that being the case, unconditionally return true.

(Given #5, it would be OK to change the function to have void return type, unless we think cases like OOM and timeouts *should* be propagated up from here.)

Please fix it (I can rs) or get the initial author or reviewer on the hook to fix it?

@@ +1027,5 @@
> +                ReportExceptionClosure reportExn(exn);
> +                DebugOnly<bool> ok = PrepareScriptEnvironmentAndInvoke(cx->runtime(),
> +                                                                       cx->global(),
> +                                                                       reportExn);
> +                MOZ_ASSERT(ok, "Should never fail");

(With the changes I proposed, this still always returns true, so this assertion is fine even though ReportExceptionClosure() always fails. But that is only if you do all the changes in the same patch. If not, think it through...)

Could use MOZ_ALWAYS_TRUE instead of DebugOnly<bool>, your call.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Does it work correctly if we return false here?

The only thing the return value here controls is the return value of the PrepareScriptEnvironmentAndInvoke call.

So we _could_ return false here, but then we'd also need to assert that PrepareScriptEnvironmentAndInvoke() returned false... and then just ignore that false return.  I think that would look moderately weird too.

If we want to change how PrepareScriptEnvironmentAndInvoke() behaves, we could of course do that.  We'd need to also change the way the embedder callback works.

But really, the boolean return value here is really meant as some sort of "did the callback actually succeed" indicator.  Since we explicitly catch and report exceptions thrown by the callback, it doesn't indicate that there is a pending exception or anything, just whether the callback succeeded at what it was trying to do.  Maybe there's no value in communicating this and we should make the function void, as you say...

Comment 11

[Boris Zbarsky [:bzbarsky]]

I suppose if we made it void, callbacks that care could just stash their "I failed" state inside themselves.... The one consumer we have so far explicitly ignores the return value of PrepareScriptEnvironmentAndInvoke.

Comment 12

[Jason Orendorff [:jorendorff]]

(In reply to Boris Zbarsky [:bz] from comment #10)
> > Does it work correctly if we return false here?
> 
> The only thing the return value here controls is the return value of the
> PrepareScriptEnvironmentAndInvoke call.

OK, but why is that the right behavior? What's wrong with the usual success-bool conventions we use throughout the engine and the JS API?

At the least it should be documented...

> But really, the boolean return value here is really meant as some sort of
> "did the callback actually succeed" indicator.  Since we explicitly catch
> and report exceptions thrown by the callback, it doesn't indicate that there
> is a pending exception or anything, just whether the callback succeeded at
> what it was trying to do.

Huh. Well... I don't know why that's a good thing, since we expect this callback to mostly be doing JSAPI calls, right? In fact, the sole existing callback CClosure::ArgClosure::operator() seems to return false exactly according to JSAPI conventions, except that the `if (!success)` case does not clear the pending exception, which is another bug in my view.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. change PrepareScriptEnvironmentAndInvoke to return void, not bool, to make it clearer that it reports exceptions for you

Comment 14

[Jason Orendorff [:jorendorff]]

Comment on attachment 8694355 [details] [diff] [review]
part 1.  change PrepareScriptEnvironmentAndInvoke to return void, not bool, to make it clearer that it reports exceptions for you

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

Thank you. Great.

::: js/src/ctypes/CTypes.cpp
@@ +7002,2 @@
>      }
> +    return false;

Yes! I had to sleep on it to figure out why this was correct, even though we totally discussed it through and I'm supposed to know what I'm doing. So please comment in the `if (cinfo->errResult)` branch explaining why it is correct to set the C return value here and also return false.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1332,5 @@
>  XPCJSRuntime::EnvironmentPreparer::invoke(HandleObject scope, js::ScriptEnvironmentPreparer::Closure& closure)
>  {
>      MOZ_ASSERT(NS_IsMainThread());
>      nsIGlobalObject* global = NativeGlobal(scope);
> +    // Not much we can do if we simply don't have a usable global here...

Style nit: blank line before this comment, please.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f84bb42a723
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ebb551d8db2

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e33a92988cd

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3ae43ee51fa

Comment 18

[Ryan VanderMeulen [:RyanVM]]

(Commenting on KWierso's behalf since he's having BMO issues)

Backed out for causing jit-test assertions.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b3cf9f8e175

https://treeherder.mozilla.org/logviewer.html#?job_id=18166522&repo=mozilla-inbound

Comment 19

[Boris Zbarsky [:bzbarsky]]

OK, so there are a few fun things going on at this point.

First, PrepareScriptEnvironmentAndInvoke asserts that if there is no preparer set up on the runtime then rt->contextList.getFirst() == rt->contextList.getLast().

Second, shell has a way to evaluate code on a new JSContext (the { newContext: true } option to evaluate()).

Third, some tests use that and invoke the debugger in a script evaluated in such a way and then have a debugger callback they throw from.  For example, js/src/jit-test/tests/debug/bug1161332.js

How do people feel about me just removing this assert from PrepareScriptEnvironmentAndInvoke and running it on the first context in the runtime even if there are multiple contexts?

The other option is to allow passing in a JSContext to use, and simply ignoring it in the case when there is an environment preparer...

Comment 20

[Boris Zbarsky [:bzbarsky]]

Or I guess option 3: bholley suggests nixing the newContext option in shell altogether.  Won't necessarily help real-life consumers.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Change PrepareScriptEnvironmentAndInvoke to take a JSContext*, not a JSRuntime*

Comment 22

[Jason Orendorff [:jorendorff]]

Comment on attachment 8695067 [details] [diff] [review]
part 2.  Change PrepareScriptEnvironmentAndInvoke to take a JSContext*, not a JSRuntime*

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

::: js/src/ctypes/CTypes.cpp
@@ +6932,5 @@
>    ArgClosure argClosure(cif, result, args, static_cast<ClosureInfo*>(userData));
>    JSRuntime* rt = argClosure.cinfo->rt;
>    RootedObject fun(rt, argClosure.cinfo->jsfnObj);
> +  MOZ_ASSERT(rt->scriptEnvironmentPreparer ||
> +             rt->contextList.getFirst() == rt->contextList.getLast());

As discussed, you can drop this assertion.

Please add a comment here:

// Arbitrarily choose a cx in which to run this code. This is bad, as JSContexts are
// stateful and have options. The hope is to eliminate the bad state (see bug XXXXXXX).

or "to eliminate JSContexts altogether" or whatever you're trying to do... :)

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2864b6f50a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a208a3bf301
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc7580c00778

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a2864b6f50a9
https://hg.mozilla.org/mozilla-central/rev/0a208a3bf301
https://hg.mozilla.org/mozilla-central/rev/dc7580c00778