Closed Bug 1198701 Opened 4 years ago Closed 3 years ago

ArrayIterator gets length property after iteration has finished

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox43 --- affected
firefox48 --- fixed

People

(Reporter: anba, Assigned: gweng)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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
Blocks: es6
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.
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.
Flags: needinfo?(andrebargull)
(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.
Flags: needinfo?(andrebargull)
Thanks! I will update the patch and set review.
Assignee: nobody → 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.
Attached patch Patch ver.1 (obsolete) — Splinter Review
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.
Attachment #8729362 - Flags: review?(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.
Attachment #8729362 - Flags: review?(till) → review+
Attached patch Patch ver.2 (obsolete) — Splinter Review
Thanks for reviewing. I updated the patch and now re-run the try tasks.
Attachment #8729362 - Attachment is obsolete: true
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.
Keywords: checkin-needed
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.
It seems only HTMLAllCollection has this issue, since NodeList and ordinary Array work fine with the spread operator.
And the member function |Collection| looks return |nsContentList| as the real querying object when invoking |Length| and other methods.
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.
Maybe |js::SpreadCallOperation| is relevant to the case:

https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Interpreter.cpp#4560
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.
Attached patch Patch ver.3 (obsolete) — Splinter Review
Attachment #8732060 - Attachment is obsolete: true
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.
Attachment #8739623 - Flags: review?(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.
Attachment #8739623 - Flags: review?(till) → review+
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?
Flags: needinfo?(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.
Flags: needinfo?(till)
Attached patch Patch ver.3Splinter Review
Thanks. I now upload and mark it as check-in needed.
Attachment #8739623 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/f88bfa306282
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1264575
You need to log in before you can comment on or make changes to this bug.