fixing issue where "uncaught" exceptions in promises wouldn't notify debugger

[mike-kaufman]

If an exception was raised inside a promise and the promise didn't have any rejection handlers, we wouldn't notify the debugger that an "unhandled" exception occurred. Fixed this up and added some simple tests for it.

This should address #4630 - Throw in async function after await is not caught by debugger

Edit: This addresses the common cases for promises, but doesn't yet address async/await constructs. Will leave #4630 open for that.

[MSLaguana]

This is true for "second chance" exceptions, not "first chance" exceptions, right? First chance exceptions are brought to the debugger at soon as any exception is thrown, while second chance is for unhandled exceptions, I believe?

[MSLaguana]

This is a perverse case, but what about this scenario:

var p = Promise.resolve(0).then(() => {
  p.catch(() => {}); // lazily added catch on the currently executing promise
  throw "Am I uncaught?";
});

[MSLaguana]

It also looks to me like this will blame the exception after the fact:

var p = Promise.resolve(0).then(() => {
    throw "foo";
}).then(() => { console.log("not executed, but no catch handler") });

Running that snippet, won't it initially think that there is a catch handler since the second then installs both (but a stub for the error case), and then when the error is passed through by the second then, this code would register as an uncaught exception at that time?

[mike-kaufman]

I'll add a test for this. I think in this case when the throw happens, there will be handler, so we wouldn't want to raise for "uncaught exceptions.

[MSLaguana]

This is where a lot of the discussion in #4630 went :) The hope being that when you are trying to inspect uncaught execptions, you want to see as much of the state at the point where the exception was thrown (to work out what's going on), while here we're already in a completely different async stack.

[MSLaguana]

FYI, part of the issue here was determining exactly what was intended. E.g.

var p = /* some promise */
p.then(something).catch(handle);
p.then(branchingPath); // No catch

If p is rejected, is that "handled"? There is a catch, down one branch, but not on the other. Does it become unhandled only at the leaf of that other path? But then we're back to not having relevant context around so breaking on unhandled exceptions is less useful.

[fatcerberus]

How were you able to implement this correctly for async functions? When I attempted a fix here, I found that at the time of the throw, the awaited promise had no handlers attached to it and so any rejection of an async function was always detected as second-chance regardless of attached .catch()es.

[mike-kaufman]

This is true for "second chance" exceptions, not "first chance" exceptions, right? First chance exceptions are brought to the debugger at soon as any exception is thrown, while second chance is for unhandled exceptions, I believe?

Yes, this is the case. I'll add some tests for exceptions inside promises to validate we break correctly on "first chance" exceptions as well.

[mike-kaufman]

How were you able to implement this correctly for async functions? When I attempted a fix here, I found that at the time of the throw, the awaited promise had no handlers attached to it and so any rejection of an async function was always detected as second-chance regardless of attached .catch()es.

@fatcerberus - I didn't notice this, but I'll look into it. I need to add some more tests for async/await as well here.

[fatcerberus]

