Bug 196484 - Web Inspector: fake value descriptors for promises add a catch handler, preventing "rejectionhandled" events from being fired
Summary: Web Inspector: fake value descriptors for promises add a catch handler, preve...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-02 00:19 PDT by Devin Rousso
Modified: 2019-04-15 17:02 PDT (History)
13 users (show)

See Also:


Attachments
Patch (21.11 KB, patch)
2019-04-02 00:35 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (24.09 KB, patch)
2019-04-15 13:53 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (24.29 KB, patch)
2019-04-15 14:06 PDT, Devin Rousso
joepeck: review+
Details | Formatted Diff | Diff
Patch (24.33 KB, patch)
2019-04-15 14:42 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-04-02 00:19:03 PDT
```
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).
Comment 1 Devin Rousso 2019-04-02 00:19:17 PDT
<rdar://problem/49114725>
Comment 2 Devin Rousso 2019-04-02 00:35:49 PDT
Created attachment 366474 [details]
Patch

For bots, to see if any tests fail.
Comment 3 Joseph Pecoraro 2019-04-15 11:26:50 PDT
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.
Comment 4 Devin Rousso 2019-04-15 13:53:43 PDT
Created attachment 367455 [details]
Patch
Comment 5 Devin Rousso 2019-04-15 14:06:11 PDT
Created attachment 367457 [details]
Patch
Comment 6 Joseph Pecoraro 2019-04-15 14:18:54 PDT
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.
Comment 7 Joseph Pecoraro 2019-04-15 14:19:18 PDT
Comment on attachment 367457 [details]
Patch

r=me
Comment 8 Devin Rousso 2019-04-15 14:42:07 PDT
Created attachment 367461 [details]
Patch
Comment 9 WebKit Commit Bot 2019-04-15 17:02:38 PDT
Comment on attachment 367461 [details]
Patch

Clearing flags on attachment: 367461

Committed r244312: <https://trac.webkit.org/changeset/244312>
Comment 10 WebKit Commit Bot 2019-04-15 17:02:39 PDT
All reviewed patches have been landed.  Closing bug.