1054055 - Make "ignore caught exceptions work" with C++ exceptions

Status: NEW

(DevTools :: Debugger, enhancement, P5)

Description

[James Long (:jlongster)]

I think it was in bug 966452 that we made it so that promises that are GCed but have an unhandled rejection will report that error onto the console. I have verified this behavior in nightly with the following code:

(function() {
  var p = new Promise(function(resolve, reject) {
    foo();
    resolve('hello!');
  });
})();

After ~5 seconds, I see an error in the console about foo not being defined. Yay.

The problem is I'm not sure it should be a real error. I liked that at first, but fitzgen made me aware that it's confusing because it's not an exception that comes from normal code. That means that interactions with "pause on exceptions" and "ignore caught exceptions" in the debugger are ill-defined, and is confusing to the programmer.

I tried "pause on exceptions" and it actually worked. It paused (it didn't seem to wait 5 seconds either so not sure if it was actually waiting on GC or if something else is going on...). However, the "ignore caught exceptions" is completely ignored now. That's what is weird; in promises the exception is *always* caught, the question is just if the programmer handled it or not.

Take this code:

(function() {
  var p = new Promise(function(resolve, reject) {
    foo();
    resolve('hello!');
  }).then(
    function() {},
    function(err) {
      console.log('caught', err);
    }
  );
})();

Now we have an error handler, so even though we "caught" the exception the debugger pauses when `foo` is called. We have different notions of caught/uncaught here so it's confusing.

I say we display the error more as a warning and print the error with a stack trace.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I believe this is because we rely on Debugger.Script.prototype.isInCatchScope to ignore caught exceptions. We need a way to determine if the promise internals are on the stack or something. Not completely clear to me.

Comment 2

[:Paolo Amadini]

I also think unhandled rejections should be displayed differently, as they generally indicate a missing rejection handler. Most promise chains should end with a handler that explicitly reports the error.

If you try the same code with Promise.jsm promises, you'll see what we currently display there. Note that there might be a third way to report those errors that is better than both the current reporting and the Promise.jsm one.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Fundamentally, uncaught exceptions and unhandled rejections are the same thing; the latter are just async exceptions.

One important difference is that for exceptions you can kinda sorta know at throw time whether it's caught but for a rejection you don't know whether it's handled until sometime later.

Regardless of the exact string we use for reporting promise rejections (and I would be happy to communicate to the console information about the thing that's being reported being a promise rejection, not an exception), it seems like we should decide how promise rejections, whether due to thrown exceptions or not, should interact with the break-on-exception stuff in debugger, right?

In any case, the current "automatically report uncaught exceptions" mechanism in the JS engine is slated to go away.  See bug 981187.  We should figure out what exactly it means to "break only on uncaught exception" in that world, where we may not even know whether it's caught until we've unwound the stack.  How do we currently handle conditional catch clauses?  Those seem closest to this issue in some ways...

Comment 4

[Boris Zbarsky [:bzbarsky]]

So to be precise, I think this bug is conflating two issues:

1)  What should happen with promise rejection reporting?

2)  What is the right interaction of "ignore caught exception" and situations where the
    stack contains C++?

As an illustration of #2, consider the following testcase:

  function f() { failbecauseIamnotdefined(); }
  var t = document.addEventListener("foo", f);
  try {
    document.dispatchEvent(new Event("foo"));
  } catch (e) {}

This does not stop on the exception if I ask the debugger to stop only on uncaught exceptions, even though the exception is very much uncaught for all practical purposes: it's reported to the console and all that.  This is because the debugger seems to assume some sort of lexical scoping for try/catch (which is kinda true) and that C++ never reports exceptions (which is just not true at all).

The Promise situation is the obverse of this testcase: there the C++ is catching the exception, but the debugger thinks it's uncaught.

I'm going to focus this bug on #2, because I think that's an actual debugger issue, whereas reporting of promise rejections is more of a console issue.

Comment 5

[James Long (:jlongster)]

Great! I don't have strong opinions on what the behavior should be, as long as things are consistent. I'm not familiar with any of the code that deals with uncaught exceptions so I can't really answer your questions about how we deal with conditional catches.

Comment 6

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

FWIW this is a known limitation from the implementation in bug 906963, which aimed at an "80% solution". Conditional exceptions work, presumably because try notes take them into account. It would be great to properly handle the other 20% though!

onExceptionUnwind is where the debugger backend decides whether an exception is uncaught or not:

http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#2196

It just calls DebuggerScript_isInCatchScope for every frame:

http://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp#3906

Comment 7

[Jason Laster [:jlast]]

Honza, could you look into this?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Note that what we may need from Gecko here is to say, before every call into JS, whether it plans to report the exception or rethrow it.

For extra fun, in some cases Gecko doesn't know this until it's had a chance to examine the exception value. :(

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I tested this and the original description in comment #0 is still valid.

Honza