Bug 209124 – Promise.{all,allSettled,race} does not always close iterator

• Component: JavaScriptCore
• Status: RESOLVED WONTFIX

[Alexey Shvayka]

Test case:
  delete Promise.resolve;

  Promise.all({
    [Symbol.iterator]() { return this; },
    return() { console.log("iterator closed"); },
  });

Expected:
  Promise rejected with TypeError, "iterator closed" logged

Actual:
  Promise rejected with TypeError, no logs

ECMA262: https://tc39.es/ecma262/#sec-promise.all (step 6.a)
Test262: https://test262.report/browse/built-ins/Promise/all/invoke-resolve-get-error-close.js

[Alexey Shvayka]

Created attachment 393622 [details]
Patch

[Alexey Shvayka]

Created attachment 393648 [details]
Patch

Inline @getIteratorAndPromiseResolve and drop @isObject check.

[Ross Kirsling]

Comment on attachment 393648 [details]
Patch

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

> Source/JavaScriptCore/builtins/PromiseConstructor.js:64
> +            var promiseResolve = this.resolve;
> +            if (typeof promiseResolve !== "function")
> +                @throwTypeError("Promise.resolve is not a function");

I wish we could stick the try-catch for iterator.return under this `if` to avoid the rethrow, but I guess a this.resolve getter might throw in an arbitrary way...

> Source/JavaScriptCore/builtins/PromiseConstructor.js:67
> +                iterator.return();

Shouldn't we check whether iterator.return is undefined (per step 5)?

> Source/JavaScriptCore/builtins/PromiseConstructor.js:81
> +        var wrapper = {};
> +        wrapper.@@iterator = function() { return iterator; };
> +
> +        for (var value of wrapper) {

I wonder if this is really better than operating the iterator manually in a while loop?

[Keith Miller]

Imo, the spec is doing the wrong thing here... If I were a web-dev asked to polyfill this I would do what JSC does today. I think it makes more sense for the spec to do the intuitive thing unless we have a compelling reason otherwise. Also, FWIW, looking at test262 report at one point every engine failed this test... https://test262.report/browse/built-ins/Promise/all/invoke-resolve-get-error-close.js?date=2019-07-06.

[Keith Miller]

Comment on attachment 393648 [details]
Patch

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

>> Source/JavaScriptCore/builtins/PromiseConstructor.js:81
>> +        for (var value of wrapper) {
> 
> I wonder if this is really better than operating the iterator manually in a while loop?

It's definitely better once we start optimizing the builtin iterators as the front end knows what for-of does in a way it can't for manual iteration. Also getting the details of iterating right is a PITA with a while loop. Trust me... 🤮

[Alexey Shvayka]

(In reply to Ross Kirsling from comment #3)
> > Source/JavaScriptCore/builtins/PromiseConstructor.js:67
> > +                iterator.return();
> 
> Shouldn't we check whether iterator.return is undefined (per step 5)?

If `iterator.return` is null or undefined, calling it will throw a TypeError that will be ignored due to empty `catch` clause.

This patch would not bring 100% spec compliance though: `this.resolve` should be looked up before `iterator.next` (step 6 of https://tc39.es/ecma262/#sec-getiterator), not after.

I hope that spec change (https://github.com/tc39/ecma262/pull/1912) will be approved during next TC-39 meeting. Great job on the fix, Keith!

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #6)
> I hope that spec change (https://github.com/tc39/ecma262/pull/1912) will be
> approved during next TC-39 meeting. Great job on the fix, Keith!

The change has gained consensus and is already implemented in SpiderMonkey.
r262544 synced updated test262 coverage.

[Keith Miller]

(In reply to Alexey Shvayka from comment #7)
> (In reply to Alexey Shvayka from comment #6)
> > I hope that spec change (https://github.com/tc39/ecma262/pull/1912) will be
> > approved during next TC-39 meeting. Great job on the fix, Keith!
> 
> The change has gained consensus and is already implemented in SpiderMonkey.
> r262544 synced updated test262 coverage.

Oh you changed test262 for me, thanks a bunch! I was about to look into it now that TC-39 is done.