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

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: jorendorff, Assigned: arai)

Tracking

(Blocks 1 bug, {dev-doc-complete, site-compat})

unspecified
mozilla51
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(7 attachments, 2 obsolete attachments)

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.
Reporter

Updated

5 years ago
Blocks: es6

Updated

5 years ago
Keywords: dev-doc-needed
Assignee

Comment 1

3 years ago
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.
Assignee: nobody → arai.unmht
See Also: → 1021835
Assignee

Comment 2

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

Comment 3

3 years ago
with WIP patches, there is 25% difference :/
Assignee

Comment 4

3 years ago
Added some more affected cases.
Assignee

Comment 5

3 years ago
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
Attachment #8767828 - Flags: review?(jorendorff)
Assignee

Comment 6

3 years ago
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?
Attachment #8767830 - Flags: feedback?(jorendorff)
Assignee

Comment 7

3 years ago
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.
Attachment #8767831 - Flags: review?(jorendorff)
Assignee

Comment 8

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

baseline part is very straight forward.
Assignee

Comment 9

3 years ago
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.
Assignee

Comment 10

3 years ago
and added testcase for returning non-object from iterator.next(), in several situations.
Attachment #8767835 - Flags: review?(jorendorff)
Reporter

Comment 11

3 years ago
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."
Attachment #8767828 - Flags: review?(jorendorff) → review+
Reporter

Comment 12

3 years ago
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...
Attachment #8767830 - Flags: feedback?(jorendorff)
Reporter

Updated

3 years ago
Attachment #8767831 - Flags: review?(jorendorff) → review+
Reporter

Updated

3 years ago
Attachment #8767835 - Flags: review?(jorendorff) → review+
Assignee

Comment 13

3 years ago
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.
Attachment #8767832 - Flags: review?(jdemooij)
Assignee

Comment 14

3 years ago
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.
Attachment #8767834 - Flags: review?(jdemooij)
Attachment #8767832 - Flags: review?(jdemooij) → review+
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.
Attachment #8767834 - Flags: review?(jdemooij)
Assignee

Comment 16

3 years ago
Thank you for reviewing :)
  * Removed LCheckIsObjT
  * Renamed LCheckIsObjV to LCheckIsObj
  * Changed LCheckIsObj to inherit from LInstructionHelper, and call oolCallVM in CodeGenerator
Attachment #8767830 - Attachment is obsolete: true
Attachment #8767834 - Attachment is obsolete: true
Attachment #8778862 - Flags: review?(jdemooij)
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();
    }
Attachment #8778862 - Flags: review?(jdemooij) → review+
Assignee

Comment 18

3 years ago
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
You need to log in before you can comment on or make changes to this bug.