1016936 - Iteration: throw if the value returned by iterator.next() is not an object

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Per ES6 draft rev 25 (2014 May 22) 7.4.3
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-iteratornext
Step 4 requires this check.

I don't know when this was added.

Comment 1

[Tooru Fujisawa [:arai]]

I'm about to fix this with following solution:

for native functions, I'll check if returned value is an object, and throw if not.
for JS code, I'll add an dedicated opcode to check if the top-most value is an object, and throw if not.

I believe the opcode can be shared with bug 1021835.

Comment 2

[Tooru Fujisawa [:arai]]

as suggested by Waldo in IRC, I'll try creating another patch that re-uses BytecodeEmitter::emitRequireObjectCoercible, and compare performance.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: simple performance comparison between self-hosted function and opcode

with WIP patches, there is 25% difference :/

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: performance comparison between self-hosted function and opcode

Added some more affected cases.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: (Plan A) Part 1: Throw if the value returned by iterator.next() is not an object.

Plan A ("opcode" in the graph)

(based on the patch in bug 1184922, but has almost no relation)

Added a new opcode JSOP_CHECKISOBJ that checks if the value on the stack is an object, and throws if not.
It has uint8_t operand, that is a kind of the check, it's reflected to the error message.
Currently there's only one kind, IteratorNext [1], but I'm about to add another one in bug 1021835 for GetIterator [2].
(maybe we could just store the message number, but it's uint16_t...)
JIT support is added in Part 2 and Part 3.

In native function, ForOfIterator handles it, so just added isObject check, replacing ToObject.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ccb98432027

[1] https://tc39.github.io/ecma262/#sec-iteratornext
[2] https://tc39.github.io/ecma262/#sec-getiterator

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: (Plan B) Part 1: Throw if the value returned by iterator.next() is not an object.

Plan B ("self-hosted" in the graph)

Added a new self-hosted function RequireIsObjectIteratorNext, that checks if the passed value is an object, and throws if not.

Splitted the core code of BytecodeEmitter::emitRequireObjectCoercible out to BytecodeEmitter::emitCallSelfHosted1AfterDup, and shared it with emitRequireObjectCoercible and emitRequireIsObjectIteratorNext.

native function implementation is almost same as Plan A.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=de02aa9c19ef


I'd like to go with Plan A for performance reason tho, can I have your opinion which way to go?

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 1.1: Fix existing tests to follow the behavior change in IteratorNext.

in any way, there are several testcases that doesn't match to the spec.
fixed them to avoid returning non-object from iterator.next(), or just changed to accept the exception thrown by IteratorNext.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: (Plan A) Part 2: Support JSOP_CHECKISOBJ in baseline.

attaching JIT part for Plan A, without r? for now.

baseline part is very straight forward.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: (Plan A) Part 3: Support JSOP_CHECKISOBJ in Ion.

Ion part is also mostly straightforward, but it has 2 LIR for MIRType::Value and others, as others don't need isObject check, but just throws.

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Add test for IteratorNext with non-object returned by iterator.next().

and added testcase for returning non-object from iterator.next(), in several situations.

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 8767828 [details] [diff] [review]
(Plan A) Part 1: Throw if the value returned by iterator.next() is not an object.

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

::: js/src/vm/Opcodes.h
@@ +220,5 @@
>       */ \
>      macro(JSOP_DUP2,      13, "dup2",       NULL,         1,  2,  4, JOF_BYTE) \
>      \
> +    /*
> +     * Check if the top value of the stack is an object, and throws if not.

"Checks that the top value on the stack is an object, and throws a TypeError if not. The operand 'kind' is used only to generate an appropriate error message."

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 8767830 [details] [diff] [review]
(Plan B) Part 1: Throw if the value returned by iterator.next() is not an object.

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

It's too bad this isn't as fast - I like it...

Comment 13

[Tooru Fujisawa [:arai]]

Comment on attachment 8767832 [details] [diff] [review]
(Plan A) Part 2: Support JSOP_CHECKISOBJ in baseline.

Thank you for reviewing :)
I'll go with Plan A.

Part 2 supports JSOP_CHECKISOBJ in baseline, it calls ThrowCheckIsObject (that throws the TypeError) if given value is not an Object.

Comment 14

[Tooru Fujisawa [:arai]]

Comment on attachment 8767834 [details] [diff] [review]
(Plan A) Part 3: Support JSOP_CHECKISOBJ in Ion.

Part3 supports JSOP_CHECKISOBJ in Ion.

If the type of the value is known to be Object, it becomes NOP.

otherwise, it uses MCheckIsObj instruction.
If the type of the value is known to be non-Object, it becomes LCheckIsObjT, that just throws with ThrowCheckIsObject.
If the type of the value is unknown ( MIRType::Value), it becomes LCheckIsObjV, that throws with ThrowCheckIsObject if the value is not an Object.

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8767834 [details] [diff] [review]
(Plan A) Part 3: Support JSOP_CHECKISOBJ in Ion.

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

::: js/src/jit/CodeGenerator.cpp
@@ +11580,5 @@
> +    ValueOperand checkValue = ToValue(ins, LCheckIsObjV::CheckValue);
> +    Label done;
> +    masm.branchTestObject(Assembler::Equal, checkValue, &done);
> +    pushArg(Imm32(ins->mir()->checkKind()));
> +    callVM(ThrowCheckIsObjectInfo, ins);

I think it would be nicer to make LCheckIsObj a non-call instruction (in LIR-shared.h), and then use oolCallVM here. The non-object case should be rare, so if we make it a non-call instruction we don't penalize the is-object path.

::: js/src/jit/Lowering.cpp
@@ +4515,5 @@
> +{
> +    MDefinition* checkVal = ins->checkValue();
> +    MOZ_ASSERT(checkVal->type() == MIRType::Value);
> +
> +    if (checkVal->type() == MIRType::Value) {

This will always be true because of the BoxInputsPolicy (and see the assert before this), so we can remove CheckIsObjT. The is-object check may be unnecessary if we know it's not an object, but it shouldn't matter as it's a slow path.

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Support JSOP_CHECKISOBJ in Ion.

Thank you for reviewing :)
  * Removed LCheckIsObjT
  * Renamed LCheckIsObjV to LCheckIsObj
  * Changed LCheckIsObj to inherit from LInstructionHelper, and call oolCallVM in CodeGenerator

Comment 17

[Jan de Mooij [:jandem]]

Comment on attachment 8778862 [details] [diff] [review]
Part 3: Support JSOP_CHECKISOBJ in Ion.

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

Looks good.

::: js/src/jit/IonBuilder.cpp
@@ +10809,5 @@
> +
> +    MCheckIsObj* check = MCheckIsObj::New(alloc(), current->pop(), kind);
> +    current->add(check);
> +    current->push(check);
> +    return resumeAfter(check);

You can remove the resumeAfter call (just return true), and add this to MCheckIsObj:

    AliasSet getAliasSet() const override {
        return AliasSet::None();
    }

Comment 18

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec5f0eea0b6c1fc4b50369ceadbc18972b599953
Bug 1016936 - Part 1: Throw if the value returned by iterator.next() is not an object. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdfe71b2abc66b2a875f49e10c5557c354748e77
Bug 1016936 - Part 2: Support JSOP_CHECKISOBJ in baseline. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/494760e727953aa80b28fb948ff1d8fc6196ba4b
Bug 1016936 - Part 3: Support JSOP_CHECKISOBJ in Ion. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/9286619fafa0907c79c0f4ac712ca09d10ddc4b5
Bug 1016936 - Part 4: Add test for IteratorNext with non-object returned by iterator.next(). r=jorendorff

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ec5f0eea0b6c
https://hg.mozilla.org/mozilla-central/rev/bdfe71b2abc6
https://hg.mozilla.org/mozilla-central/rev/494760e72795
https://hg.mozilla.org/mozilla-central/rev/9286619fafa0

Comment 20

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/retuning-non-object-value-by-iterator-next-will-throw/

Comment 21

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/51#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Iteration_protocols#The_iterator_protocol