@MSLaguana It’s true, promises are interesting in that you can have branching exception handling paths; there is no analogue for this in synchronous code so the question of what to do in that case is a tricky one. FWIW I would intuitively expect any case that leads to a HostPromiseRejectionTracker callback to be considered “unhandled” in the debugger, but I also acknowledge that detecting the case you describe is nontrivial. :(

[MSLaguana]

It looks like chrome doesn't have a perfect solution to this either, they must have some heuristics:

async function foo() {
  throw "err";
}

async function bar() {
  try {
    var p = foo();
  } catch (e) {
    console.log("caught " + e);
  }
}
bar();

This doesn't break the debugger if you break on uncaught exceptions, but it does end up printing that there is an uncaught promise rejection.

[fatcerberus]

That IS an uncaught rejection, is it not? You’re not doing anything with the foo() promise so its a bit like the branching case above, I think.

[MSLaguana]

Yes it is, but it doesn't trigger the "pause on uncaught exceptions" handling as it should. If you remove the (essentially useless here) try/catch then it will trigger the "pause on uncaught exceptions" behavior

[MSLaguana]

To be a bit clearer,

async function pauses() {
  var p = Promise.reject("err");
}

async function doesNotPause() {
  try {
    var p = Promise.reject('err');
  } catch () {}
}

the first function will trigger "pause on uncaught exceptions" in chrome, the second will not.

[MSLaguana]

… And the same is true if you convert these into standard functions rather than async functions.

[MSLaguana]

It also looks like in the forking-promise case chrome will only break for an "uncaught" error if it has no handler, but if there is any catch for the promise then it won't break at the throw, while it does report an unhandled rejection later on:

var p = Promise.resolve(0).then(() => { throw "err";});
// as-is, no break-on-uncaught
// remove the first line, it does. 
p.then(() => {}, () => {});
p.then(() => {}).then(() => {});

[mike-kaufman]

Update here:

Promises:

• I added some code that will do a depth-first-search across the set of promise reject reactions, and identify any promises that have been "then'd" and have the default rejection handler. This seems to correctly identify any paths in the promise tree that will result in unhandled rejections at the time we do the search. It obviously doesn't account for any catch handlers added later.
• Added a bunch of test cases for promises.
• Only problem I've found with current implementation is this code:

var p = Promise.resolve(0).then(function f() {
  p.catch(() => {}); // lazily added catch on the currently executing promise
  throw "Am I uncaught?";
});

• In case above, the issue is that when we invoke f(), there are no handlers attached, but when the throw happens, there is a handler attached. This felt like an edge case and it seemed much more complex to move the "are we in a promise & will this result in an unhandled rejection" logic later, so I'm leaving this as-is.

async functions:

• Still need to do more work here & add some more tests.
• currently we think all rejections are uhandled at the time of the throw. Hoping to find a work-around to this.

[fatcerberus]

[async functions:] currently we think all rejections are uhandled at the time of the throw. Hoping to find a work-around to this.

Yes, this was the problem I ran into. If you try to follow the chain of promise reactions from that point you hit a wall because, for whatever reason, there are no reactions attached to the promise being rejected even if there's been a .catch() attached to the async function's promise in the meantime.

I suspect this might have to do with how await is specified in ECMAScript (assuming that we are compliant here)--the handler that resumes the generator is attached after the fact.

[MSLaguana]

Might want to add && !willBeUnhandled to early-terminate this outer loop when we already know the promise is unhandled.

[MSLaguana]

Or actually just return true in the inner loop rather than setting a flag and breaking out

[mike-kaufman]

We break out & my personal preference is a single return point is easier to read/reason about, vs multiple return points.

[MSLaguana]

Do you know whether the thrower function is marshalled between contexts correctly to make sure this comparison returns true even if I add thens in other contexts?

[mike-kaufman]

Do you know what the JS looks like to create multiple script contexts?

I spent some time digging through what happens here. The default "thrower function" in EntryThen, and we get that from the EntryThen's script context - so assuming default then method is called on some promise p, p's scriptContext + javascriptLibrary are the same EntryThens.

Now, it's possible that someone could change the then method on a promise to a function from another context, and in that case, things would be off, but I don't know that we care about supporting those cases.

[MSLaguana]

Depends on the host; in ch I think it's something like var crossSiteFunc = WScript.LoadScriptFile('someScript.js', 'samethread');, where you want someScript.js to define a function that is then exposed to the original context. In node it's simpler, using var vm = require('vm'); var ctx = {}; vm.createContext(ctx); vm.runInContext(evalString, ctx)

[mike-kaufman]

OK, updated logic to check equality on the underlying FunctionInfo to see if it is the default thrower function. Also added some test cases where the promise & the then function are from different script contexts.

[mike-kaufman]

squashed & rebased

[MSLaguana]

Minor typo here, Unexecpted