Bug 150358 - Support for promise rejection events (unhandledrejection)
Summary: Support for promise rejection events (unhandledrejection)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
: 124066 (view as bug list)
Depends on: 166488 166925
Blocks:
  Show dependency treegraph
 
Reported: 2015-10-20 00:59 PDT by jochen
Modified: 2019-07-26 18:01 PDT (History)
19 users (show)

See Also:


Attachments
[PATCH] Work in Progress (64.71 KB, patch)
2016-11-28 11:11 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
Patch (82.87 KB, patch)
2017-01-05 15:06 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (82.94 KB, patch)
2017-01-05 15:32 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-elcapitan (1.21 MB, application/zip)
2017-01-05 16:51 PST, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (deleted)
2017-01-05 16:52 PST, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-elcapitan (541.54 KB, application/zip)
2017-01-05 16:57 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.35 MB, application/zip)
2017-01-05 16:58 PST, Build Bot
no flags Details
Patch (77.59 KB, patch)
2017-01-05 22:07 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (77.59 KB, patch)
2017-01-05 23:09 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (1.00 MB, application/zip)
2017-01-09 08:44 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1009.44 KB, application/zip)
2017-01-09 08:46 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (860.03 KB, application/zip)
2017-01-09 08:53 PST, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (872.55 KB, application/zip)
2017-01-09 11:06 PST, Build Bot
no flags Details
Patch (78.08 KB, patch)
2017-01-11 10:44 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (78.12 KB, patch)
2017-01-11 10:50 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (823.61 KB, application/zip)
2017-01-11 12:29 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (1.71 MB, application/zip)
2017-01-11 12:32 PST, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.33 MB, application/zip)
2017-01-11 12:43 PST, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (3.57 MB, application/zip)
2017-01-11 12:46 PST, Build Bot
no flags Details
Patch (78.33 KB, patch)
2017-01-12 05:20 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (808.96 KB, application/zip)
2017-01-12 06:28 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (871.62 KB, application/zip)
2017-01-12 06:37 PST, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (788.44 KB, application/zip)
2017-01-12 07:02 PST, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (881.06 KB, application/zip)
2017-01-12 07:22 PST, Build Bot
no flags Details
Patch (81.65 KB, patch)
2017-01-12 07:50 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (791.66 KB, application/zip)
2017-01-12 08:55 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (847.09 KB, application/zip)
2017-01-12 09:02 PST, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (820.49 KB, application/zip)
2017-01-12 09:11 PST, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.17 MB, application/zip)
2017-01-12 12:13 PST, Build Bot
no flags Details
Patch (80.83 KB, patch)
2017-02-20 00:14 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (85.25 KB, patch)
2017-02-20 01:41 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (88.94 KB, patch)
2017-02-20 02:08 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (1.04 MB, application/zip)
2017-02-20 03:19 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.17 MB, application/zip)
2017-02-20 03:28 PST, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-elcapitan (951.86 KB, application/zip)
2017-02-20 03:32 PST, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (7.11 MB, application/zip)
2017-02-20 03:36 PST, Build Bot
no flags Details
Patch (92.33 KB, patch)
2017-02-20 05:09 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1.01 MB, application/zip)
2017-02-20 06:16 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.02 MB, application/zip)
2017-02-20 06:25 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (964.28 KB, application/zip)
2017-02-20 06:30 PST, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (6.67 MB, application/zip)
2017-02-20 06:38 PST, Build Bot
no flags Details
Patch (145.66 KB, patch)
2017-03-09 11:12 PST, Yusuke Suzuki
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.15 MB, application/zip)
2017-03-09 12:49 PST, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (1.76 MB, application/zip)
2017-03-09 12:57 PST, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-elcapitan (842.16 KB, application/zip)
2017-03-09 13:03 PST, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (14.99 MB, application/zip)
2017-03-09 13:13 PST, Build Bot
no flags Details
[PATCH] Rebaselined - For Bots (142.50 KB, patch)
2017-04-19 22:26 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.16 MB, application/zip)
2017-04-19 23:59 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (1.57 MB, application/zip)
2017-04-20 00:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (50.44 MB, application/zip)
2017-04-20 00:50 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-elcapitan (875.91 KB, application/zip)
2017-04-20 04:18 PDT, Build Bot
no flags Details
[PATCH] Rebaselined - For Bots (144.86 KB, patch)
2017-04-20 12:45 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (912.00 KB, application/zip)
2017-04-20 14:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1000.53 KB, application/zip)
2017-04-20 14:38 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (1.84 MB, application/zip)
2017-04-20 15:08 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (14.89 MB, application/zip)
2017-04-20 15:11 PDT, Build Bot
no flags Details
[PATCH] Rebaselined - For Bots (146.78 KB, patch)
2017-04-20 17:44 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (887.34 KB, application/zip)
2017-04-20 19:06 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.14 MB, application/zip)
2017-04-20 19:14 PDT, Build Bot
no flags Details
[PATCH] Rebaselined - For Bots (150.41 KB, patch)
2017-04-20 19:18 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (938.37 KB, application/zip)
2017-04-20 20:45 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.16 MB, application/zip)
2017-04-20 20:46 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (1.84 MB, application/zip)
2017-04-20 21:00 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (168.36 KB, patch)
2017-04-20 21:04 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (958.13 KB, application/zip)
2017-04-20 22:30 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (889.42 KB, application/zip)
2017-04-20 22:34 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (1.84 MB, application/zip)
2017-04-20 23:01 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (11.37 MB, application/zip)
2017-04-21 00:58 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (168.80 KB, patch)
2017-04-21 17:46 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (995.49 KB, application/zip)
2017-04-21 19:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-elcapitan (938.02 KB, application/zip)
2017-04-21 19:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (20.53 MB, application/zip)
2017-04-21 20:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (1.81 MB, application/zip)
2017-04-22 00:53 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (173.66 KB, patch)
2017-04-25 19:16 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1.19 MB, application/zip)
2017-04-25 20:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.78 MB, application/zip)
2017-04-25 20:43 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (60.87 MB, application/zip)
2017-04-25 21:10 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (2.07 MB, application/zip)
2017-04-25 22:15 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (174.32 KB, patch)
2017-04-26 16:47 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (909.14 KB, application/zip)
2017-04-26 19:07 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (174.94 KB, patch)
2017-04-26 19:51 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (173.24 KB, patch)
2017-04-27 11:32 PDT, Joseph Pecoraro
sbarati: review+
sbarati: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2015-10-20 00:59:11 PDT
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
Comment 1 jochen 2015-10-20 00:59:41 PDT
Sam, could you please comment on this?
Comment 2 jochen 2015-11-30 06:26:55 PST
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
Comment 3 Richard Connamacher 2016-06-23 22:41:36 PDT
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.
Comment 4 Joseph Pecoraro 2016-09-22 22:21:35 PDT
How have I not heard about this until now!
Comment 5 Radar WebKit Bug Importer 2016-09-22 22:22:23 PDT
<rdar://problem/28441651>
Comment 6 Joseph Pecoraro 2016-11-28 11:11:56 PST
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")
Comment 7 Joseph Pecoraro 2016-11-28 16:13:42 PST
>     - 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)`.
Comment 8 Yusuke Suzuki 2016-12-07 05:38:08 PST
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.
Comment 9 Yusuke Suzuki 2017-01-02 14:22:31 PST
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.
Comment 10 Joseph Pecoraro 2017-01-03 18:53:26 PST
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.
Comment 11 Yusuke Suzuki 2017-01-03 23:29:50 PST
(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.
Comment 12 Yusuke Suzuki 2017-01-05 15:06:10 PST
Created attachment 298141 [details]
Patch

WIP: we need cross-origin check. This will be done by using extended SourceOrigin
Comment 13 Yusuke Suzuki 2017-01-05 15:12:23 PST
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.
Comment 14 Joseph Pecoraro 2017-01-05 15:16:23 PST
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});
Comment 15 Yusuke Suzuki 2017-01-05 15:29:48 PST
(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 })`
Comment 16 Yusuke Suzuki 2017-01-05 15:32:29 PST
Created attachment 298145 [details]
Patch

