[Webkit-unassigned] [Bug 147502] Uncaught Exception in promise does not trigger break on uncaught exception breakpoint

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=147502

Devin Rousso <drousso at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |drousso at apple.com

--- Comment #1 from Devin Rousso <drousso at apple.com> ---
So I did a bunch of digging into this with Keith, and I think we've figured out why this is not working.  Here's how everything flows:

1. Setting the "Uncaught Exceptions" breakpoint changes the `m_pauseOnExceptionsState` on `JSC::Debugger`
2. When `VM::throwException` is called, we `Interpreter::notifyDebuggerOfExceptionToBeThrown`
3. This function determines `hasCatchHandler` by looking up the call stack for any CallFrame/CodeBlock that has a specified catch handler
4. Inside "PromiseOperations.js", `promiseReactionJob` is the microtask that gets executed based on the resolve/reject of a previous promise
5. Inside `promiseReactionJob`, there exists a try-catch around the actual execution of the `then` function(s)
 - As a result of this, the `Interpreter` is actually able to find a `catch`, and it therefore thinks that this exception is NOT uncaught, even though the Promise will immediately throw the same exception again if there was no `.catch()` (or second callback for a `.then()`) specified

Keith suggested that one way we'd be able to "identify" these Promise functions is to change the `this` value of these global functions in "PromiseOperations.js" to be the related Promise, meaning that we could then look at the try-catch frame and reset `hasCatchHandler` to `false` in the case that it's from one of these "PromiseOperations.js" functions.

To further complicate things, however, is that Promises have a "different" concept of an uncaught exception, since multiple `.then()` can be added to the same promise, thereby creating multiple "chains" of execution.  For example:

```
let p = new Promise((resolve, reject) => reject("test"));
p.then((x) => console.log("1", x));
p.then((x) => console.log("2a", x), (x) => console.log("2b", x));
```

In this case, we have two chains, one which is caught (1) and one which is not (2).  In this case, would the original error "test" be considered uncaught or not?  I believe that if we EVER have any path that results in an uncaught exception, then yes.  If so, then we need to consider an uncaught exception promise to be one that has a single promise somewhere in its chain that has no catch handler.

```
bool promiseHasCatch(Promise p) {
    bool hasCatch = false;
    for (Promise child of p. at promiseReactions) {
        if (!promiseHasCatch(p))
            return false;

        hasCatch = true;
    }
    return hasCatch || !!p. at onRejected;
}
```

In order to do this, however, we need to identify which promises actually have specified catch handlers, since "PromisePrototype.js" automatically creates a function for `onRejected` if one is not supplied to the `.then()`.  My "simple" fix to this is to add another private property `promiseHasCatch` to the Promise that is only set when `onRejected` is specified and not a stub.

So, in summary, Keith and I believe that in order to "fix" this issue, one would need to:
 - have a way to distinguish when a Promise's `onRejected` handler is specified or auto-generated
 - be able to recursively crawl the promise chain to see if there exist any paths that completely lack an `onRejected` handler
 - identify that the catching CallFrame/CodeBlock is one of the private global functions used by promises (I didn't check to see if they're used elsewhere)
 - retrieve the corresponding Promise object from the current CallFrame (probably by setting `this` to the Promise)

It's worth noting that the "All Exceptions" breakpoint works just fine for the example above, since that breakpoint doesn't care about whether it's caught or not.

An alternative solution would be to reclassify the try-catch used by builtins to be a `HandlerType::SynthesizedCatch`, but that is probably much more work and may have huge unforeseen consequences.

If there is a quicker solution or any "workarounds" that anyone knows of, I would be very interested to hear 😁

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180831/b6819e6e/attachment-0001.html>