Bug 197600 – Implement Promise.allSettled

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

Implement Promise.allSettled

[Dean Jackson]

https://github.com/tc39/proposal-promise-allSettled/

Shipping in Firefox since version 68.
Shipping in V8 since https://chromium.googlesource.com/v8/v8.git/+/1f6d27e8df819b448712dface6ad367fb8de426b

[Radar WebKit Bug Importer]

<rdar://problem/50483885>

[Radar WebKit Bug Importer]

<rdar://problem/50483886>

[Dean Jackson]

Created attachment 369095 [details]
WIP

[EWS Watchlist]

Comment on attachment 369095 [details]
WIP

Attachment 369095 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12107506

New failing tests:
http/tests/css/filters-on-iframes.html

[EWS Watchlist]

Created attachment 369096 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Dean Jackson]

Created attachment 369149 [details]
WIP 2

[Keith Miller]

Comment on attachment 369149 [details]
WIP 2

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

> Source/JavaScriptCore/builtins/PromiseConstructor.js:87
> +    return @Promise.all.@call(this, @Array.from(iterable, function (item) {

I don't think this is correct. You can't use properties of the private globals unless the spec says to do so since they can be changed by the client and thus can be observed and/or changed.

[EWS Watchlist]

Comment on attachment 369149 [details]
WIP 2

Attachment 369149 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12114097

New failing tests:
legacy-animation-engine/compositing/reflections/load-video-in-reflection.html

[EWS Watchlist]

Created attachment 369161 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Ross Kirsling]

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

[Yusuke Suzuki]

Comment on attachment 369149 [details]
WIP 2

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

Some early comment.

> Source/JavaScriptCore/builtins/PromiseConstructor.js:88
> +        var promiseCapability = @newPromiseCapability(outerThis);

You can just use arrow function if necessary. But seems that the spec requires manual iteration. So I think we should do here.

[Mathias Bynens]

(In reply to Ross Kirsling from comment #11)
> *** Bug 196332 has been marked as a duplicate of this bug. ***

Slightly off-topic, but why is it that whenever I file a WebKit bug, it ultimately gets closed as a duplicate of another bug that was filed *after* my initial report? This happened more than a few times already. Should I just give up?

[Saam Barati]

(In reply to Mathias Bynens from comment #13)
> (In reply to Ross Kirsling from comment #11)
> > *** Bug 196332 has been marked as a duplicate of this bug. ***
> 
> Slightly off-topic, but why is it that whenever I file a WebKit bug, it
> ultimately gets closed as a duplicate of another bug that was filed *after*
> my initial report? This happened more than a few times already. Should I
> just give up?

No, you should not give up.

We're not super picky about marking the first filed bug as the "original" bug. We usually just pick whichever bug is most convenient.

[Keith Miller]

(In reply to Saam Barati from comment #14)
> (In reply to Mathias Bynens from comment #13)
> > (In reply to Ross Kirsling from comment #11)
> > > *** Bug 196332 has been marked as a duplicate of this bug. ***
> > 
> > Slightly off-topic, but why is it that whenever I file a WebKit bug, it
> > ultimately gets closed as a duplicate of another bug that was filed *after*
> > my initial report? This happened more than a few times already. Should I
> > just give up?
> 
> No, you should not give up.
> 
> We're not super picky about marking the first filed bug as the "original"
> bug. We usually just pick whichever bug is most convenient.

I personally just use whichever bug I see first. I also sometimes forget to associate the bug with my patch so I end up creating a new bug and duping the old one.

[Dean Jackson]

Sorry Matthias. This was my fault. I didn't search for bugs before filing.

[Dean Jackson]

Created attachment 370789 [details]
Patch

[Keith Miller]

Comment on attachment 370789 [details]
Patch

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

r=me.

> Source/JavaScriptCore/builtins/PromiseConstructor.js:150
> +            promiseCapability.@resolve.@call(@undefined, values);

Why does this need to be passed undefined as the this value? Isn't @resolve an internal method? Seems like the this value shouldn't matter.

> Source/JavaScriptCore/builtins/PromiseConstructor.js:152
> +        promiseCapability.@reject.@call(@undefined, error);

Ditto.

> JSTests/test262/expectations.yaml:1789
> +test/intl402/Collator/ignore-invalid-unicode-ext-values.js:
> +  default: 'Test262Error: Locale en is affected by key co; value standard. Expected SameValue(ëenû, ëen-AUû) to be true'
> +  strict mode: 'Test262Error: Locale en is affected by key co; value standard. Expected SameValue(ëenû, ëen-AUû) to be true'

What? Does this test use promise.allSettled?

Was this already failing?

> JSTests/test262/expectations.yaml:1819
> +test/intl402/fallback-locales-are-supported.js:
> +  default: "Test262Error: Locale zh-Hans-CN is supported, but fallback zh-Hans isn't. Expected SameValue(ë-1û, ë-1û) to be false (Testing with Collator.)"
> +  strict mode: "Test262Error: Locale zh-Hans-CN is supported, but fallback zh-Hans isn't. Expected SameValue(ë-1û, ë-1û) to be false (Testing with Collator.)"

Ditto...

[Dean Jackson]

Comment on attachment 370789 [details]
Patch

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

>> Source/JavaScriptCore/builtins/PromiseConstructor.js:150
>> +            promiseCapability.@resolve.@call(@undefined, values);
> 
> Why does this need to be passed undefined as the this value? Isn't @resolve an internal method? Seems like the this value shouldn't matter.

From https://tc39.github.io/proposal-promise-allSettled/

14. If remainingElementsCount.[[Value]] is 0, then
    14.a Let valuesArray be ! CreateArrayFromList(values).
    14.b Return ? Call(promiseCapability.[[Resolve]], undefined, « valuesArray »).

I don't understand why :)

>> JSTests/test262/expectations.yaml:1789
>> +  strict mode: 'Test262Error: Locale en is affected by key co; value standard. Expected SameValue(ëenû, ëen-AUû) to be true'
> 
> What? Does this test use promise.allSettled?
> 
> Was this already failing?

I think it might be a NMOS thing.

Ooooh... it's actually an en-AU thing!!!! Curse you foreigners with your non USA locales!

[Dean Jackson]

Comment on attachment 370789 [details]
Patch

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

>>> JSTests/test262/expectations.yaml:1789
>>> +  strict mode: 'Test262Error: Locale en is affected by key co; value standard. Expected SameValue(ëenû, ëen-AUû) to be true'
>> 
>> What? Does this test use promise.allSettled?
>> 
>> Was this already failing?
> 
> I think it might be a NMOS thing.
> 
> Ooooh... it's actually an en-AU thing!!!! Curse you foreigners with your non USA locales!

Just realised that I forgot to add a :)

[Dean Jackson]

Committed r245869: <https://trac.webkit.org/changeset/245869>

[Keith Miller]

Comment on attachment 370789 [details]
Patch

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

>>>> JSTests/test262/expectations.yaml:1789
>>>> +  strict mode: 'Test262Error: Locale en is affected by key co; value standard. Expected SameValue(ëenû, ëen-AUû) to be true'
>>> 
>>> What? Does this test use promise.allSettled?
>>> 
>>> Was this already failing?
>> 
>> I think it might be a NMOS thing.
>> 
>> Ooooh... it's actually an en-AU thing!!!! Curse you foreigners with your non USA locales!
> 
> Just realised that I forgot to add a :)

How dare you have the audacity to test an Internationalization API with a non-USA system locale!

In all seriousness though this seems like an unfortunate bug... It's kinda weird that these results would depend on the locale though.