WIP: Fix linking error
Comment 17 Build Bot 2017-01-05 16:51:31 PST
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.
Comment 18 Build Bot 2017-01-05 16:51:34 PST
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
Comment 19 Build Bot 2017-01-05 16:51:56 PST
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.
Comment 20 Build Bot 2017-01-05 16:52:02 PST
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
Comment 21 Build Bot 2017-01-05 16:57:22 PST
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.
Comment 22 Build Bot 2017-01-05 16:57:26 PST
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
Comment 23 Build Bot 2017-01-05 16:58:38 PST
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.
Comment 24 Build Bot 2017-01-05 16:58:42 PST
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
Comment 25 Yusuke Suzuki 2017-01-05 22:07:17 PST
Created attachment 298175 [details]
Patch

WIP: Fix failures
Comment 26 Yusuke Suzuki 2017-01-05 23:09:23 PST
Created attachment 298177 [details]
Patch

WIP: Fix failures
Comment 27 Build Bot 2017-01-09 08:44:10 PST
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.
Comment 28 Build Bot 2017-01-09 08:44:15 PST
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
Comment 29 Build Bot 2017-01-09 08:46:52 PST
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.
Comment 30 Build Bot 2017-01-09 08:46:56 PST
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
Comment 31 Build Bot 2017-01-09 08:53:35 PST
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.
Comment 32 Build Bot 2017-01-09 08:53:40 PST
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
Comment 33 Build Bot 2017-01-09 11:06:16 PST
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.
Comment 34 Build Bot 2017-01-09 11:06:20 PST
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
Comment 35 Yusuke Suzuki 2017-01-10 21:04:40 PST
This cross origin check requires extended SourceOrigin support which I'm working on for dynamic import and worker modules.
Comment 36 Yusuke Suzuki 2017-01-11 10:44:41 PST
Created attachment 298600 [details]
Patch

