Check whether the iterator is callable in spread

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[HyeockJin Kim]

version: 13.1.2(15609.3.5.1.3)

Firefox:
 [...{}]
 Uncaught TypeError: ({}) is not iterable

Safari:
 [...{}]
 TypeError: undefined is not a function

In safari, I think the error message is completely wrong.
The message `is not iterable` should be displayed, not `is not a function`.

If the issue hasn't been fixed yet, can I try this issue?

[Ross Kirsling]

(In reply to HyeockJin Kim from comment #0)
> In safari, I think the error message is completely wrong.
> The message `is not iterable` should be displayed, not `is not a function`.

The "undefined function" is specifically Symbol.iterator, but I agree that the current error message is a really unhelpful way of expressing the problem.

> If the issue hasn't been fixed yet, can I try this issue?

Sure!

[Radar WebKit Bug Importer]

<rdar://problem/68385305>

[HyeockJin Kim]

Created attachment 408094 [details]
Patch

[HyeockJin Kim]

Created attachment 408595 [details]
Patch

[HyeockJin Kim]

Created attachment 408597 [details]
Patch

[Darin Adler]

Comment on attachment 408597 [details]
Patch

WebKit project requires tests when you fix a bug. So we need to add a new test alongside this fix, showing the new correct behavior.

[Alexey Shvayka]

Comment on attachment 408597 [details]
Patch

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

Thank you for submitting the patch. I've left a comment on the approach.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1577
> +        if (UNLIKELY(scope.exception())) {

We can't override an exception as it won't necessarily be an unhelpful "undefined is not a function" one; it could be a userland-thrown error.
Instead, we should fix `iterationFunction`, which is defined in Source/JavaScriptCore/builtins/IteratorHelpers.js.
While we do that, we need to make sure Symbol.iterator is looked up only once.

[Darin Adler]

Comment on attachment 408597 [details]
Patch

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

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1577
>> +        if (UNLIKELY(scope.exception())) {
> 
> We can't override an exception as it won't necessarily be an unhelpful "undefined is not a function" one; it could be a userland-thrown error.
> Instead, we should fix `iterationFunction`, which is defined in Source/JavaScriptCore/builtins/IteratorHelpers.js.
> While we do that, we need to make sure Symbol.iterator is looked up only once.

So it seems we need multiple tests to show our correct handling of these different cases.

[HyeockJin Kim]

Created attachment 408724 [details]
Patch

[Darin Adler]

Comment on attachment 408724 [details]
Patch

Can we also make the test cover the "user defined error is not turned into a type error" case that the earlier patch got wrong? I know that’s definitely not wrong with this patch, but it seems we would want to be thorough.

[HyeockJin Kim]

> "user defined error is not turned into a type error" case 

I don't know how to write the above test in Webkit.
Can you explain a little more?

[Alexey Shvayka]

Comment on attachment 408724 [details]
Patch

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

> Source/JavaScriptCore/builtins/IteratorHelpers.js:35
> +    if (@isUndefinedOrNull(iterable) || @isUndefinedOrNull(iterable.@@iterator))

Please note that Get(iterable, @@iterator) is performed twice.
It is observable if `iterable` is a Proxy or @@iterator is a getter.

> Source/JavaScriptCore/builtins/IteratorHelpers.js:36
> +        @throwTypeError(`${iterable} is not iterable`);

Please note we are calling ToString on (likely) non-primitive, which is also observable.
The spec (https://tc39.es/ecma262/#sec-getiterator) doesn't have such call.

Instead, let's do this:

```
if (@isUndefinedOrNull(iterable))
    @throwTypeError("Spread syntax requires ...iterable not be null or undefined");

var iteratorMethod = iterable.@@iterator;
if (!@isCallable(iteratorMethod))
    @throwTypeError("Spread syntax requires ...iterable[Symbol.iterator] to be a function");

var iterator = iteratorMethod.@call(iterable);
```

Error messages I am suggesting are similar to ones we have in Source/JavaScriptCore/builtins/ArrayConstructor.js/from.

[Darin Adler]

I think we need an object where the iteration throws an exception. Maybe an object that throws an exception from valueOf?

[Alexey Shvayka]

(In reply to HyeockJin Kim from comment #11)
> > "user defined error is not turned into a type error" case 
> 
> I don't know how to write the above test in Webkit.
> Can you explain a little more?

```
const myErr = new TypeError();
shouldThrowExactly(() => [...{get [Symbol.iterator]() { throw myErr; }}], myErr);
```

[HyeockJin Kim]

Created attachment 408779 [details]
Patch

[Alexey Shvayka]

(In reply to HyeockJin Kim from comment #15)
> Created attachment 408779 [details]
> Patch

This looks great, thank you for taking the feedback into account.

Two minor nits before we land:
* Lets rename the bug & ChangeLog entries to a more descriptive name. Technically, the error message was not wrong (it was just bad).
* Lets rename the test to drop the "watchpoint" bit since it's not related to ArrayIterator watchpoints.

Please resubmit the patch with cq? via --request-commit so a WebKit reviewer will land the patch after reviewing.

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #16)
> * Lets rename the bug & ChangeLog entries to a more descriptive name.
> Technically, the error message was not wrong (it was just bad).

If you don't have Bugzilla rights to do so, please rename only in patch, I will adjust the bug.

[HyeockJin Kim]

Created attachment 408789 [details]
Patch

[Darin Adler]

Comment on attachment 408789 [details]
Patch

I appreciated all the effort that has gone into this, but I think we still need more tests.

Alexey noted that Get twice and toString are both observable. We need tests that cover those and prove that we do Get once and toString zero times.

[HyeockJin Kim]

Created attachment 408835 [details]
Patch

[HyeockJin Kim]

I tried to count when calling toString and iterator.
Are there more tests to be added?

[Darin Adler]

Comment on attachment 408835 [details]
Patch

Looks good. I don’t want to set commit-queue+ until we see if jsc-armv7-tests fails again.

[Alexey Shvayka]

Comment on attachment 408835 [details]
Patch

(In reply to Darin Adler from comment #22)
> Comment on attachment 408835 [details]
> Patch
> 
> Looks good. I don’t want to set commit-queue+ until we see if
> jsc-armv7-tests fails again.

cq+ since EWS is all-green.

Congratulations on your first patch landed!

[EWS]

Committed r267125: <https://trac.webkit.org/changeset/267125>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408835 [details].

[Saam Barati]

Do these extra checks cost us any perf?

[Alexey Shvayka]

(In reply to Saam Barati from comment #25)
> Do these extra checks cost us any perf?

No, the checks themselves are quick and, more importantly, they are on the slow path (!isJSArray) of the slow paths (slow_path_spread, operationSpreadGeneric).

[Saam Barati]

(In reply to Alexey Shvayka from comment #26)
> (In reply to Saam Barati from comment #25)
> > Do these extra checks cost us any perf?
> 
> No, the checks themselves are quick and, more importantly, they are on the
> slow path (!isJSArray) of the slow paths (slow_path_spread,
> operationSpreadGeneric).

Iterating something that's not an array is not the slow path. We want spread to be fast over all types.