Bug 150358 – Support for promise rejection events (unhandledrejection)

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

any feedback on the proposed addition of promise rejection events to HTML: https://github.com/domenic/unhandled-rejections-browser-spec / https://github.com/whatwg/html/pull/224

Sam, could you please comment on this?

This moved forward and is now a pull request on whatwg/html and tc39 respectively: https://github.com/whatwg/html/pull/224 https://github.com/tc39/ecma262/pull/76

[Richard Connamacher]

This feature is now in the ECMAScript and HTML standards.

ECMAScript 2016 defines when the host's unhandled promise rejection events should fire:
 http://www.ecma-international.org/ecma-262/7.0/#sec-host-promise-rejection-tracker

HTML Living Standard defines the window unhandledrejection and rejectionhandled events:
https://html.spec.whatwg.org/multipage/webappapis.html#unhandled-promise-rejections

These events have thus far been implemented in Chrome and Node.

This is very useful, since uncaught (and unhandled) errors thrown inside promises are not seen by the window.onerror event, and in Safari do not even cause a message to be printed to the console. They just disappear silently into the ether. The unhandledrejection event fixes that, like window.onerror for promises.

[Joseph Pecoraro]

How have I not heard about this until now!

[Radar WebKit Bug Importer]

<rdar://problem/28441651>

[Joseph Pecoraro]

Created attachment 295505 [details]
[PATCH] Work in Progress

I haven't written tests or updated LayoutTests, but this was a preliminary attempt that looked good in my most basic tests. A few things that need to be handled:

    - PromiseRejectionEvent
      - when constructed with a value for "promise" that is not an instance of a Promise we should wrap it in a Promise (Blink does this)
        - new PromiseRejectionEvent("event-name", {promise:1}) => should be as if {promise: Promise.resolve(1)}

    - Layout Tests
      - update existing
      - write new tests covering the Event and event handlers

    - Web Inspector support (probably a follow-up bug)
      - pause on unhandled rejection
      - if log errors for unhandled rejections then later suppress these errors when the promise does get handled ("revoke")

[Joseph Pecoraro]

>     - PromiseRejectionEvent
>       - when constructed with a value for "promise" that is not an instance
> of a Promise we should wrap it in a Promise (Blink does this)
>         - new PromiseRejectionEvent("event-name", {promise:1}) => should be
> as if {promise: Promise.resolve(1)}

This is defined in Web IDL:
<https://heycam.github.io/webidl/#es-promise>

Converting a JavaScript Value `v` to IDL `Promise<any>` type is the equivalent of `Promise.resolve(v)`.

[Yusuke Suzuki]

Comment on attachment 295505 [details]
[PATCH] Work in Progress

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

Awesome. Early stage review.

> Source/JavaScriptCore/builtins/PromiseOperations.js:109
> +        @hostPromiseRejectionTracker(promise, "reject");

I recommend using some bytecode intrinsic constants instead of the string `"reject"`.
For example, @promiseStatePending is represented as a bytecode intrinsic constant.
The benefit of the bytecode intrinsic constant is we can share the same value in C++ and JS.
You can see how to define it in bytecode/BytecodeIntrinsicRegistry.h.

> Source/JavaScriptCore/builtins/PromisePrototype.js:62
> +            @hostPromiseRejectionTracker(promise, "handle");

Ditto.

> Source/JavaScriptCore/runtime/JSGlobalObject.h:167
> +enum class JSPromiseRejectionOperation {
> +    Reject, // When a promise is rejected without any handlers.
> +    Handle, // When a handler is added to a rejected promise for the first time.
> +};

You can expose these enums directly to the JS world through bytecode intrinsic constants.

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:940
> +    JSPromiseRejectionOperation operation;
> +    if (operationString == "reject")
> +        operation = JSPromiseRejectionOperation::Reject;
> +    else {
> +        ASSERT(operationString == "handle");
> +        operation = JSPromiseRejectionOperation::Handle;
> +    }

And then, you can just `static_cast<>()` the argument's number to the enum.

[Yusuke Suzuki]

Once it is done, I would like to add a promiseRejectionTracker in the JSC shell.
It is super useful if the JSC shell can report error drained by promises when exiting. That makes the async JSTests easy.

[Joseph Pecoraro]

If anyone wants to take over for this that would be fine by me. I don't know when I'll find time to come back to this.

[Yusuke Suzuki]

(In reply to comment #10)
> If anyone wants to take over for this that would be fine by me. I don't know
> when I'll find time to come back to this.

OK, I'll work on this :)
It is super useful to debug asynchronous features in WebCore/JSC shell.

[Yusuke Suzuki]

Created attachment 298141 [details]
Patch

WIP: we need cross-origin check. This will be done by using extended SourceOrigin

[Yusuke Suzuki]

Comment on attachment 298141 [details]
Patch

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

> Source/WebCore/dom/PromiseRejectionEvent.cpp:70
> +JSValue PromiseRejectionEvent::sanitizedPromiseValue(ExecState& state)
> +{
> +    auto promise = m_promise.get();
> +    if (!promise)
> +        return jsNull();
> +
> +    if (promise.isObject() && &worldForDOMObject(promise.getObject()) != &currentWorld(&state))
> +        return jsNull();
> +
> +    return promise;
> +}
> +
> +JSValue PromiseRejectionEvent::sanitizedReasonValue(ExecState& state)
> +{
> +    auto reason = m_reason.get();
> +    if (!reason)
> +        return jsNull();
> +
> +    if (reason.isObject() && &worldForDOMObject(reason.getObject()) != &currentWorld(&state))
> +        return jsNull();
> +
> +    return reason;
> +}

I think this is not necessary.
Instead, we should perform cross-origin check in the hostPromiseRejectionTracker correctly.

[Joseph Pecoraro]

Comment on attachment 298141 [details]
Patch

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

>> Source/WebCore/dom/PromiseRejectionEvent.cpp:70
>> +}
> 
> I think this is not necessary.
> Instead, we should perform cross-origin check in the hostPromiseRejectionTracker correctly.

