Fix debugger ignoring exceptions thrown after an await

[fatcerberus]

If code such as the snippet below is run under the debugger, the host is not notified of the thrown exception via JsDiagDebugEventRuntimeException:

(async () => {
    await null;
    throw new Error();
})();

Note that moving the throw before the await makes the debugger able to intercept it again. This issue occurs because of an AutoCatchHandlerExists being in scope when the promise reaction is run:
https://github.com/Microsoft/ChakraCore/blob/ad79be8811806622190b55266c73f20b600ed46c/lib/Runtime/Library/JavascriptPromise.cpp#L1037-L1051

The fix is to not construct an AutoCatchHandlerExists in this situation. While the exception is indeed caught and squelched, it's not actually swallowed but converted to a promise rejection, which may well be fatal depending on the host. We need the debugger to be able to intercept it before that happens so the user can view the callstack, variables, etc. before the stack is unwound.

There is no test coverage for this yet, I will try to add some in the next day or so.

Fixes #4630.

[fatcerberus]

This is currently targeting master. Should I retarget it to 1.8 or 1.9 or leave it as-is?

[boingoing]

Sounds reasonable to me. Looks like we get the same unexpected behavior in Edge.

I think master is the right branch to target for this one.

Not sure exactly what we accomplish by having AutoCatchHandlerExists here in the first place. I'll defer to someone who does. @akroshg maybe?

Please do add a couple of unit tests for this covering both scenarios in the change and thank you for contributing. 👍

[rhuanjl]

I think this needs some special casing/more thought.

The current behaviour means too few break points. This change in my opinion means too many.

Notably with this change (per having tried it out) you get break points for exceptions within promise reactions even if they have a .catch() on the end of them.

[jackhorton]

I know @kfarnung has been working with JsDiag recently, too

[agarwal-sandeep]

@rhuanjl is right. First chance exception should only be reported if there is no catch handler.

[fatcerberus]

@agarwal-sandeep I assume you mean second-chance. First-chance (if enabled) triggers for any exception, even if caught.

[agarwal-sandeep]

Sorry. yes I meant second chance.

[fatcerberus]

Alright, I’ll see if I can find a way to fix the case above while avoiding false positives. I think the only way to get 100% correct behavior will be to walk the promise chain looking for a .catch().

[fatcerberus]

The key thing we want to ensure is that the stack isn’t allowed to unwind before a potentially fatal error occurs (note the host may induce a crash on an unhandled rejection) without the debugger seeing it, even if first-chance exceptions are not enabled.

[fatcerberus]

Could somebody a bit more familiar with the ES specification explain to me what the difference between a Promise Reaction Job and a Resolve Thenable Job is? They seem to do the same thing, i.e. calling the continuation passed to .then().

[fatcerberus]

Okay so here's the challenge: We need a way to ensure this is treated as a second-chance exception:

(async () => {
    await null;
    throw new Error();
})();

Without causing false positives for, e.g. this case:

(async () => {
    await null;
    throw new Error();
})().catch(() => {});

[MSLaguana]

One problem is that it isn't completely clear to me what behavior we want here. I agree with your case there, but what about the slightly different case of

(async () => {
    await null;
    throw new Error();
})().then(() => {});

Should that be considered an unhandled execption? And is it unhandled at the point of the throw, or at the point of the then promise being rejected with no handler?

[fatcerberus]

Arguing that it’s second-chance exception only after a chain of .then()s is akin to arguing that an uncaught exception shouldn’t be reported to the debugger yet because there’s another line of code after it. You do that and now your stack frame has unwound and the breakpoint is worthless. The idea is that if there’s no handler to catch the rejection in question right now, then you want to get a breakpoint before the throw so that you don’t lose the stack frame it was thrown from and can examine locals, etc.

If the error turns out not to be fatal then it’s a false positive but ultimately harmless—just send a Resume command and execution continues normally.

The thing is, it’s tricky to know if a promise is unhandled without walking the entire promise chain.

[fatcerberus]

The above being said, I would agree the change needs more thought/discussion. I’ll hold off on making further changes pending input from others.

[MSLaguana]

Walking the promise chain is also not super clear to me, since you can have a case like

var p = (async () => {
    await null;
    throw new Error();
})();

p.then(() => {}).catch(() => {});
p.then(() => {});

My best intuition for that case is it should be considered handled since the error does get caught and examined at some point, but this is something that doesn't have a good equivalent in synchronous error throwing scenarios.

The other behavior that I could see some argument for would be that all chains must have an error handler, and so the above case would instead be considered eligible for a second chance exception. This seems less useful to me though since as soon as you have any branches on a promise chain, even if your "main" logic deals with errors properly, you would start getting false positives.

