Bug 196484 – Web Inspector: fake value descriptors for promises add a catch handler, preventing "rejectionhandled" events from being fired

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

```
window.addEventListener("unhandledrejection", (event) => {
    console.log("unhandledrejection", event);
});

window.addEventListener("rejectionhandled", (event) => {
    console.log("rejectionhandled", event);
});

var promise = Promise.reject(42);

setTimeout(() => {
    promise.catch((x) => {
        console.log(x);
    });
}, 500);
```

Running that code with Web Inspector open will cause the following to be logged:
1
[Log] unhandledrejection – PromiseRejectionEvent { ... }
[Error] Unhandled Promise Rejection: 42
[Log] 42

There should also be a "[Log] rejectionhandled - PromiseRejectionEvent { ... }", but that doesn't get fired because when `InjectedScript` creates a fake value descriptor (since there is a native getter `PromiseRejectionEvent.prototype.get promise`), it adds a `catch` handler to the promise to prevent "The PromiseRejectionEvent.promise getter can only be used on instances of PromiseRejectionEvent" messages from being logged to the console.  Since this `catch` handler is added as a response to "unhandledrejection", it becomes handled and is therefore no longer kept track of to fire a "rejectionhandled" when a `catch` handler is added.

<https://html.spec.whatwg.org/multipage/webappapis.html#unhandled-promise-rejections>

If the "unhandledrejection" event listener is removed (or the `event` value isn't logged to the console), the "rejectionhandled" event is fired as expected, because we don't generate a preview (which involves generating a fake value descriptor).

[Devin Rousso]

<rdar://problem/49114725>

[Devin Rousso]

Created attachment 366474 [details]
Patch

For bots, to see if any tests fail.

[Joseph Pecoraro]

Comment on attachment 366474 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366474&action=review

r- just because this needs a test and we should be able to add one. Otherwise this looks great.

> Source/JavaScriptCore/runtime/ErrorInstance.h:69
> +    bool isNativeGetterTypeError() { return m_nativeGetterTypeError; }

Nit: `const`

> Source/WebCore/Modules/streams/WritableStream.js:157
> +        return @Promise.@reject(@makeGetterTypeError("WritableStream", "closed"));

Awesome!

> Source/WebCore/bindings/js/JSDOMPromiseDeferred.h:260
> +JSC::EncodedJSValue createRejectedPromiseWithTypeError(JSC::ExecState&, const String&, bool forNativeGetter);

Nit: This boolean should really be an enum ForNativeGetter { Yes, No }. Right now at callsites it is not easily readable.

[Devin Rousso]

Created attachment 367455 [details]
Patch

[Devin Rousso]

Created attachment 367457 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 367457 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=367457&action=review

> LayoutTests/inspector/runtime/promise-native-getter.html:22
> +    }, 5);

I'd make this 50 instead of 5, but it shouldn't really matter as long as it is enough of a tick.

[Joseph Pecoraro]

Comment on attachment 367457 [details]
Patch

r=me

[Devin Rousso]

Created attachment 367461 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 367461 [details]
Patch

Clearing flags on attachment: 367461

Committed r244312: <https://trac.webkit.org/changeset/244312>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.