Closed Bug 1016936 Opened 10 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jorendorff, Assigned: arai)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(7 files, 2 obsolete files)

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.
Blocks: es6
Keywords: dev-doc-needed
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
as suggested by Waldo in IRC, I'll try creating another patch that re-uses BytecodeEmitter::emitRequireObjectCoercible, and compare performance.
with WIP patches, there is 25% difference :/
Added some more affected cases.
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)
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)
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)
attaching JIT part for Plan A, without r? for now.

baseline part is very straight forward.
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.
and added testcase for returning non-object from iterator.next(), in several situations.
Attachment #8767835 - Flags: review?(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."
Attachment #8767828 - Flags: review?(jorendorff) → review+
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)
Attachment #8767831 - Flags: review?(jorendorff) → review+
Attachment #8767835 - Flags: review?(jorendorff) → review+
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)
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)
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+
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.