What about someone creating their own PromiseRejectionEvent with their own values? Or maybe that is not possible?

    new PromiseRejectionEvent("name, {promise: value1, reason: value2});

[Yusuke Suzuki]

(In reply to comment #14)
> Comment on attachment 298141 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298141&action=review
> 
> >> Source/WebCore/dom/PromiseRejectionEvent.cpp:70
> >> +}
> > 
> > I think this is not necessary.
> > Instead, we should perform cross-origin check in the hostPromiseRejectionTracker correctly.
> 
> What about someone creating their own PromiseRejectionEvent with their own
> values? Or maybe that is not possible?
> 
>     new PromiseRejectionEvent("name, {promise: value1, reason: value2});

This should be allowed. We can create such an event object.
The problem is that the current patch does not perform any Promise type check for value1 (value2 is OK since it is any in IDL).

Yup, we can perform promise check super easily: just introducing type check to JSDOMConverter's IDLPromise case.
But I'm not sure whether IDL's Promise<any> should perform this type check.
This prohibits us from passing user-defined thenables here. Like,

`new PromiseRejectionEvent("name", { promise: new BlueBirdPromise(), reason: any })`

[Yusuke Suzuki]

Created attachment 298145 [details]
Patch

WIP: Fix linking error

[Build Bot]

Comment on attachment 298145 [details]
Patch

Attachment 298145 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2840702

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298152 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 298145 [details]
Patch

Attachment 298145 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2840683

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298153 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 298145 [details]
Patch

Attachment 298145 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2840754

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298154 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 298145 [details]
Patch

Attachment 298145 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2840752

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298155 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

Created attachment 298175 [details]
Patch

WIP: Fix failures

[Yusuke Suzuki]

Created attachment 298177 [details]
Patch

WIP: Fix failures

[Build Bot]

Comment on attachment 298177 [details]
Patch

Attachment 298177 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2858634

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298361 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 298177 [details]
Patch

Attachment 298177 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2858635

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298363 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 298177 [details]
Patch

Attachment 298177 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2858643

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298364 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 298177 [details]
Patch

Attachment 298177 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2859043

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298370 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

This cross origin check requires extended SourceOrigin support which I'm working on for dynamic import and worker modules.

[Yusuke Suzuki]

Created attachment 298600 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 298601 [details]
Patch

WIP

[Build Bot]

Comment on attachment 298601 [details]
Patch

Attachment 298601 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2871012

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298607 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 298601 [details]
Patch

Attachment 298601 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2871006

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298608 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 298601 [details]
Patch

Attachment 298601 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2871058

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298609 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 298601 [details]
Patch

Attachment 298601 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2871026

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298610 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

Created attachment 298671 [details]
Patch

WIP

[Build Bot]

Comment on attachment 298671 [details]
Patch

Attachment 298671 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2877283

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298677 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 298671 [details]
Patch

Attachment 298671 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2877289

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298678 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 298671 [details]
Patch

Attachment 298671 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2877312

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298679 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 298671 [details]
Patch

Attachment 298671 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2877429

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298680 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

Created attachment 298682 [details]
Patch

WIP

[Build Bot]

Comment on attachment 298682 [details]
Patch

Attachment 298682 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2877735

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298685 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 298682 [details]
Patch

Attachment 298682 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2877740

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298686 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 298682 [details]
Patch

Attachment 298682 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2877747

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298687 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Comment on attachment 298682 [details]
Patch

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

> LayoutTests/js/dom/unhandled-promise-rejection-console-report.html:14
> +window.promise = [];

Nit: promises

> LayoutTests/js/dom/unhandled-promise-rejection-console-report.html:19
> +setTimeout(function () { finishJSTest(); }, 100);

Its weird to have a 100ms delay to finish a test in the successful case.

Can you get away with your own resolve microtask like this:

    for (var i = 0; i < 3; ++i)
        window.promises[i] = Promise.reject(i);
    Promise.resolve().then(finishJSTest);
    window.onunhandledrejection = function (e) { return true; }

> LayoutTests/js/dom/unhandled-promise-rejection-handle-during-event.html:26
> +    setTimeout(function () { finishJSTest(); });

Style: Could be setTimeout(finishJSTest)

> LayoutTests/js/dom/unhandled-promise-rejection-handle-in-handler.html:31
> +    setTimeout(function () {
> +        finishJSTest();
> +    }, 100);

Style: Could be setTimeout(finishJSTest, 100)

Arg, another one. I guess this one is unavoidable if the goal is to ensure no onrejectionhandled happens for some small amount of time.

[Build Bot]

Comment on attachment 298682 [details]
Patch

Attachment 298682 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2878370

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298704 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

Created attachment 302120 [details]
Patch

rebaseline

[Yusuke Suzuki]

Created attachment 302127 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 302127 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:3250
> +        });

OK, nice. This patch has found a bug in media element. I'll extract this part.

[Yusuke Suzuki]

Created attachment 302129 [details]
Patch

[Build Bot]

Comment on attachment 302129 [details]
Patch

Attachment 302129 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3158590

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 302132 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 302129 [details]
Patch

Attachment 302129 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3158611

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 302133 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 302129 [details]
Patch

Attachment 302129 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3158601

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 302134 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 302129 [details]
Patch

Attachment 302129 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3158603

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 302135 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

One more finding: We should expose some utility that returns *handled* rejected promise since it is used in whatwg streams.

https://streams.spec.whatwg.org/#readable-stream-reader-generic-release
Set reader.[[closedPromise]].[[PromiseIsHandled]] to true.

[Yusuke Suzuki]

Created attachment 302137 [details]
Patch

[Build Bot]

Comment on attachment 302137 [details]
Patch

Attachment 302137 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3159185

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 302139 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 302137 [details]
Patch

Attachment 302137 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3159208

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 302140 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 302137 [details]
Patch

Attachment 302137 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3159205

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 302141 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 302137 [details]
Patch

Attachment 302137 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3159219

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 302142 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

I'll investigate it more in this weekend.
Basically, there are several issues,

1. Promise use of Media Element is slightly different from the spec, it causes unintended unhandled promise rejections
2. Stream needs to avoid unhandled promise rejections. Some part of spec explicitly describes it. I think it involves major clean up of Stream / Fetch JS implementation related to promises.
3. Another unhandled promise rejections. I need to investigate it...

[Yusuke Suzuki]

Created attachment 303937 [details]
Patch

[Yusuke Suzuki]

Yeah, the patch is getting its shape! The failures decrease to 18!
I'm now investigating these failures to check whether this requires test expect file rebaselining.
Some may be real bug. But most of them are just requiring expect file rebaselining because we previously ignored unhandled promises.

[Build Bot]

Comment on attachment 303937 [details]
Patch

Attachment 303937 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3277499

