1198701 - ArrayIterator gets length property after iteration has finished

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

Test case:
---
var array = {
  __proto__: Array.prototype,
  get length() {
    print("get length");
    return {
      valueOf() {
        print("valueOf");
        return 0;
      }
    };
  }
};
var it = array[Symbol.iterator]();
it.next();
it.next();
---

Expected: "get length" and "valueOf" printed once
Actual: "get length" and "valueOf" printed twice

Comment 1

[Greg Weng [:snowmantw][:gweng][:λ]]

I would like to take a look if nobody already took it.

And I've tested it again: on Chrome/Node.js it will print result correctly as STR described, and SpiderMonkey prints the line twice.

Comment 2

[Greg Weng [:snowmantw][:gweng][:λ]]

I've a WIP patch to fix it, which bypasses the operation to get the length if the `index` is already set as MAX_UINT32. This patch could generate the result as the STR expected. However, according to the ES6 spec:

http://www.ecma-international.org/ecma-262/6.0/#sec-%arrayiteratorprototype%.next

The step 9. indicates that it should:

    9. Else,
        a. Let len be ToLength(Get(a, "length")).

Before it compares the `index` and `len`:

    10. If index ≥ len, then...

Therefore, it looks the operation to call the getter is inevitable. I don't know if this means V8 implemented it incorrectly (which will invalidate this bug), or it's just a detail not in the spec so that we can make the path faster.

Comment 3

[Till Schneidereit [:till]]

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #2)
> Therefore, it looks the operation to call the getter is inevitable. I don't
> know if this means V8 implemented it incorrectly (which will invalidate this
> bug), or it's just a detail not in the spec so that we can make the path
> faster.

The steps you mention should be performed during the first call, but not the second:

First call:
- Steps 1-8 succeed normally.
- In step 9, the `length` getter is called, and the two lines are printed (the first when calling the getter itself, the second when implicitly calling `valueOf` by coercing the result with ToLength).
- In step 10, `index` and `len` are both 0, so steps a and b are executed.
- In step 10.a, [[IteratedObject]] is set to `undefined`.
- The remaining steps succeed normally.

Second call:
- Steps 1-4 succeed normally.
- `a` is undefined, so the algorithm is aborted in step 5.
- the end, without a second set of print statements called.

I haven't looked at our implementation, but it seems like we must either get step 10 or steps 4,5 wrong.

Comment 4

[Greg Weng [:snowmantw][:gweng][:λ]]

Thanks! I will update the patch and set review.

Comment 5

[Greg Weng [:snowmantw][:gweng][:λ]]

I've found our implementation corresponding to the Step 10:

https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/Array.js#734-740

doesn't

    "a. Set the value of the [[IteratedObject]] internal slot of O to undefined."

Instead of, it set the `[[ArrayIteratorNextIndex]]` to MAX_UNIT32. And that part has a FIXME about the bug 924058 (using "ToLength" instead). However, in the spec, `ToLength` is used for set the local variable `len`, not for the `[[ArrayIteratorNextIndex]]`. I may change that regards the intention of original implementation.

Comment 6

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch ver.1

Hi Till,

Sorry it's my first SpiderMonkey patch, so if I set the wrong reviewer please forgive me.

In the patch I modify the 'Array.js' according to the spec, but I'm not sure if to set the internal slot as `undefined` is good to the engine. I also remove the lines to set `ITERATOR_SLOT_NEXT_INDEX` because I think it would be meaningless after to set the whole internal slots as `undefined`.

The patch passed the Try test (with one known intermittent issue):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f736a09e82bc

and I'm waiting another round with the new testcase reported as STR.

Comment 7

[Greg Weng [:snowmantw][:gweng][:λ]]

This is the latest Try result with the test I added:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88723dfd424e&selectedJob=17933283

Comment 8

[Till Schneidereit [:till]]

Comment on attachment 8729362 [details] [diff] [review]
Patch ver.1

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

Thanks, this is much better. r=me with feedback addressed.

::: js/src/builtin/Array.js
@@ +717,2 @@
>  
>  

Please add a comment mentioning which spec algorithm this implements here, similar to those for other functions:
// ES6, 22.1.5.2.1

@@ +723,5 @@
>                              "ArrayIteratorNext");
>      }
> +
> +    // Step 4.
> +    //var a = UnsafeGetObjectFromReservedSlot(this, ITERATOR_SLOT_TARGET);

Please remove the disabled line.

