Closed Bug 624041 Opened 9 years ago Closed 9 years ago

enumerator should skip elements deleted via array_shift/array_reverse

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: igor, Assigned: dmandelin)

References

()

Details

(Whiteboard: softblocker, fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

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.
We have a method to delete a range of ids from iterators. That should happen here. Nice catch.
blocking2.0: beta8+ → betaN+
Whiteboard: softblocker
Assignee: general → dmandelin
Attached patch Patch (obsolete) — Splinter Review
Figured I'd bang this out since it's easy.
Attachment #502701 - Flags: review?(igor)
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.
Attachment #502701 - Flags: review?(igor) → review+
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
Status: NEW → ASSIGNED
Whiteboard: softblocker → softblocker, fixed-in-tracemonkey
Backed out, turned everything orange:

http://hg.mozilla.org/tracemonkey/rev/39bac86bb168
Whiteboard: softblocker, fixed-in-tracemonkey → softblocker
Attached patch Patch, v2Splinter Review
Bleh. Extra semicolon in the original. Currently running a fixed version on try.
Attachment #502701 - Attachment is obsolete: true
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.
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.
Attachment #502936 - Flags: review?(igor)
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.
Attachment #502936 - Flags: review?(igor) → review+
http://hg.mozilla.org/tracemonkey/rev/cd38397ec06e
Whiteboard: softblocker → softblocker, fixed-in-tracemonkey
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.