ArrayIterator gets length property after iteration has finished

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: anba, Assigned: gweng)

Tracking

(Blocks: 1 bug)

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 affected, firefox48 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
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
(Reporter)

Updated

3 years ago
Blocks: 694100
(Assignee)

Comment 1

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

Comment 2

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

Comment 4

3 years ago
Thanks! I will update the patch and set review.
(Assignee)

Updated

3 years ago
Assignee: nobody → gweng
(Assignee)

Comment 5

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

Comment 6

3 years ago
Created attachment 8729362 [details] [diff] [review]
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.
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+
(Assignee)

Comment 9

3 years ago
Created attachment 8732060 [details] [diff] [review]
Patch ver.2

Thanks for reviewing. I updated the patch and now re-run the try tasks.
Attachment #8729362 - Attachment is obsolete: true
(Assignee)

Comment 10

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

Comment 13

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

Comment 15

3 years ago
It seems only HTMLAllCollection has this issue, since NodeList and ordinary Array work fine with the spread operator.
(Assignee)

Comment 16

3 years ago
And the member function |Collection| looks return |nsContentList| as the real querying object when invoking |Length| and other methods.
(Assignee)

Comment 17

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

Comment 18

3 years ago
Maybe |js::SpreadCallOperation| is relevant to the case:

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

Comment 19

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

Comment 21

3 years ago
Created attachment 8739623 [details] [diff] [review]
Patch ver.3
Attachment #8732060 - Attachment is obsolete: true
(Assignee)

Comment 22

3 years ago
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+
(Assignee)

Comment 24

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

Comment 26

3 years ago
Created attachment 8740730 [details] [diff] [review]
Patch ver.3

Thanks. I now upload and mark it as check-in needed.
Attachment #8739623 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 28

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f88bfa306282
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1264575
You need to log in before you can comment on or make changes to this bug.