@@ +728,5 @@
> +    var a = UnsafeGetReservedSlot(this, ITERATOR_SLOT_TARGET);
> +    var result = { value: undefined, done: false };
> +
> +    // Step 5.
> +    if ('undefined' === typeof a) {

Please switch the sides of this condition. At least in the JS engine, it's customary to have the thing you're testing on the left side.

But really, you can change this to `if (!a)`, because a will always be an object or falsy. Also, see comment below.

@@ +750,2 @@
>      if (index >= len) {
> +        UnsafeSetReservedSlot(this, ITERATOR_SLOT_TARGET, undefined);

I think it'd be slightly preferable to set this to `null` instead of `undefined`: IIRC, the JITs behave a little bit better with type sets that are object|null, not object|undefined.

::: js/src/tests/ecma_6/Array/iterator_next.js
@@ +15,5 @@
> +  it.next();
> +  it.next();
> +  if (typeof reportCompare === 'function') {
> +    reportCompare(1, lengthCalledTimes,
> +      'when an iterator get length zero, it shouldn\'t access it again');

Uber-nit: use double-quotes (or template strings) for he string if you want to use single-quotes inside it, instead of escaping.

Comment 9

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch ver.2

Thanks for reviewing. I updated the patch and now re-run the try tasks.

Comment 10

[Greg Weng [:snowmantw][:gweng][:λ]]

It passed try tasks with some build error (after retries it looks good):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=adcd72496902&selectedJob=18258075

Therefore I now set the check-in flag.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dbf70d71fe29

Comment 12

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2678b91f4165 for https://treeherder.mozilla.org/logviewer.html#?job_id=24206519&repo=mozilla-inbound

Comment 13

[Greg Weng [:snowmantw][:gweng][:λ]]

Okay I now switch back. First thing I'm reading now is:

https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.h#136

I try to find out the implementation of `document.all` to see what's happening.

Comment 14

[Greg Weng [:snowmantw][:gweng][:λ]]

And the test is from:

https://bugzilla.mozilla.org/show_bug.cgi?id=1211624

Comment 15

[Greg Weng [:snowmantw][:gweng][:λ]]

It seems only HTMLAllCollection has this issue, since NodeList and ordinary Array work fine with the spread operator.

Comment 16

[Greg Weng [:snowmantw][:gweng][:λ]]

And the member function |Collection| looks return |nsContentList| as the real querying object when invoking |Length| and other methods.

Comment 17

[Greg Weng [:snowmantw][:gweng][:λ]]

Since |document.all| still returns an healthy |HTMLAllCollection|, I think it happens only when trying to apply the spread operator on the type. I may need to figure out the implementation of spread operator.

Comment 18

[Greg Weng [:snowmantw][:gweng][:λ]]

Maybe |js::SpreadCallOperation| is relevant to the case:

https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Interpreter.cpp#4560

Comment 19

[Greg Weng [:snowmantw][:gweng][:λ]]

I've resolved the regression (finally). The error is because when iterating |HTMLAllCollection| via the spread operator, the internal slot is NOT |null|. So if I set the condition as `if(!a)`, it will make the function returns as the target has been iterated once. So I change the condition to `if(a === null)`, the regression will disappear.

The most time I spent is at the debugging and tracing. Although in the end I realized it is such a simple mistake, I still feel glad about the things I've learned, especially how to debug SpiderMonkey + DOM binding + Firefox.

Comment 20

[Greg Weng [:snowmantw][:gweng][:λ]]

The updated path passed try tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=81ea7274c24e

Comment 21

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch ver.3

Comment 22

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8739623 [details] [diff] [review]
Patch ver.3

Hi Till, I've updated the patch. And I set the review again because I'm not quite sure whether I need to set the review again for such regression. Thanks.

Comment 23

[Till Schneidereit [:till]]

Comment on attachment 8739623 [details] [diff] [review]
Patch ver.3

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

Great job tracking this down! Change looks good, but we should have a test that would've caught this regression. That probably should be an xpcshell test. r=me with that added.

::: js/src/builtin/Array.js
@@ +716,5 @@
>      return CreateArrayIteratorAt(obj, kind, 0);
>  }
>  
> +// ES6, 22.1.5.2.1
> +// https://tc39.github.io/ecma262/#sec-%arrayiteratorprototype%.next

Please use http://www.ecma-international.org/ecma-262/6.0/index.html#sec-%arrayiteratorprototype%.next here. The tc39.github.io links are for the current draft of ES2016, and this is an implementation of ES2015/ES6.

::: js/src/tests/ecma_6/Array/iterator_next.js
@@ +1,1 @@
> +function testIteratorNextGetLength() {

No need to wrap this code into a function - it can just be the top-level script.
Also, please rename this file to something more specific: it doesn't exhaustively test iterator next on arrays, so the name is misleading. Or, even better, add this test to the iterator_edge_cases.js test file, with a new comment
//Tests that calling |next| on an array iterator after iteration has finished
// doesn't get the array's |length| property.

Comment 24

[Greg Weng [:snowmantw][:gweng][:λ]]

Hi Till,

I've updated the patch, but I'm afraid that I didn't understand about the "xpcshell test". The case of |document.all| is protected by the test in

    dom/base/test/test_document.all_iteration.html

And it's why my first patch got backed out. So it seems that we already have a testcase for the specific failure. Did you mean to create a more general test for the possible one to set the internal slot as |undefined| rather than |null|? Or did you mean to create a test in SpiderMonkey rather than in DOM to test the similar case earlier?

Comment 25

[Till Schneidereit [:till]]

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #24)
> Hi Till,
> 
> I've updated the patch, but I'm afraid that I didn't understand about the
> "xpcshell test". The case of |document.all| is protected by the test in
> 
>     dom/base/test/test_document.all_iteration.html

My apologies, I wasn't thinking of that. You're entirely right, this is covered by existing tests, so just land the updated patch.

Comment 26

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch ver.3

Thanks. I now upload and mark it as check-in needed.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f88bfa306282

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/f88bfa306282