[fatcerberus]

@MSLaguana I was actually discussing that very case with @rhuanjl in private earlier. Given a host which implements HostPromiseRejectionTracker as crash-on-unhandled-rejection (as my own miniSphere does, and Node will eventually do), that case would create an unhandled rejection notice and crash the process. As such I would definitely call that a second-chance exception.

[rhuanjl]

I think any attempt to walk a promise chain could be quite expensive. And in the case of a branching one rather painful to implement.

I thought I'd look up what Chromium's devtools does for this: https://bugs.chromium.org/p/chromium/issues/detail?id=465666

They have an explicit "Pause on promise rejections" option. If selected it breaks for all promise rejections. If not selected it behaves the way CC does at the moment.

[fatcerberus]

@rhuanjl Keep in mind though, this is in an exception path, which are expected to be, well, exceptional. And performance is secondary under the debugger anyway. As long as it doesn’t take 10 seconds to walk the promise chain each time something is thrown then I don’t think that’ll be an issue in practice.

[fatcerberus]

So, interesting (and annoying) thing I just found: For an await continuation, at the time the ReactionTaskFunction runs, the promise controlled by it has nothing chained to it. Both promise->resolveReactions and promise->rejectReactions have length zero in this case, making it completely impossible to walk the chain to find the .catch() tacked onto the async call.

I believe the behavior described above is per spec for await completions, which leaves us with a difficult choice: We can either never trigger a second-chance exception for throw in an async function (the current behavior), or alternatively treat any unguarded exception (i.e. no syntactic try {} catch {}) in an async function as second-chance (i.e. this pull).

Personally I'm willing to accept a few false positives for promise chains with a manual .catch() over pervasive false negatives for awaited async functions any day.

I note that enabling first-chance exceptions allows even the case in the OP to be caught, contrary to my initial observations. However in large codebases, it's been my experience that enabling break-on-first-chance tends to generate far too much noise to be useful.

[dilijev]

@boingoing @MSLaguana can one of you adopt this PR?

[MSLaguana]

My understanding is that there's no consensus on whether this is the right behavior to adopt; perhaps @liminzhu can weigh in with an opinion?

[fatcerberus]

For what it's worth, I don't consider the current behavior "the right behavior" either--so it's IMO really a question of which is the lesser of two evils.

To be clear, this is never a second-chance exception:

async function f() {
    try {
        await null;
        throw new Error();
    }
    catch(e) {}
}
f();

However, the below is second-chance (with the change here) because the .catch() simply isn't visible through the promise chain at the time the error is thrown (the handler that resumes the async function doesn't get attached until after the await resolves, per the specification):

async function f() {
    await null;
    throw new Error();
}
f().catch(() => {});

[agarwal-sandeep]

I suggest we start with same approach as Chrome.
Add new enum value JsDiagBreakOnAllPromiseRejection to JsDiagBreakOnExceptionAttributes and if that's set always break on rejection.

Later if we decide to support just unhandled rejection we can enable it by adding JsDiagBreakOnUncaughtPromiseRejection.

[fatcerberus]

JsDiagBreakOnAllPromiseRejection

At what point would such a breakpoint be triggered? The whole thing is that I really want to preserve the stack frame the error was thrown from in cases like this:

async function f() {
    let something = 812;
    await null;
    throw new Error();  // want breakpoint here to view local variables
}
f();

It's my understanding that the actual promise rejection happens sometime after f() has already unwound, which defeats the purpose of the breakpoint.

[agarwal-sandeep]

JavascriptExceptionOperators::ThrowExceptionObject
JavascriptExceptionOperators::ThrowExceptionObjectInternal
JavascriptExceptionOperators::DispatchExceptionToDebugger
ProbeContainer::HasAllowedForException

HasAllowedForException determines when to report the exception to debugger, we need to add logic there to see if promise rejection needs to be reported.

Currently that logic is based on whether there is a catch handler and if the exception is from user code.
The AutoCatchHandlerExists in EntryReactionTaskFunction and EntryResolveThenableTaskFunction does check for user code, may be we need to add more logic there?

[liminzhu]

If I'm reading the spec right (cc @bterlson in case I'm wrong), unhandled promise rejections are handled by host-defined HostPromiseRejectionTracker, which quote "must complete normally in all cases" and sounds like it can't crash the process. This leads me to believe the current behavior is right b/c your code would just run to completion likely with a warning or w/e the host decides to do. If a host wants to completely ignore unhandled rejections, that's also fine per spec. I'd argue to keep the current debugging API behavior as the default behavior.

