Bug 595963: notify iterators about property deletion in array_splice, r=gal

--HG--
extra : rebase_source : de2700e0d3d7bce1453a73155c569d28cfbd4482

js/src/jsarray.cpp

@@ -2338,6 +2338,7 @@ array_splice(JSContext *cx, uintN argc, Value *vp)
     JSObject *obj = ComputeThisFromVp(cx, vp);
     if (!obj || !js_GetLengthProperty(cx, obj, &length))
         return JS_FALSE;
+    jsuint origlength = length;
 
     /* Convert the first argument into a starting index. */
     jsdouble d;
@@ -2452,6 +2453,9 @@ array_splice(JSContext *cx, uintN argc, Value *vp)
         length -= delta;
     }
 
+    if (length < origlength && !js_SuppressDeletedIndexProperties(cx, obj, length, origlength))
+        return JS_FALSE;
+
     /*
      * Copy from argv into the hole to complete the splice, and update length in
      * case we deleted elements from the end.

js/src/jsiter.cpp

@@ -871,17 +871,25 @@ js_CloseIterator(JSContext *cx, JSObject *obj)
 }
 
 /*
- * Suppress enumeration of deleted properties. We maintain a list of all active
- * non-escaping for-in enumerators. Whenever a property is deleted, we check
- * whether any active enumerator contains the (obj, id) pair and has not
- * enumerated id yet. If so, we delete the id from the list (or advance the
- * cursor if it is the next id to be enumerated).
+ * Suppress enumeration of deleted properties. This function must be called
+ * when a property is deleted and there might be active enumerators. 
+ *
+ * We maintain a list of active non-escaping for-in enumerators. To suppress
+ * a property, we check whether each active enumerator contains the (obj, id)
+ * pair and has not yet enumerated |id|. If so, and |id| is the next property,
+ * we simply advance the cursor. Otherwise, we delete |id| from the list.
  *
  * We do not suppress enumeration of a property deleted along an object's
  * prototype chain. Only direct deletions on the object are handled.
+ *
+ * This function can suppress multiple properties at once. The |predicate|
+ * argument is an object which can be called on an id and returns true or
+ * false. It also must have a method |matchesAtMostOne| which allows us to
+ * stop searching after the first deletion if true.
  */
-bool
-js_SuppressDeletedProperty(JSContext *cx, JSObject *obj, jsid id)
+template<typename IdPredicate>
+static bool
+SuppressDeletedPropertyHelper(JSContext *cx, JSObject *obj, IdPredicate predicate)
 {
     JSObject *iterobj = cx->enumerators;
     while (iterobj) {
@@ -893,7 +901,7 @@ js_SuppressDeletedProperty(JSContext *cx, JSObject *obj, jsid id)
             jsid *props_cursor = ni->currentKey();
             jsid *props_end = ni->endKey();
             for (jsid *idp = props_cursor; idp < props_end; ++idp) {
-                if (*idp == id) {
+                if (predicate(*idp)) {
                     /*
                      * Check whether another property along the prototype chain
                      * became visible as a result of this deletion.
@@ -902,14 +910,14 @@ js_SuppressDeletedProperty(JSContext *cx, JSObject *obj, jsid id)
                         AutoObjectRooter proto(cx, obj->getProto());
                         AutoObjectRooter obj2(cx);
                         JSProperty *prop;
-                        if (!proto.object()->lookupProperty(cx, id, obj2.addr(), &prop))
+                        if (!proto.object()->lookupProperty(cx, *idp, obj2.addr(), &prop))
                             return false;
                         if (prop) {
                             uintN attrs;
                             if (obj2.object()->isNative()) {
                                 attrs = ((Shape *) prop)->attributes();
                                 JS_UNLOCK_OBJ(cx, obj2.object());
-                            } else if (!obj2.object()->getAttributes(cx, id, &attrs)) {
+                            } else if (!obj2.object()->getAttributes(cx, *idp, &attrs)) {
                                 return false;
                             }
                             if (attrs & JSPROP_ENUMERATE)
@@ -925,7 +933,7 @@ js_SuppressDeletedProperty(JSContext *cx, JSObject *obj, jsid id)
                         goto again;
 
                     /*
-                     * No property along the prototype chain steppeded in to take the
+                     * No property along the prototype chain stepped in to take the
                      * property's place, so go ahead and delete id from the list.
                      * If it is the next property to be enumerated, just skip it.
                      */
@@ -935,7 +943,8 @@ js_SuppressDeletedProperty(JSContext *cx, JSObject *obj, jsid id)
                         memmove(idp, idp + 1, (props_end - (idp + 1)) * sizeof(jsid));
                         ni->props_end = ni->endKey() - 1;
                     }
-                    break;
+                    if (predicate.matchesAtMostOne())
+                        break;
                 }
             }
         }
@@ -944,6 +953,38 @@ js_SuppressDeletedProperty(JSContext *cx, JSObject *obj, jsid id)
     return true;
 }
 