WIP
Comment 37 Yusuke Suzuki 2017-01-11 10:50:15 PST
Created attachment 298601 [details]
Patch

WIP
Comment 38 Build Bot 2017-01-11 12:29:52 PST
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.
Comment 39 Build Bot 2017-01-11 12:29:57 PST
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
Comment 40 Build Bot 2017-01-11 12:32:46 PST
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.
Comment 41 Build Bot 2017-01-11 12:32:51 PST
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
Comment 42 Build Bot 2017-01-11 12:43:25 PST
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.
Comment 43 Build Bot 2017-01-11 12:43:29 PST
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
Comment 44 Build Bot 2017-01-11 12:46:09 PST
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.
Comment 45 Build Bot 2017-01-11 12:46:14 PST
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
Comment 46 Yusuke Suzuki 2017-01-12 05:20:03 PST
Created attachment 298671 [details]
Patch

WIP
Comment 47 Build Bot 2017-01-12 06:28:28 PST
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.
Comment 48 Build Bot 2017-01-12 06:28:33 PST
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
Comment 49 Build Bot 2017-01-12 06:37:40 PST
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.
Comment 50 Build Bot 2017-01-12 06:37:46 PST
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
Comment 51 Build Bot 2017-01-12 07:01:55 PST
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.
Comment 52 Build Bot 2017-01-12 07:02:02 PST
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
Comment 53 Build Bot 2017-01-12 07:22:03 PST
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.
Comment 54 Build Bot 2017-01-12 07:22:10 PST
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
Comment 55 Yusuke Suzuki 2017-01-12 07:50:45 PST
Created attachment 298682 [details]
Patch

WIP
Comment 56 Build Bot 2017-01-12 08:55:45 PST
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.
Comment 57 Build Bot 2017-01-12 08:55:51 PST
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
Comment 58 Build Bot 2017-01-12 09:01:59 PST
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.
Comment 59 Build Bot 2017-01-12 09:02:05 PST
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
Comment 60 Build Bot 2017-01-12 09:11:28 PST
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.
Comment 61 Build Bot 2017-01-12 09:11:34 PST
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
Comment 62 Joseph Pecoraro 2017-01-12 11:05:38 PST
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.
Comment 63 Build Bot 2017-01-12 12:13:48 PST
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.
Comment 64 Build Bot 2017-01-12 12:13:55 PST
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
Comment 65 Yusuke Suzuki 2017-02-20 00:14:28 PST
Created attachment 302120 [details]
Patch

