Closed Bug 624041 Opened 12 years ago Closed 12 years ago

enumerator should skip elements deleted via array_shift/array_reverse


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
blocking2.0 --- betaN+


(Reporter: igor, Assigned: dmandelin)




(Whiteboard: softblocker, fixed-in-tracemonkey)


(1 file, 1 obsolete file)

Consider the following example:

var a = [0, 1];
for (var i in a) {

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) {

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]

>+++ 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.
Whiteboard: softblocker → softblocker, fixed-in-tracemonkey
Backed out, turned everything orange:
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+
Whiteboard: softblocker → softblocker, fixed-in-tracemonkey
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.