[webkit-reviews] review granted: [Bug 150358] Support for promise rejection events (unhandledrejection) : [Attachment 308417] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 150358: Support for promise rejection events (unhandledrejection)
https://bugs.webkit.org/show_bug.cgi?id=150358

Attachment 308417: [PATCH] Proposed Fix

https://bugs.webkit.org/attachment.cgi?id=308417&action=review




--- Comment #191 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 308417
  --> https://bugs.webkit.org/attachment.cgi?id=308417
[PATCH] Proposed Fix

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

r=me

> Source/JavaScriptCore/builtins/PromisePrototype.js:61
> +    this. at promiseIsHandled = true;

So anytime we .then(). a promise, it will be handled, therefore not reporting
any exceptions even if we didn't provide a .catch() handle or onRejected
function?
Is that the expected behavior?

Maybe we want to only set promiseIsHandled if the onRejected parameter is a
function (before we do the local assignment)?
I'm not sure if this behavior is specced or not.

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:754
> +    auto operation =
static_cast<JSPromiseRejectionOperation>(operationValue.toUInt32(exec));

nit: Can you add a DEBUG assert (even though this is only called via builtin)
that the UINT32 raw value is equal to one of the two enum values

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:755
> +    RETURN_IF_EXCEPTION(scope, encodedJSValue());

Should be an assertion, since this is a number.

> Source/WebCore/ChangeLog:16
> +	   This is currently implemented only for Documents and not yet Web
Workers.

Do we have a bug to do this for Workers?

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:410
> +    // FIXME: If script has muted errors (cross origin), terminate these
steps.

File a bug?

> Source/WebCore/dom/PromiseRejectionEvent.idl:29
> +    Exposed=(Window,Worker),

Should this include Worker for right now?

> Source/WebCore/dom/RejectedPromiseTracker.cpp:121
> +    m_aboutToBeNotifiedRejectedPromises.append(UnhandledPromise { vm,
globalObject, promise, createScriptCallStackFromReason(state, reason) });

Can we open a bug about not taking a stack trace here all the time? I'd be for
only doing this w/ Inspector open.

> Source/WebCore/dom/RejectedPromiseTracker.cpp:128
> +    bool removed =
m_aboutToBeNotifiedRejectedPromises.removeFirstMatching([&] (UnhandledPromise&
unhandledPromise) {

Having this be a Vector seems slightly dangerous since programs could cause you
to do this linear search repeatedly.

> Source/WebCore/dom/RejectedPromiseTracker.cpp:152
> +    Vector<UnhandledPromise> items;
> +    items.swap(m_aboutToBeNotifiedRejectedPromises);

style nit:
I'd write this as:
Vector<UnhandledPromise> items = WTFMove(m_aboutToBeNotifiedRejectedPromises);

> Source/WebCore/dom/RejectedPromiseTracker.cpp:200
> +    initializer.cancelable = false;

The spec you link to doesn't set this value. Should we just rely on the default
value?