That said, as a JS programmer it sucks to have my exceptions get swollen, so it's nice to have a flag as @agarwal-sandeep proposed to break on rejections. Ideally only breaking at unhandled rejections would be the best, but sounds like we can only break on all rejections without a significant amount of work?

[fatcerberus]

The HostPromiseRejectionTracker “must complete normally” meaning the handler itself can’t throw - however it’s perfectly fine to store the exception and later throw it from the event loop (which is what Node currently warns will happen in the future). Any breakpoint triggered in this case needs to happen earlier than that—at the point of throw.

[rhuanjl]

@liminzhu HostPromiseRejectionTracker itself must complete normally but it's up to the host what to do with the notification.
As an example case Node:
Node currently checks once per event loop cycle for any notifications that were not later handled if there are any it prints a warning message which says:

UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: rejection reason here
DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Note particularly the depreciation warning - there's apparently a plan for future versions of Node to exit when they encounter uncaught promise rejections.

On detecting if the rejection is handled you can do it most of the time for "normal" promises -however async/await that you basically can't do it for at all as the internal logic of their implementation means the handler is normally attached after the rejection occurs.

Additionally you can't do it for stuff like this:

let foo = Promise.reject("because");

foo.catch(()=>{/*handled gracefully*/});

(As a side note cases like this are why Node.js and the WhatW spec for handling HostPromiseRejection notifications involves storing the notifications rejected/handled and then clearing them down once per event loop tick or equivalent)

Unfortunately by the time you've waited for the script/tick/whatever to complete to see if the rejection was handled later the stack has unwound so a debugger break point becomes far less useful.

[liminzhu]

Thanks for the clarification! I think my point stands though. If it is host defined behavior, then by default the JS engine should do nothing. Nice features/other options should be provided behind flags.

[fatcerberus]

@liminzhu I think the introduction of HostPromiseRejectionTracker to the discussion muddled the issue, so to be clear: this PR isn’t about runtime behavior, but debugger behavior when an exception is thrown. As it stands the CC debugger is completely useless for debugging errors in async functions because the error is silently swallowed into a promise rejection regardless of whether there’s a handler or not. By the time the host even sees the associated promise rejection—if it even does—the entire callstack that caused the error has, by definition, already been unwound. That’s what this PR is meant to fix.

[liminzhu]

Yeah I understand this is debugging API behavior we're talking about. My main issue is that debugging behavior correlates to host behavior. If the host doesn't want to throw there, it is also debatable whether it is correct/desired behavior for the debugger to break (esp if we break on first chance). Given host behavior is out of our control and we cannot possibly have the debugging API behavior that makes everyone happy, I'd rather us do nothing by default in this case. Obviously some would want to inspect promise rejection, so we can provide some help behind a flag. Does that make sense to you?

[fatcerberus]

The main problem right now is that, assuming the host does want this breakpoint behavior, there’s no way to implement it on the host side - by the time you get the HostPromiseRejectionTracker callback, it’s already too late.

I’d be fine with a flag. As I said above, enabling first-chance exceptions already does catch the case in the OP, but that generates too many false positives so it would be nice to only break for promise rejection cases.

[fatcerberus]

I can add a flag to this PR to control the behavior. Would that be acceptable?

[liminzhu]

Sounds awesome to me :).

[fatcerberus]

Doing some further testing, I'm thinking there isn't a bug in the debugger at all but rather in the usercode/non-usercode detection. So I may close this pull and pursue that fix instead.

With first-chance exceptions disabled, the following causes the debugger to pause on the throw statement:

new Promise(() => {
	throw new Error("PIG!!!");
});

While this does not:

Promise.resolve().then(() => {
	throw new Error("PIG!!!");
});

From what I can tell, the latter is considered to have a catch in user code, while the former is not. Neither one will crash the script by default. I think we should at least be consistent here. The fact that we're not suggests the "are we in user code?" detection is going wrong somehow.

[MSLaguana]

I think that the new Promise(() => { throw new Error();}) case is actually executed synchronously and doesn't have any error handlers at the time.

[fatcerberus]

It is indeed executed synchronously, but it still results in a rejected promise and a swallowed exception, same as the .then() case. The debugger behavior should be consistent.

[fatcerberus]

I'm going to close this PR and I'll open a new one once I think through what the best way to implement this properly is. Because even in the case the user wants the breakpoint, this change results in too many false positives. For example with this change in place...

async function f1()
{
    try { await f2(); }
    catch {}
}

async function f2()
{
    await null;
    throw new Error();
}

f1();

...results in a debugger breakpoint because the try-catch is not on the stack at the time the error is thrown.