New failing tests:
js/promises-tests/promises-tests-2-2-6.html
inspector/worker/resources-in-worker.html
media/track/track-cues-pause-on-exit.html
inspector/debugger/break-on-exception-throw-in-promise.html
js/dom/dom-static-property-for-in-iteration.html
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/test_ecdh_bits.html
media/modern-media-controls/start-support/start-support-manual-play.html
inspector/debugger/break-on-uncaught-exception-throw-in-promise.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/event_pause_noautoplay.html
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/test_ecdh_keys.html
fast/mediastream/MediaStream-MediaElement-setObject-null.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/event_play_noautoplay.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/paused_true_during_pause.html
http/tests/security/video-cross-origin-caching.html

[Build Bot]

Created attachment 303957 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 303937 [details]
Patch

Attachment 303937 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3277494

New failing tests:
js/promises-tests/promises-tests-2-2-6.html
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/test_ecdh_keys.html
media/W3C/audio/events/event_pause_manual.html
media/track/track-cues-pause-on-exit.html
inspector/debugger/break-on-exception-throw-in-promise.html
js/dom/dom-static-property-for-in-iteration.html
streams/reference-implementation/abstract-ops.html
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/test_ecdh_bits.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/paused_true_during_pause.html
inspector/debugger/break-on-uncaught-exception-throw-in-promise.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/event_pause_noautoplay.html
imported/w3c/web-platform-tests/streams/readable-byte-streams/general.html
imported/w3c/web-platform-tests/streams/readable-streams/tee.html
http/tests/security/video-cross-origin-caching.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/event_play_noautoplay.html
media/W3C/video/events/event_pause_manual.html
media/W3C/video/paused/paused_true_during_pause.html

[Build Bot]

Created attachment 303961 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 303937 [details]
Patch

Attachment 303937 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3277571

New failing tests:
js/promises-tests/promises-tests-2-2-6.html
inspector/worker/resources-in-worker.html
media/track/track-cues-pause-on-exit.html
inspector/debugger/break-on-exception-throw-in-promise.html
js/dom/dom-static-property-for-in-iteration.html
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/test_ecdh_bits.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/paused_true_during_pause.html
inspector/debugger/break-on-uncaught-exception-throw-in-promise.html
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/test_ecdh_keys.html
http/tests/security/video-cross-origin-caching.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/event_play_noautoplay.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/event_pause_noautoplay.html

[Build Bot]

Created attachment 303963 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 303937 [details]
Patch

Attachment 303937 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3277555

New failing tests:
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/test_ecdh_bits.html
streams/brand-checks.html
media/track/track-cues-pause-on-exit.html
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/test_ecdh_keys.html
http/tests/security/video-cross-origin-caching.html

[Build Bot]

Created attachment 303970 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

I'm going to pick this up again!

[Joseph Pecoraro]

> 1. Promise use of Media Element is slightly different from the spec, it
> causes unintended unhandled promise rejections

I see you slightly tackled this in your most current patch.

> 2. Stream needs to avoid unhandled promise rejections. Some part of spec
> explicitly describes it. I think it involves major clean up of Stream /
> Fetch JS implementation related to promises.
> 3. Another unhandled promise rejections. I need to investigate it...

I'll look into these.

[Joseph Pecoraro]

Comment on attachment 303937 [details]
Patch

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

> Source/JavaScriptCore/builtins/PromiseOperations.js:91
>  @globalPrivate
> +function newRejectedPromise(error)
> +{
> +    return @Promise.@reject(error);
> +}

This is unused, since everyone just uses @Promise.@reject directly right now. I think its fine, since the only real special case we care about is creating a handled rejected promise (the function below).

[Joseph Pecoraro]

Support for workers will require a lot of smaller miscellaneous changes. Essentially making the equivalent of JSMainThreadExecState for workers, since JSMainThreadExecState automatically does operations when completing a task in the script execution context (microtask checkpoint, poke the promise tracker queue, etc), and that doesn't appear to exist for Workers.

To avoid cluttering and increasing the complexity of this patch lets add Worker support in a follow-up.

[Joseph Pecoraro]

Created attachment 307555 [details]
[PATCH] Rebaselined - For Bots

Patch only, updated with a few new tests, plenty of new test expectations, and a few minor changes (mainly for inspector). Lets see what new tests fail.

[Build Bot]

Comment on attachment 307555 [details]
[PATCH] Rebaselined - For Bots

Attachment 307555 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3568148

New failing tests:
webrtc/libwebrtc/release-while-creating-offer.html
media/track/track-cues-pause-on-exit.html
imported/w3c/web-platform-tests/streams/piping/general.html
inspector/worker/resources-in-worker.html
webrtc/libwebrtc/release-while-setting-local-description.html
fast/mediastream/MediaStream-MediaElement-setObject-null.html

[Build Bot]

Created attachment 307565 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307555 [details]
[PATCH] Rebaselined - For Bots

Attachment 307555 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3568180

New failing tests:
imported/w3c/web-platform-tests/streams/piping/general.html
streams/brand-checks.html
imported/w3c/web-platform-tests/streams/readable-streams/tee.html
streams/reference-implementation/abstract-ops.html
media/track/track-cues-pause-on-exit.html

[Build Bot]

Created attachment 307571 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307555 [details]
[PATCH] Rebaselined - For Bots

Attachment 307555 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3568254

New failing tests:
imported/w3c/web-platform-tests/streams/piping/general.html
webrtc/libwebrtc/release-while-creating-offer.html
webrtc/libwebrtc/release-while-setting-local-description.html
media/track/track-cues-pause-on-exit.html

[Build Bot]

Created attachment 307575 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307555 [details]
[PATCH] Rebaselined - For Bots

Attachment 307555 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3569194

New failing tests:
imported/w3c/web-platform-tests/streams/piping/general.html
inspector/worker/resources-in-worker.html
media/track/track-cues-pause-on-exit.html

[Build Bot]

Created attachment 307588 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Created attachment 307618 [details]
[PATCH] Rebaselined - For Bots

[Joseph Pecoraro]

> Regressions: Unexpected text-only failures (1)
>   imported/w3c/web-platform-tests/streams/piping/general.html [ Failure ]

This is not failing for me. I'm going to rebaseline again

btw, what's the different between the new tests you added and the wpt tests?

[Joseph Pecoraro]