rebaseline
Comment 66 Yusuke Suzuki 2017-02-20 01:41:34 PST
Created attachment 302127 [details]
Patch
Comment 67 Yusuke Suzuki 2017-02-20 02:00:55 PST
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.
Comment 68 Yusuke Suzuki 2017-02-20 02:08:42 PST
Created attachment 302129 [details]
Patch
Comment 69 Build Bot 2017-02-20 03:19:23 PST
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.
Comment 70 Build Bot 2017-02-20 03:19:28 PST
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
Comment 71 Build Bot 2017-02-20 03:27:56 PST
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.
Comment 72 Build Bot 2017-02-20 03:28:02 PST
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
Comment 73 Build Bot 2017-02-20 03:32:32 PST
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.
Comment 74 Build Bot 2017-02-20 03:32:37 PST
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
Comment 75 Build Bot 2017-02-20 03:36:31 PST
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.
Comment 76 Build Bot 2017-02-20 03:36:38 PST
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
Comment 77 Yusuke Suzuki 2017-02-20 03:36:54 PST
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.
Comment 78 Yusuke Suzuki 2017-02-20 05:09:50 PST
Created attachment 302137 [details]
Patch
Comment 79 Build Bot 2017-02-20 06:16:26 PST
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.
Comment 80 Build Bot 2017-02-20 06:16:32 PST
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
Comment 81 Build Bot 2017-02-20 06:25:13 PST
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.
Comment 82 Build Bot 2017-02-20 06:25:21 PST
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
Comment 83 Build Bot 2017-02-20 06:30:27 PST
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.
Comment 84 Build Bot 2017-02-20 06:30:33 PST
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
Comment 85 Build Bot 2017-02-20 06:38:03 PST
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.
Comment 86 Build Bot 2017-02-20 06:38:11 PST
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
Comment 87 Yusuke Suzuki 2017-03-09 08:16:18 PST
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...
Comment 88 Yusuke Suzuki 2017-03-09 11:12:04 PST
Created attachment 303937 [details]
Patch
Comment 89 Yusuke Suzuki 2017-03-09 12:29:58 PST
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.
Comment 90 Build Bot 2017-03-09 12:49:35 PST
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
Comment 91 Build Bot 2017-03-09 12:49:40 PST
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
Comment 92 Build Bot 2017-03-09 12:57:38 PST
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
Comment 93 Build Bot 2017-03-09 12:57:46 PST
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
Comment 94 Build Bot 2017-03-09 13:03:31 PST
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
Comment 95 Build Bot 2017-03-09 13:03:38 PST
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
Comment 96 Build Bot 2017-03-09 13:13:41 PST
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
Comment 97 Build Bot 2017-03-09 13:13:48 PST
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
Comment 98 Joseph Pecoraro 2017-04-18 15:52:43 PDT
I'm going to pick this up again!
Comment 99 Joseph Pecoraro 2017-04-18 15:54:08 PDT
> 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.
Comment 100 Joseph Pecoraro 2017-04-18 16:20:01 PDT
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).
Comment 101 Joseph Pecoraro 2017-04-18 20:52:48 PDT
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.
Comment 102 Joseph Pecoraro 2017-04-19 22:26:06 PDT
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.
Comment 103 Build Bot 2017-04-19 23:59:33 PDT
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
Comment 104 Build Bot 2017-04-19 23:59:36 PDT
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
Comment 105 Build Bot 2017-04-20 00:21:28 PDT
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
Comment 106 Build Bot 2017-04-20 00:21:30 PDT
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
Comment 107 Build Bot 2017-04-20 00:50:01 PDT
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
Comment 108 Build Bot 2017-04-20 00:50:06 PDT
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
Comment 109 Build Bot 2017-04-20 04:18:07 PDT
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
Comment 110 Build Bot 2017-04-20 04:18:09 PDT
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
Comment 111 Joseph Pecoraro 2017-04-20 12:45:38 PDT
Created attachment 307618 [details]
[PATCH] Rebaselined - For Bots
Comment 112 Joseph Pecoraro 2017-04-20 14:11:47 PDT
> 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
Comment 113 jochen 2017-04-20 14:20:54 PDT
btw, what's the different between the new tests you added and the wpt tests?
Comment 114 Joseph Pecoraro 2017-04-20 14:37:21 PDT
(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.
Comment 115 Build Bot 2017-04-20 14:37:40 PDT
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
Comment 116 Build Bot 2017-04-20 14:37:42 PDT
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
Comment 117 Build Bot 2017-04-20 14:38:41 PDT
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
Comment 118 Build Bot 2017-04-20 14:38:43 PDT
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
Comment 119 Joseph Pecoraro 2017-04-20 14:38:58 PDT
(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).
Comment 120 Joseph Pecoraro 2017-04-20 14:56:12 PDT
(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.
Comment 121 Build Bot 2017-04-20 15:08:57 PDT
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
Comment 122 Build Bot 2017-04-20 15:08:59 PDT
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
Comment 123 Build Bot 2017-04-20 15:11:07 PDT
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
Comment 124 Build Bot 2017-04-20 15:11:10 PDT
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
Comment 125 Joseph Pecoraro 2017-04-20 16:12:47 PDT
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.
Comment 126 Joseph Pecoraro 2017-04-20 17:44:39 PDT
Created attachment 307668 [details]
[PATCH] Rebaselined - For Bots
Comment 127 Joseph Pecoraro 2017-04-20 17:45:24 PDT
Pretty soon I'm going to start marking remaining tests as flakey if they continue to be flakey with this error: See Bug 171094.
Comment 128 Alexey Proskuryakov 2017-04-20 18:43:00 PDT
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.
Comment 129 Joseph Pecoraro 2017-04-20 18:55:04 PDT
(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.
Comment 130 Build Bot 2017-04-20 19:06:42 PDT
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
Comment 131 Build Bot 2017-04-20 19:06:44 PDT
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
Comment 132 Build Bot 2017-04-20 19:14:50 PDT
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
Comment 133 Build Bot 2017-04-20 19:14:51 PDT
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
Comment 134 Joseph Pecoraro 2017-04-20 19:18:24 PDT
Created attachment 307679 [details]
[PATCH] Rebaselined - For Bots

This should be a final round for the bots.
Comment 135 Build Bot 2017-04-20 20:45:31 PDT
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
Comment 136 Build Bot 2017-04-20 20:45:33 PDT
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
Comment 137 Build Bot 2017-04-20 20:46:10 PDT
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
Comment 138 Build Bot 2017-04-20 20:46:12 PDT
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
Comment 139 Build Bot 2017-04-20 21:00:51 PDT
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
Comment 140 Build Bot 2017-04-20 21:00:53 PDT
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
Comment 141 Joseph Pecoraro 2017-04-20 21:04:41 PDT
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
Comment 142 Ryosuke Niwa 2017-04-20 21:29:33 PDT
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?
Comment 143 Build Bot 2017-04-20 22:30:03 PDT
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
Comment 144 Build Bot 2017-04-20 22:30:05 PDT
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
Comment 145 Build Bot 2017-04-20 22:34:36 PDT
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
Comment 146 Build Bot 2017-04-20 22:34:38 PDT
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
Comment 147 Build Bot 2017-04-20 23:01:45 PDT
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
Comment 148 Build Bot 2017-04-20 23:01:47 PDT
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
Comment 149 Build Bot 2017-04-21 00:58:20 PDT
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
Comment 150 Build Bot 2017-04-21 00:58:22 PDT
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
Comment 151 Joseph Pecoraro 2017-04-21 17:44:19 PDT
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.
Comment 152 Joseph Pecoraro 2017-04-21 17:46:50 PDT
Created attachment 307850 [details]
[PATCH] Proposed Fix
Comment 153 Joseph Pecoraro 2017-04-21 19:15:59 PDT
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.
Comment 154 Build Bot 2017-04-21 19:25:31 PDT
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
Comment 155 Build Bot 2017-04-21 19:25:33 PDT
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
Comment 156 Build Bot 2017-04-21 19:26:59 PDT
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
Comment 157 Build Bot 2017-04-21 19:27:01 PDT
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
Comment 158 Build Bot 2017-04-21 20:21:09 PDT
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
Comment 159 Build Bot 2017-04-21 20:21:11 PDT
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
Comment 160 Build Bot 2017-04-22 00:53:20 PDT
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
Comment 161 Build Bot 2017-04-22 00:53:22 PDT
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
Comment 162 Yusuke Suzuki 2017-04-22 02:18:08 PDT
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.
Comment 163 Ryosuke Niwa 2017-04-22 12:07:04 PDT
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.
Comment 164 Joseph Pecoraro 2017-04-25 14:27:36 PDT
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.
Comment 165 Joseph Pecoraro 2017-04-25 15:20:34 PDT
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.
Comment 166 Alexey Proskuryakov 2017-04-25 15:29:15 PDT
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.
Comment 167 Joseph Pecoraro 2017-04-25 15:49:58 PDT
(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.
Comment 168 Joseph Pecoraro 2017-04-25 15:53:22 PDT
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).
Comment 169 Alexey Proskuryakov 2017-04-25 17:26:24 PDT
> 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.
Comment 170 youenn fablet 2017-04-25 17:38:29 PDT
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.
Comment 171 Joseph Pecoraro 2017-04-25 19:16:12 PDT
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.
Comment 172 Build Bot 2017-04-25 20:36:03 PDT
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
Comment 173 Build Bot 2017-04-25 20:36:05 PDT
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
Comment 174 Build Bot 2017-04-25 20:43:31 PDT
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
Comment 175 Build Bot 2017-04-25 20:43:34 PDT
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
Comment 176 Joseph Pecoraro 2017-04-25 20:53:03 PDT
> 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.
Comment 177 Joseph Pecoraro 2017-04-25 21:08:34 PDT
(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.
Comment 178 Build Bot 2017-04-25 21:10:08 PDT
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
Comment 179 Build Bot 2017-04-25 21:10:12 PDT
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
Comment 180 Build Bot 2017-04-25 22:15:32 PDT
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
Comment 181 Build Bot 2017-04-25 22:15:35 PDT
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
Comment 182 Joseph Pecoraro 2017-04-26 16:47:14 PDT
Created attachment 308308 [details]
[PATCH] Proposed Fix

Those tests were bogus updated results from an earlier patch. Reverted the bogus changes.
Comment 183 Build Bot 2017-04-26 19:07:02 PDT
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
Comment 184 Build Bot 2017-04-26 19:07:04 PDT
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
Comment 185 Joseph Pecoraro 2017-04-26 19:51:52 PDT
Created attachment 308325 [details]
[PATCH] Proposed Fix
Comment 186 Saam Barati 2017-04-26 20:06:41 PDT
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?
Comment 187 Yusuke Suzuki 2017-04-27 03:23:03 PDT
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.
Comment 188 Joseph Pecoraro 2017-04-27 11:23:36 PDT
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.
Comment 189 Joseph Pecoraro 2017-04-27 11:28:39 PDT
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.
Comment 190 Joseph Pecoraro 2017-04-27 11:32:02 PDT
Created attachment 308417 [details]
[PATCH] Proposed Fix
Comment 191 Saam Barati 2017-04-27 17:41:09 PDT
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?
Comment 192 Joseph Pecoraro 2017-04-27 20:17:00 PDT
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.
Comment 193 Joseph Pecoraro 2017-04-27 20:37:48 PDT
*** Bug 124066 has been marked as a duplicate of this bug. ***
Comment 194 Joseph Pecoraro 2017-04-27 20:38:08 PDT
<https://trac.webkit.org/r215916>
Comment 195 Lucas Forschler 2019-02-06 09:19:10 PST
Mass move bugs into the DOM component.