624041 - enumerator should skip elements deleted via array_shift/array_reverse

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Consider the following example:

var a = [0, 1];
for (var i in a) {
    print(i);
    a.shift();
}

Currently it prints 0 and 1 because the enumerator does not see elements deleted by the shift method. 

A similar issue affects the reverse method:

var a = [, 0, 1];
for (var i in a) {
    a.reverse();
    print(i);
}

Again, the test case prints 1,2 as the enumerator does not see the delete operation (effectively the hole movements) done by the reverse method.

This is similar to the bug 595963 where the issue was with array_splice.

Comment 1

[Andreas Gal :gal]

We have a method to delete a range of ids from iterators. That should happen here. Nice catch.

Comment 2

[David Mandelin [:dmandelin]]

Attachment: Patch

Figured I'd bang this out since it's easy.

Comment 3

[Igor Bukanov]

Comment on attachment 502701 [details] [diff] [review]
Patch

>+++ b/js/src/jit-test/tests/basic/bug624041-1.js
>@@ -0,0 +1,6 @@
>+var a = [0, 1];
>+for (var i in a) {
>+    print(i);
>+    a.shift();
>+}

The test case does not crash so the print is not that useful for automated regressions testing ;) Instead it is necessary to assert here either the number of iterations is one or the expected index set.

>+++ b/js/src/jit-test/tests/basic/bug624041-2.js
>@@ -0,0 +1,6 @@
>+var a = [, 0, 1];
>+for (var i in a) {
>+    a.reverse();
>+    print(i);
>+}

Same here.

Comment 4

[David Mandelin [:dmandelin]]

Gah. My brain is pretty fried. Thanks for catching that. In fixing the 'shift' test case I found some funny behavior with one version of it. I'll check into that and file later if there is a bug.

http://hg.mozilla.org/tracemonkey/rev/72ac46e9f64e

Comment 5

[Jeff Walden [:Waldo]]

Backed out, turned everything orange:

http://hg.mozilla.org/tracemonkey/rev/39bac86bb168

Comment 6

[David Mandelin [:dmandelin]]

Attachment: Patch, v2

Bleh. Extra semicolon in the original. Currently running a fixed version on try.

Comment 7

[David Mandelin [:dmandelin]]

The new patch has 2 xpcshell test failures. I'll try to look at them soon but for now I'm working on the more important bug 599854.

Comment 8

[David Mandelin [:dmandelin]]

Locally, the patch gets only 1 xpcshell-test failure, and I get the same failure without the patch, so I guess it actually looks good. Also, the point of the failure seems to have nothing to do with arrays.

Comment 9

[Igor Bukanov]

Comment on attachment 502936 [details] [diff] [review]
Patch, v2

>@@ -1478,28 +1478,34 @@ array_reverse(JSContext *cx, uintN argc,
>+        bool ok = true;
>         for (; lo < hi; lo++, hi--) {
>+            Value origlo = obj->getDenseArrayElement(lo);
>+            Value orighi = obj->getDenseArrayElement(hi);
>+            obj->setDenseArrayElement(lo, orighi);
>+            if (orighi.isMagic(JS_ARRAY_HOLE))
>+                ok = js_SuppressDeletedProperty(cx, obj, INT_TO_JSID(lo)) || ok;


I should have seen the missed OOM check in the prev patch. But here we have another problem: the code should not continue looping after the first OOM and return immediately. 


>+            obj->setDenseArrayElement(hi, origlo);
>+            if (origlo.isMagic(JS_ARRAY_HOLE))
>+                ok = js_SuppressDeletedProperty(cx, obj, INT_TO_JSID(hi)) || ok;
>+            }

Nit: bad indent for }.

r+ with that fixed.

Comment 10

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/tracemonkey/rev/cd38397ec06e

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/cd38397ec06e