(In reply to jochen from comment #113)
> btw, what's the different between the new tests you added and the wpt tests?

Nothing specific, they are very basic.

I briefly tested the wpt's on w3c-test.org and I plan on importing them into WebKit.

[Build Bot]

Comment on attachment 307618 [details]
[PATCH] Rebaselined - For Bots

Attachment 307618 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3571857

New failing tests:
imported/w3c/web-platform-tests/streams/piping/general.html
media/track/track-cues-pause-on-exit.html

[Build Bot]

Created attachment 307638 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307618 [details]
[PATCH] Rebaselined - For Bots

Attachment 307618 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3571853

New failing tests:
imported/w3c/web-platform-tests/streams/piping/general.html
media/track/track-cues-pause-on-exit.html

[Build Bot]

Created attachment 307639 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

(In reply to Joseph Pecoraro from comment #114)
> (In reply to jochen from comment #113)
> > btw, what's the different between the new tests you added and the wpt tests?
> 
> Nothing specific, they are very basic.

Err, there are a couple specific tests we have regarding seeing Console Logs or not seeing Console Logs based on return value from onunhandledrejection. I don't think wpt has tests for this (or can even test this).

[Joseph Pecoraro]

(In reply to jochen from comment #113)
> btw, what's the different between the new tests you added and the wpt tests?

If you work on the web-platform-tests for promise rejection events you may be interested in:
https://github.com/w3c/web-platform-tests/issues/5629

I'll be filing other web-platform-tests issues / browser issues if I encounter issues / differences.

[Build Bot]

Comment on attachment 307618 [details]
[PATCH] Rebaselined - For Bots

Attachment 307618 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3572052

New failing tests:
streams/brand-checks.html
media/W3C/audio/paused/paused_true_during_pause.html
media/track/track-cues-pause-on-exit.html
imported/w3c/web-platform-tests/streams/piping/general.html
imported/w3c/web-platform-tests/streams/readable-streams/tee.html
streams/reference-implementation/abstract-ops.html

[Build Bot]

Created attachment 307644 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307618 [details]
[PATCH] Rebaselined - For Bots

Attachment 307618 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3571965

New failing tests:
imported/w3c/web-platform-tests/streams/piping/general.html
media/track/track-cues-pause-on-exit.html

[Build Bot]

Created attachment 307645 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

So all (most?) of the testharness failures for onunhandledrejection are because our testharnessreport.js has a setTimeout in the completion handler. When completion happens, we have 0 failures, then after a setTimeout, we update the UI by which time there are non-0 failures because onunhandledrejection (like onerror) acts like a failure.

The setTimeout is adding flakiness. It was added for Resource Timing tests less than a year ago. So I'm going to try removing it. That should mean a lot of the rebaslines I just did might be able to go back to what they had.

[Joseph Pecoraro]

Created attachment 307668 [details]
[PATCH] Rebaselined - For Bots

[Joseph Pecoraro]

Pretty soon I'm going to start marking remaining tests as flakey if they continue to be flakey with this error: See Bug 171094.

[Alexey Proskuryakov]

What would that mean for our test coverage? Reducing test coverage in unrelated features doesn't seem like something that should be done to add another feature.

[Joseph Pecoraro]

(In reply to Alexey Proskuryakov from comment #128)
> What would that mean for our test coverage? Reducing test coverage in
> unrelated features doesn't seem like something that should be done to add
> another feature.

It seems only to be an issue with media tests (video.play can produce an error) and streams test. And the ones I'm marking as flakey are web-platform-tests which I don't think we want to modify the test directly.

[Build Bot]

Comment on attachment 307668 [details]
[PATCH] Rebaselined - For Bots

Attachment 307668 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3573382

New failing tests:
streams/reference-implementation/readable-stream-templated.html
imported/w3c/web-platform-tests/user-timing/measure_exceptions_navigation_timing.html
media/track/track-cues-pause-on-exit.html
imported/w3c/web-platform-tests/html/syntax/parsing/html5lib_innerHTML_adoption01.html

[Build Bot]

Created attachment 307675 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307668 [details]
[PATCH] Rebaselined - For Bots

Attachment 307668 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3573391

New failing tests:
media/track/track-cues-pause-on-exit.html
streams/reference-implementation/readable-stream-templated.html
streams/reference-implementation/abstract-ops.html
media/modern-media-controls/start-support/start-support-manual-play.html
imported/w3c/web-platform-tests/user-timing/measure_exceptions_navigation_timing.html
imported/w3c/web-platform-tests/html/syntax/parsing/html5lib_innerHTML_adoption01.html

[Build Bot]

Created attachment 307677 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Created attachment 307679 [details]
[PATCH] Rebaselined - For Bots

This should be a final round for the bots.

[Build Bot]

Comment on attachment 307679 [details]
[PATCH] Rebaselined - For Bots

Attachment 307679 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3573783

New failing tests:
media/track/track-cues-pause-on-exit.html

[Build Bot]

Created attachment 307687 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307679 [details]
[PATCH] Rebaselined - For Bots

Attachment 307679 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3573774

New failing tests:
media/track/track-cues-pause-on-exit.html

[Build Bot]

Created attachment 307688 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307679 [details]
[PATCH] Rebaselined - For Bots

Attachment 307679 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3573806

New failing tests:
media/track/track-cues-pause-on-exit.html
media/W3C/video/events/event_pause_manual.html

[Build Bot]

Created attachment 307689 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Created attachment 307690 [details]
[PATCH] Proposed Fix

First reviewable patch.

Things left for follow-ups:

  • Worker Support
    - workers process microtasks differently, I have a WIP
  • Promise<T> binding auto conversions
    - affects only PromiseRejectionEvent construction right now
  • Muting of Errors
    - This is mostly a feature of onerror

[Ryosuke Niwa]

Comment on attachment 307690 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/builtins/BuiltinNames.h:85
> +    macro(hostPromiseRejectionTracker) \

It's very strange

> Source/WebCore/bindings/js/JSDOMConvertPromise.h:-28
> -#include "JSObject.h"

Where this did this file come from!? It seems like this is a new file. Why do we have diff like this?

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:404
> +    // The state of InternalPromise is hidden in the browser side.

I don't understand what this means.
Is JSInternalPromise builtin-only objects that shouldn't be exposed to author scripts?

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:410
> +    // FIXME: Cross origin check.
> +    auto sourceOrigin = exec->callerSourceOrigin();
> +    UNUSED_PARAM(sourceOrigin);

What kind of cross-origin check are we expecting to perform?

[Build Bot]

Comment on attachment 307690 [details]
[PATCH] Proposed Fix

Attachment 307690 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3574150

New failing tests:
media/video-play-pause-events.html
media/video-played-ranges-1.html
media/video-multiple-concurrent-playback.html

[Build Bot]

Created attachment 307693 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307690 [details]
[PATCH] Proposed Fix

Attachment 307690 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3574154

New failing tests:
media/video-play-pause-events.html
media/video-played-ranges-1.html
media/video-multiple-concurrent-playback.html

[Build Bot]

Created attachment 307694 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307690 [details]
[PATCH] Proposed Fix

Attachment 307690 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3574212

New failing tests:
media/video-play-pause-events.html
media/video-played-ranges-1.html
media/video-multiple-concurrent-playback.html

[Build Bot]

Created attachment 307696 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307690 [details]
[PATCH] Proposed Fix

Attachment 307690 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3574685

New failing tests:
media/video-play-pause-events.html
media/video-played-ranges-1.html
media/video-multiple-concurrent-playback.html

[Build Bot]

Created attachment 307707 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Comment on attachment 307690 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/builtins/BuiltinNames.h:85
>> +    macro(hostPromiseRejectionTracker) \
> 
> It's very strange

This is the name of the hook in the spec:

    ... perform HostPromiseRejectionTracker(promise, "handle") ...

>> Source/WebCore/bindings/js/JSDOMConvertPromise.h:-28
>> -#include "JSObject.h"
> 
> Where this did this file come from!? It seems like this is a new file. Why do we have diff like this?

This is just git thinking the files are similar. I'll put up another patch where git considers them different.

>> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:404
>> +    // The state of InternalPromise is hidden in the browser side.
> 
> I don't understand what this means.
> Is JSInternalPromise builtin-only objects that shouldn't be exposed to author scripts?

Yusuke added this originally. Your comment is mostly correct. Internal Promise looks like WebCore internal only promises used by module loading. But the salient point is that we don't want to expose them to author scripts.

>> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:410
>> +    UNUSED_PARAM(sourceOrigin);
> 
> What kind of cross-origin check are we expecting to perform?

The spec says:
https://html.spec.whatwg.org/multipage/webappapis.html#the-hostpromiserejectiontracker-implementation

    2. If script has muted errors, terminate these steps.

I'll clarify this.

[Joseph Pecoraro]

Created attachment 307850 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 307850 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/html/HTMLMediaElement.cpp:3295
> +        m_promiseTaskQueue.enqueueTask([this]() {
> +            scheduleTimeupdateEvent(false);
> +            scheduleEvent(eventNames().pauseEvent);
> +            rejectPendingPlayPromises(DOMError::create("AbortError", "The operation was aborted."));
> +        });

Apparently this seems to break:

    media/track/track-cues-pause-on-exit.html

So I'll have to think about this. The rest of the patch is reviewable.

[Build Bot]

Comment on attachment 307850 [details]
[PATCH] Proposed Fix

Attachment 307850 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3580384

New failing tests:
media/track/track-cues-pause-on-exit.html

[Build Bot]

Created attachment 307862 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307850 [details]
[PATCH] Proposed Fix

Attachment 307850 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3580387

New failing tests:
media/track/track-cues-pause-on-exit.html

[Build Bot]

Created attachment 307863 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307850 [details]
[PATCH] Proposed Fix

Attachment 307850 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3580672

New failing tests:
media/track/track-cues-pause-on-exit.html

[Build Bot]

Created attachment 307871 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307850 [details]
[PATCH] Proposed Fix

Attachment 307850 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3582827

New failing tests:
media/track/track-cues-pause-on-exit.html

[Build Bot]

Created attachment 307899 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

Comment on attachment 307690 [details]
[PATCH] Proposed Fix

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

>>> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:404
>>> +    // The state of InternalPromise is hidden in the browser side.
>> 
>> I don't understand what this means.
>> Is JSInternalPromise builtin-only objects that shouldn't be exposed to author scripts?
> 
> Yusuke added this originally. Your comment is mostly correct. Internal Promise looks like WebCore internal only promises used by module loading. But the salient point is that we don't want to expose them to author scripts.

Right. JSInternalPromise also uses the same operation functions in PromiseOperations.js, thus, there is a change that InternalPromise comes here.
But we do not want to expose it to userspace.

[Ryosuke Niwa]

Comment on attachment 307850 [details]
[PATCH] Proposed Fix

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

Reviewed test changes.

> LayoutTests/ChangeLog:38
> +        Report results immediately and then finish the test after a turn. This way
> +        if the test ends with a pass, but may get unhandled rejections after
> +        completing which should not make the test appear as if it failed. Some tests
> +        have unhandled promise rejections but are expected to pass. Likewise some
> +        tests perform cleanup in their own completion callbacks, which happen after
> +        this initial completion callback, and we want to report results after all
> +        the work is done as it may eliminate non-deterministic debug test output.

I'm afraid that some tests might be reporting some errors in completion handlers.
It seems to me there isn't much harm to rebaselining the tests with unhandled promise rejection logs so long as they're deterministic
although we may want to do that in a separate patch since I'd expect there will be a lot of tests that have unhandled rejected promises.

> LayoutTests/js/dom/dom-static-property-for-in-iteration-expected.txt:115
> -PASS a["offsetTop"] is 1689
> +PASS a["offsetTop"] is 1719

Why did this change? Does this change each time a new global property is added!?

> LayoutTests/js/dom/unhandled-promise-rejection-basic.html:17
> +window.onunhandledrejection = function (e) {
> +    error = e;

We could have done:
window.error = error and spelled out error instead of using a single letter variable name.

> LayoutTests/js/dom/unhandled-promise-rejection-basic.html:18
> +    shouldBe(`error.type`, `"unhandledrejection"`);

It seems template literal isn't needed in all these shouldBe statements.

> LayoutTests/js/dom/unhandled-promise-rejection-bindings-type-error.html:17
> +    error = e;

Ditto.

> LayoutTests/js/dom/unhandled-promise-rejection-bindings-type-error.html:18
> +    shouldBe(`error.promise`, `promise`);

Ditto about not needing template literal.

> LayoutTests/js/dom/unhandled-promise-rejection-console-no-report.html:19
> +window.onunhandledrejection = function (e) {

Spell out error or omit the argument.

> LayoutTests/js/dom/unhandled-promise-rejection-console-no-report.html:27
> +}, 100);

Why 100ms? setTimeout(0) should be sufficient given that would be in the next runloop.
Promise rejection happens at the end of the current micro task.

> LayoutTests/js/dom/unhandled-promise-rejection-console-report-expected.txt:4
> +Test unhandled promise rejection event produces console messages.

The condition under which this test should be included in the description.
e.g. "The test passes when you see three console messages for unhandled promise rejection"

> LayoutTests/js/dom/unhandled-promise-rejection-console-report-expected.txt:10
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE

It's misleading to use js-test-pre.js in this test case because the pass condition depends on the stdout output.

> LayoutTests/js/dom/unhandled-promise-rejection-console-report.html:18
> +window.onunhandledrejection = function (e) { return true; };

Spell out error or omit the argument.

> LayoutTests/js/dom/unhandled-promise-rejection-console-report.html:19
> +setTimeout(function () { finishJSTest(); }, 100);

Ditto about not needing 100ms delay.

> LayoutTests/js/dom/unhandled-promise-rejection-handle-during-event.html:17
> +window.onunhandledrejection = function (e) {
> +    error = e;

Ditto about window.error = error.

> LayoutTests/js/dom/unhandled-promise-rejection-handle-during-event.html:19
> +    shouldBe(`error.type`, `"unhandledrejection"`);

ditto about not needing template literals.

> LayoutTests/js/dom/unhandled-promise-rejection-handle-during-event.html:26
> +    promise[1].catch(function () { });
> +    promise[2].catch(function () { });
> +    setTimeout(function () { finishJSTest(); });

Why not arrow functions? I know this would result in setTimeout(~, 0) but why don't we explicitly do that?

> LayoutTests/js/dom/unhandled-promise-rejection-handle-during-event.html:31
> +for (var i = 0; i < 3; ++i)
> +    window.promise[i] = Promise.reject(i);

What's the significance of having three rejected promises?
Also, I'd prefer if this was done inside evalAndLog so that it's apparent in the expected result.

> LayoutTests/js/dom/unhandled-promise-rejection-handle-in-handler.html:19
> +window.onunhandledrejection = function (e) {
> +    error = e;

Ditto about window.error = error.

> LayoutTests/js/dom/unhandled-promise-rejection-handle-in-handler.html:25
> +    promise.catch(function (r) {
> +        window.reason = r;

Maybe window.reason = reason?

> LayoutTests/js/dom/unhandled-promise-rejection-handle-in-handler.html:31
> +    }, 100);

Ditto about not needing 100ms delay.

> LayoutTests/js/dom/unhandled-promise-rejection-handle.html:20
> +window.onunhandledrejection = function (e) {
> +    error = e;

Ditto about window.error = error.

> LayoutTests/js/dom/unhandled-promise-rejection-handle.html:35
> +window.onrejectionhandled = function (e) {
> +    window.handled = e;

Ditto. Maybe window.handledError?

> LayoutTests/js/dom/unhandled-promise-rejection-order.html:19
> +window.onunhandledrejection = function (e) {
> +    error = e;

Ditto about window.error = error.

> LayoutTests/js/dom/unhandled-promise-rejection-order.html:28
> +    if (count === 3)
> +        finishJSTest();

Manually managing index, why don't we push each error into a global array,
and then call a separate function in setTimeout(~, 0) to assert them all at once?
That'd make it possible to always call finishJSTest in that function.

> LayoutTests/media/video-autoplay-allowed-but-fullscreen-required.html:18
> +        var promise = video.play();

Why not const or let?

> LayoutTests/media/video-main-content-deny-display-none.html:18
> +        var promise = video.play();

Ditto. Use const or let?

> LayoutTests/media/video-main-content-deny-not-in-dom.html:18
> +        var promise = video.play();

Ditto.

> LayoutTests/media/video-main-content-deny-not-visible.html:18
> +        var promise = video.play();

Ditto.

> LayoutTests/media/video-main-content-deny-obscured.html:18
> +        var promise = video.play();

Ditto.

> LayoutTests/media/video-main-content-deny-too-small.html:19
> +        var promise = video.play();

Ditto.

[Joseph Pecoraro]

Comment on attachment 307850 [details]
[PATCH] Proposed Fix

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

>> LayoutTests/ChangeLog:38
>> +        the work is done as it may eliminate non-deterministic debug test output.
> 
> I'm afraid that some tests might be reporting some errors in completion handlers.
> It seems to me there isn't much harm to rebaselining the tests with unhandled promise rejection logs so long as they're deterministic
> although we may want to do that in a separate patch since I'd expect there will be a lot of tests that have unhandled rejected promises.

I understand the concern but that seems like it would be an inappropriate use of testharness.js.

add_completion_callback is used to add a callback _after_ a set of tests completes. It is provided the status (PASS, FAIL, TIMEOUT). So effectively it is being run after tests have completed. If a test had asynchronous work that would have caused the test to fail it should have failed the test earlier.

This also appears to match the reporting out of PASS/FAIL on w3c-test.org. Those tests may have unexpected promise rejections reported after the tests completed, but the test still shows a PASS at the top. They have something like:

    WindowTestEnvironment.prototype.on_tests_ready = function() {
        ...
        add_completion_callback(function (tests, harness_status) {
            this_obj.output_handler.show_results(tests, harness_status);
        });
        ...
    };

Which roughly appears to match us here.

The only reason I left in the setTimeout is that some tests decide to "clean up" the page in completion handlers. In one Resource Timing tests this is important as it hides a lot of debug output written to the page containing raw timing numbers (which are dynamic) and would need to be hidden in order to the test to have deterministic output.

We could be stricter here and disable testharness.js's window.onerror / window.onunhandledpromiserejection once the test is done, but I think that might hide unintended issues that should instead be addressed instead of hidden.

>> LayoutTests/js/dom/dom-static-property-for-in-iteration-expected.txt:115
>> +PASS a["offsetTop"] is 1719
> 
> Why did this change? Does this change each time a new global property is added!?

Correct, this is logging all of the properties of an element using the equivalent of `for (var i in element)`. Which is likely happening because we insert the <p id="description"> and #console as the first children of <body>. So as the console fills up with pass messages for earlier properties, then when we evaluate offsetTop it includes all of the console messages preceding it. I can clean this up but it doesn't seem important.

>> LayoutTests/js/dom/unhandled-promise-rejection-basic.html:18
>> +    shouldBe(`error.type`, `"unhandledrejection"`);
> 
> It seems template literal isn't needed in all these shouldBe statements.

This is a style used heavily by JavaScriptCore tests that I tend to agree with. Anytime we use a string to evaluate CODE then template strings are vastly friendlier then using string literals.

Most of these comments on the LayoutTests are style preferences that I'd prefer to keep. They are mostly canonical patterns used in tests, and given the tests are pretty small they should be easy to follow.

>> LayoutTests/js/dom/unhandled-promise-rejection-console-no-report.html:27
>> +}, 100);
> 
> Why 100ms? setTimeout(0) should be sufficient given that would be in the next runloop.
> Promise rejection happens at the end of the current micro task.

While that is technically true, this is trying to detect an incorrect implementation! So if an incorrect implementation incorrectly emitted things later then a micro-task it would not have been caught by setTimeout(0).

>> LayoutTests/js/dom/unhandled-promise-rejection-console-report-expected.txt:4
>> +Test unhandled promise rejection event produces console messages.
> 
> The condition under which this test should be included in the description.
> e.g. "The test passes when you see three console messages for unhandled promise rejection"

Sounds good!

>> LayoutTests/js/dom/unhandled-promise-rejection-console-report.html:19
>> +setTimeout(function () { finishJSTest(); }, 100);
> 
> Ditto about not needing 100ms delay.

In this case we should be able to do setTimeout 0, so I'll update this one.

>> LayoutTests/js/dom/unhandled-promise-rejection-handle-during-event.html:31
>> +    window.promise[i] = Promise.reject(i);
> 
> What's the significance of having three rejected promises?
> Also, I'd prefer if this was done inside evalAndLog so that it's apparent in the expected result.

This is testing that the unhandled promise handler is called with only the unhandled promise, the others are handled so should not have triggered the handler.

>> LayoutTests/js/dom/unhandled-promise-rejection-handle-in-handler.html:31
>> +    }, 100);
> 
> Ditto about not needing 100ms delay.

A 0 here should be correct, I'll change.

>> LayoutTests/js/dom/unhandled-promise-rejection-order.html:28
>> +        finishJSTest();
> 
> Manually managing index, why don't we push each error into a global array,
> and then call a separate function in setTimeout(~, 0) to assert them all at once?
> That'd make it possible to always call finishJSTest in that function.

Sounds good.

[Joseph Pecoraro]

Comment on attachment 307850 [details]
[PATCH] Proposed Fix

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

>>> LayoutTests/js/dom/unhandled-promise-rejection-console-no-report.html:27
>>> +}, 100);
>> 
>> Why 100ms? setTimeout(0) should be sufficient given that would be in the next runloop.
>> Promise rejection happens at the end of the current micro task.
> 
> While that is technically true, this is trying to detect an incorrect implementation! So if an incorrect implementation incorrectly emitted things later then a micro-task it would not have been caught by setTimeout(0).

Actually these cannot be changed to setTimeout(0). The Promise rejection happens at the end of the micro task, but post mircotasks we "queues a task" to notify about unhandled rejections. So we do have to wait a couple task queues. I'll leave these 100ms.

[Alexey Proskuryakov]

The rule of thumb is that no timeout other than 0ms is ever correct in regression tests. We can have threads and processes frozen for several seconds.

[Joseph Pecoraro]

(In reply to Alexey Proskuryakov from comment #166)
> The rule of thumb is that no timeout other than 0ms is ever correct in
> regression tests. We can have threads and processes frozen for several
> seconds.

So would it be better to do something like:

  setTimeout(function() { // Queue a task after a checkpoint
    setTimeout(function() { // Queue a task after notify unresolved promises task
      finishTest();
    }, 0);
  }, 0);

Without testing I think that might technically be correct in some of these tests, but it looks unnecessarily complex.

FWIW, I don't think anyone is writing web-platform-tests with this consideration in mind. So as more and more tests are being written in web-platform-tests it seems like we would have to continually mark them as flakey which is nearly equivalent to no test coverage.

[Joseph Pecoraro]

Another alternative is to not use timeouts and end tests when expected behavior is reached. But this has other issues:

  • An unsuccessful test is now a timeout, which is harder to debug
  • This doesn't catch potential issues if extra events were emitted that the test did not expect. Which could hide an implementation issue. (Some of these tests attempt to cover that situation by making sure event listeners are not fired in some cases).

[Alexey Proskuryakov]

> Another alternative is to not use timeouts and end tests when expected behavior is reached.

shouldBecomeEqualTo() is one way to do that that's used in the tests.

It is true that failing by timeout is harder to triage.

[youenn fablet]

This is a concern shared by other browsers and fixing wpt flakiness due to 100 ms timeouts happened and is happening.
There is no simple solution here, see wpt streams/piping/flow-control.js for a try at it.
Setting 5 second timeout seems to be ok for bots and allows giving hints at where tests might be timing out.

[Joseph Pecoraro]

Created attachment 308189 [details]
[PATCH] Proposed Fix

Fix for the HTMLMediaElement case. There might be some new test changes so this is another for bots.

[Build Bot]

Comment on attachment 308189 [details]
[PATCH] Proposed Fix

Attachment 308189 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3606823

New failing tests:
media/video-multiple-concurrent-playback.html
media/video-play-pause-events.html
media/video-played-collapse.html
http/tests/misc/acid3.html

[Build Bot]

Created attachment 308199 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308189 [details]
[PATCH] Proposed Fix

Attachment 308189 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3606792

New failing tests:
media/video-multiple-concurrent-playback.html
media/video-play-pause-events.html

[Build Bot]

Created attachment 308200 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

> New failing tests:
> media/video-multiple-concurrent-playback.html
> media/video-play-pause-events.html

Okay, I don't know how the current change could cause these, so again I'll have to look into more media tests.

[Joseph Pecoraro]

(In reply to Joseph Pecoraro from comment #176)
> > New failing tests:
> > media/video-multiple-concurrent-playback.html
> > media/video-play-pause-events.html
> 
> Okay, I don't know how the current change could cause these, so again I'll
> have to look into more media tests.

These both do not appear to be caused by the HTMLMediaElement changes in this patch.

[Build Bot]

Comment on attachment 308189 [details]
[PATCH] Proposed Fix

Attachment 308189 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3606837

New failing tests:
media/video-play-pause-events.html
media/video-multiple-concurrent-playback.html
media/video-played-collapse.html
media/video-played-reset.html

[Build Bot]

Created attachment 308208 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308189 [details]
[PATCH] Proposed Fix

Attachment 308189 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3607238

New failing tests:
media/video-play-pause-events.html
media/video-multiple-concurrent-playback.html
media/video-played-collapse.html

[Build Bot]

Created attachment 308214 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Created attachment 308308 [details]
[PATCH] Proposed Fix

Those tests were bogus updated results from an earlier patch. Reverted the bogus changes.

[Build Bot]

Comment on attachment 308308 [details]
[PATCH] Proposed Fix

Attachment 308308 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3615143

New failing tests:
media/video-played-reset.html

[Build Bot]

Created attachment 308320 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Created attachment 308325 [details]
[PATCH] Proposed Fix

[Saam Barati]

Comment on attachment 308308 [details]
[PATCH] Proposed Fix

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

Some preliminary comments. I'll continue to look later.

> Source/JavaScriptCore/builtins/PromiseOperations.js:117
> +        @hostPromiseRejectionTracker(promise, @promiseRejectionReject);

I wonder if this is worth doing only when we're debugging, and we could also create some mechanism to determine during bytecode generation time statically if something is true/false.

Could be something like this:
if (@isDebugging) // create some special debugging enabled AST node, and if statement could check for it
    ....

Anyways, I'm not tied to this, but it's less than ideal that we almost make this call even though the vast majority of users will never use web inspector.

> Source/JavaScriptCore/runtime/JSGlobalObject.h:268
> +    WriteBarrier<JSFunction> m_promiseResolveFunction;

This is unused.

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

Is this worth addressing?

> Source/WebCore/html/HTMLMediaElement.cpp:3300
> +        m_promiseTaskQueue.enqueueTask([this]() {

Is a raw |this| safe to capture here?

[Yusuke Suzuki]

Comment on attachment 308308 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/builtins/PromiseOperations.js:117
>> +        @hostPromiseRejectionTracker(promise, @promiseRejectionReject);
> 
> I wonder if this is worth doing only when we're debugging, and we could also create some mechanism to determine during bytecode generation time statically if something is true/false.
> 
> Could be something like this:
> if (@isDebugging) // create some special debugging enabled AST node, and if statement could check for it
>     ....
> 
> Anyways, I'm not tied to this, but it's less than ideal that we almost make this call even though the vast majority of users will never use web inspector.

Even if the inspector is not opened, unhandled rejection event is emitted. So, we can observe this behavior with the code `window.addEventListener('unhandledrejection', ..., false)`.
One idea I have is that implementing large part of them in JSC side and only invokes registered hanlder if some WebCore operation is necessary. If we can implement the system in JS,
we can optimize it easily in JSC side. (For example, if the remember set of rejected promises is implemented in JS WeakMap, we can optimize it if we optimize WeakMap operations in JSC).
But I think it's not a big deal right now.

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

Personally, if this bug will be fixed soon, I think separate bug is OK.
It involves reasonable amount of change to SourceOrigin to remember the origin of the executing code.
And maybe, separating security check system from CachedScript is also required.

[Joseph Pecoraro]

Comment on attachment 308308 [details]
[PATCH] Proposed Fix

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

Thanks for looking!

>>> Source/JavaScriptCore/builtins/PromiseOperations.js:117
>>> +        @hostPromiseRejectionTracker(promise, @promiseRejectionReject);
>> 
>> I wonder if this is worth doing only when we're debugging, and we could also create some mechanism to determine during bytecode generation time statically if something is true/false.
>> 
>> Could be something like this:
>> if (@isDebugging) // create some special debugging enabled AST node, and if statement could check for it
>>     ....
>> 
>> Anyways, I'm not tied to this, but it's less than ideal that we almost make this call even though the vast majority of users will never use web inspector.
> 
> Even if the inspector is not opened, unhandled rejection event is emitted. So, we can observe this behavior with the code `window.addEventListener('unhandledrejection', ..., false)`.
> One idea I have is that implementing large part of them in JSC side and only invokes registered hanlder if some WebCore operation is necessary. If we can implement the system in JS,
> we can optimize it easily in JSC side. (For example, if the remember set of rejected promises is implemented in JS WeakMap, we can optimize it if we optimize WeakMap operations in JSC).
> But I think it's not a big deal right now.

Correct, these operations need to always happen in web pages, and for pure JSContexts the hostPromiseRejectionTracker function will immediately bail. If making that call to a native function is expensive we could add an if (@hostHasPromiseRejectionTracker).

>>> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:410
>>> +    // FIXME: If script has muted errors (cross origin), terminate these steps.
>> 
>> Is this worth addressing?
> 
> Personally, if this bug will be fixed soon, I think separate bug is OK.
> It involves reasonable amount of change to SourceOrigin to remember the origin of the executing code.
> And maybe, separating security check system from CachedScript is also required.

This affects not only Promise Rejection but also window.onerror. Since we are not any worse than window.onerror I think its fine to handle separately.

FWIW we probably want Web Inspector to still track things even if the web exposed APIs aren't given them. So Cross Origin uncaught exceptions and unhandled promises should still show in Web Inspector even if they don't trigger onerror / onunhandledrejection.

>> Source/WebCore/html/HTMLMediaElement.cpp:3300
>> +        m_promiseTaskQueue.enqueueTask([this]() {
> 
> Is a raw |this| safe to capture here?

Yes, all these task queues get cleared if the media element is destroyed. There are many uses of different task queues capturing this in their lambdas so this is not a lone occurrence.

[Joseph Pecoraro]

Comment on attachment 308325 [details]
[PATCH] Proposed Fix

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

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

To Saam's point about only doing work where there is a debugger, we could avoid capturing a call stack here unless there is a debugger.

Then, promise rejections would have a smaller impact for users that never open developer tools.

[Joseph Pecoraro]

Created attachment 308417 [details]
[PATCH] Proposed Fix

[Saam Barati]

Comment on attachment 308417 [details]
[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.@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?

[Joseph Pecoraro]

Comment on attachment 308417 [details]
[PATCH] Proposed Fix

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

Thanks for the review!

>> Source/JavaScriptCore/builtins/PromisePrototype.js:61
>> +    this.@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.

The behavior is specced:
https://tc39.github.io/ecma262/#sec-performpromisethen

However ultimately it gets you what you want, since .then produces a new Promise which won't have a rejection handler!

So:

    <script>
    Promise.reject(1);
    Promise.reject(2).then(()=>{})
    Promise.resolve(99).then(() => { throw 3; });
    Promise.reject(99).catch(() => { throw 4; });
    </script>

Produces Unhandled Rejection messages for 1, 2, 3 and 4, because there was always some runtime exception that gets caught inside a promise without a reaction.

>> 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?

I'll file one.

>> Source/WebCore/dom/PromiseRejectionEvent.idl:29
>> +    Exposed=(Window,Worker),
> 
> Should this include Worker for right now?

Good catch! I had this half implemented and then pulled it out to work on a follow-up. I'll remove from here.

[Joseph Pecoraro]

*** Bug 124066 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

<https://trac.webkit.org/r215916>

[Lucas Forschler]

Mass move bugs into the DOM component.