1021835 - for-of should throw TypeError if the @@iterator method returns a non-object value

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Spotted by André Bargull. See bug 904723 comment 28.

// Primitive values can't be iterators.
var a = {};
a[std_iterator] = () => 0;
Number.prototype.next = () => ({value: undefined, done: true});
assertThrowsInstanceOf(() => Array.from(a), TypeError);

Comment 1

[Tom Schuster [:evilpie]]

It seems like most code now uses either GetIterator (self-hosted) or JS::ForOfIterator (C++), both check for primitive return values.

So we really just need to emit some extra byte-code to disallow primitives in for-of.

Number.prototype.next = () => ({value: undefined, done: true});
for (let x of {[Symbol.iterator]() { return 0 }}) {}

Comment 2

[Tooru Fujisawa [:arai]]

will post patch after test

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Emit JSOP_CHECKISOBJ after GetIterator in byte code.

There are several places that does GetIterator, and separated patch for each

with Bytecode (Part 1)
  * array destructuring
  * spread
  * for-of
  * yield*

ForOfIterator::init (Part 2)
  * Promise.all
  * Promise.race

GetIterator self-hosted function (Part 3, Part 4)
  * Array.from
  * %TypedArray%.from
  * %TypedArray% ctor

Inlined for better error message with decompiling (Part 4)
  * Map ctor
  * Set ctor
  * WeakMap ctor
  * WeakSet ctor


in Part 1, added JSOP_CHECKISOBJ after emitIterator.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Do not convert iterable[@@iterator]() value to object in ForOfIterator::init.

in ForOfIterator::init, replaced ToObject with isObject+toObject, to avoid converting primitive to object.

then, to test Promise.all/race, moved helper functions from js/src/tests/ecma_7/AsyncFunctions/shell.js to js/src/tests/shell.js.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Use "iterator" instead of "iterable" in the error message for iterator.

in GetIterator, JSMSG_NOT_ITERABLE is used to report invalid iterator, that is wrong.
replaced it with JSMSG_NOT_ITERATOR, that says "iterator" instead of "iterable".

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Add testcases for primitive iterator.

just added testcases.

Comment 7

[Till Schneidereit [:till]]

Comment on attachment 8810011 [details] [diff] [review]
Part 2: Do not convert iterable[@@iterator]() value to object in ForOfIterator::init.

Review of attachment 8810011 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, r=me

Comment 8

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d27d2fec192e62ec05267147c60769b6379bf2e4
Bug 1021835 - Part 1: Emit JSOP_CHECKISOBJ after GetIterator in byte code. r=evilpie

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b186ff95a5ef9ff00237fa72b19a0aa2d620a6d
Bug 1021835 - Part 2: Do not convert iterable[@@iterator]() value to object in ForOfIterator::init. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/953d2b8f17036a81990d82eac213622191019b8e
Bug 1021835 - Part 3: Use "iterator" instead of "iterable" in the error message for iterator. r=evilpie

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b4af0b1626b70d331c0769a91faa3f72a876dc2
Bug 1021835 - Part 4: Add testcases for primitive iterator. r=evilpie

Comment 9

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e366a3e90d2f87c8466dd9edf19581029023659
Bug 1021835 - followup: Skip test that needs drainJobQueue in jsreftest. r=bustage

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d27d2fec192e
https://hg.mozilla.org/mozilla-central/rev/9b186ff95a5e
https://hg.mozilla.org/mozilla-central/rev/953d2b8f1703
https://hg.mozilla.org/mozilla-central/rev/7b4af0b1626b
https://hg.mozilla.org/mozilla-central/rev/0e366a3e90d2