+class SingleIdPredicate {
+    jsid id;
+public:
+    SingleIdPredicate(jsid id) : id(id) {}
+
+    bool operator()(jsid id) { return id == this->id; }
+    bool matchesAtMostOne() { return true; }
+};
+
+bool
+js_SuppressDeletedProperty(JSContext *cx, JSObject *obj, jsid id)
+{
+    return SuppressDeletedPropertyHelper(cx, obj, SingleIdPredicate(id));
+}
+
+class IndexRangePredicate {
+    jsint begin, end;
+public:
+    IndexRangePredicate(jsint begin, jsint end) : begin(begin), end(end) {}
+
+    bool operator()(jsid id) { 
+        return JSID_IS_INT(id) && begin <= JSID_TO_INT(id) && JSID_TO_INT(id) < end;
+    }
+    bool matchesAtMostOne() { return false; }
+};
+
+bool
+js_SuppressDeletedIndexProperties(JSContext *cx, JSObject *obj, jsint begin, jsint end)
+{
+    return SuppressDeletedPropertyHelper(cx, obj, IndexRangePredicate(begin, end));
+}
+
 JSBool
 js_IteratorMore(JSContext *cx, JSObject *iterobj, Value *rval)
 {

js/src/jsiter.h

@@ -171,6 +171,9 @@ js_CloseIterator(JSContext *cx, JSObject *iterObj);
 bool
 js_SuppressDeletedProperty(JSContext *cx, JSObject *obj, jsid id);
 
+bool
+js_SuppressDeletedIndexProperties(JSContext *cx, JSObject *obj, jsint begin, jsint end);
+
 /*
  * IteratorMore() indicates whether another value is available. It might
  * internally call iterobj.next() and then cache the value until its

js/src/trace-test/tests/basic/bug595963-1.js

@@ -0,0 +1,19 @@
+function remove(k, L) {
+  for (var i in k) {
+    if (i == L)
+      k.splice(L, 1);
+  }
+}
+function f(k) {
+  var L = 0;
+  for (var i in k) {
+    if (L == 1)
+      remove(k, L);
+    L++;
+    assertEq(k[i], 3);
+  }
+  assertEq(L, 6);
+}
+
+var a = [3, 3, 3, 3, 3, 3, 3];
+f(a);

js/src/trace-test/tests/basic/bug595963-2.js

@@ -0,0 +1,19 @@
+function remove(k, L) {
+  for (var i in k) {
+    if (i == L)
+      k.splice(L, 3);
+  }
+}
+function f(k) {
+  var L = 0;
+  for (var i in k) {
+    if (L == 1)
+      remove(k, L);
+    L++;
+    assertEq(k[i], 3);
+  }
+  assertEq(L, 4);
+}
+
+var a = [3, 3, 3, 3, 3, 3, 3];
+f(a);