Bug 631723 - Make js_UpdateWatchpointsForShape return the new shape (and fix a few coding style nits). r=jimb.

--HG--
extra : rebase_source : c09fac13e674d0317f4edd7d64e5938c68c0e28a

js/src/jit-test/tests/jaeger/bug630366.js

@@ -0,0 +1,7 @@
+var o = {};
+for(var i=0; i<5; i++) {
+    o.p = 2;
+    o.watch("p", function() { });
+    o.p = 2;
+    delete o.p;
+}

js/src/jsdbgapi.cpp

@@ -825,23 +825,23 @@ WrapWatchedSetter(JSContext *cx, jsid id, uintN attrs, StrictPropertyOp setter)
     return CastAsStrictPropertyOp(FUN_OBJECT(wrapper));
 }
 
-static bool
-UpdateWatchpointShape(JSContext *cx, JSWatchPoint *wp, const js::Shape *newShape)
+static const Shape *
+UpdateWatchpointShape(JSContext *cx, JSWatchPoint *wp, const Shape *newShape)
 {
     JS_ASSERT_IF(wp->shape, wp->shape->id == newShape->id);
     JS_ASSERT(!IsWatchedProperty(cx, newShape));
 
     /* Create a watching setter we can substitute for the new shape's setter. */
-    js::StrictPropertyOp watchingSetter = 
+    StrictPropertyOp watchingSetter =
         WrapWatchedSetter(cx, newShape->id, newShape->attributes(), newShape->setter());
     if (!watchingSetter)
-        return false;
+        return NULL;
 
     /*
      * Save the shape's setter; we don't know whether js_ChangeNativePropertyAttrs will
      * return a new shape, or mutate this one.
      */
-    js::StrictPropertyOp originalSetter = newShape->setter();
+    StrictPropertyOp originalSetter = newShape->setter();
 
     /*
      * Drop the watching setter into the object, in place of newShape. Note that a single
@@ -849,21 +849,21 @@ UpdateWatchpointShape(JSContext *cx, JSWatchPoint *wp, const js::Shape *newShape
      * wrap all (JSPropertyOp, not JSObject *) setters with js_watch_set, so shapes that
      * differ only in their setter may all get wrapped to the same shape.
      */
-    const js::Shape *watchingShape = 
+    const Shape *watchingShape = 
         js_ChangeNativePropertyAttrs(cx, wp->object, newShape, 0, newShape->attributes(),
                                      newShape->getter(), watchingSetter);
     if (!watchingShape)
-        return false;
+        return NULL;
 
     /* Update the watchpoint with the new shape and its original setter. */
     wp->setter = originalSetter;
     wp->shape = watchingShape;
 
-    return true;
+    return watchingShape;
 }
 
-bool
-js_SlowPathUpdateWatchpointsForShape(JSContext *cx, JSObject *obj, const js::Shape *newShape)
+const Shape *
+js_SlowPathUpdateWatchpointsForShape(JSContext *cx, JSObject *obj, const Shape *newShape)
 {
     /*
      * The watchpoint code uses the normal property-modification functions to install its
@@ -873,11 +873,11 @@ js_SlowPathUpdateWatchpointsForShape(JSContext *cx, JSObject *obj, const js::Sha
      * proceed without interference.
      */
     if (IsWatchedProperty(cx, newShape))
-        return true;
+        return newShape;
 
     JSWatchPoint *wp = FindWatchPoint(cx->runtime, obj, newShape->id);
     if (!wp)
-        return true;
+        return newShape;
 
     return UpdateWatchpointShape(cx, wp, newShape);
 }

js/src/jsdbgapiinlines.h

@@ -46,19 +46,19 @@
 
 #if defined(JS_HAS_OBJ_WATCHPOINT) && defined(__cplusplus)
 
-extern bool
+extern const js::Shape *
 js_SlowPathUpdateWatchpointsForShape(JSContext *cx, JSObject *obj, const js::Shape *newShape);
 
 /*
- * Update any watchpoints on |obj| on |new_shape->id| to use |new_shape|. Property-manipulating
+ * Update any watchpoints on |obj| on |newShape->id| to use |newShape|. Property-manipulating
  * functions must call this any time it takes on a new shape to represent a potentially
  * watched property, or when it mutates a shape's attributes/setter/getter.
  */
-static inline bool
+static inline const js::Shape *
 js_UpdateWatchpointsForShape(JSContext *cx, JSObject *obj, const js::Shape *newShape)
 {
     if (JS_CLIST_IS_EMPTY(&cx->runtime->watchPointList))
-        return true;
+        return newShape;
 
     return js_SlowPathUpdateWatchpointsForShape(cx, obj, newShape);
 }

js/src/jsscope.cpp

@@ -768,10 +768,9 @@ JSObject::addProperty(JSContext *cx, jsid id,
         return NULL;
 
     /* Update any watchpoints referring to this property. */
-    if (!js_UpdateWatchpointsForShape(cx, this, shape)) {
+    shape = js_UpdateWatchpointsForShape(cx, this, shape);
+    if (!shape)
         METER(wrapWatchFails);
-        return NULL;
-    }
 
     return shape;
 }
@@ -896,13 +895,14 @@ JSObject::putProperty(JSContext *cx, jsid id,
             return NULL;
         }
 
-        const Shape *new_shape =
+        const Shape *newShape =
             addPropertyInternal(cx, id, getter, setter, slot, attrs, flags, shortid, spp);
-        if (!js_UpdateWatchpointsForShape(cx, this, new_shape)) {
-            METER(wrapWatchFails);
+        if (!newShape)
             return NULL;
-        }
-        return new_shape;
+        newShape = js_UpdateWatchpointsForShape(cx, this, newShape);
+        if (!newShape)
+            METER(wrapWatchFails);
+        return newShape;
     }
 
     /* Property exists: search must have returned a valid *spp. */
@@ -1038,12 +1038,10 @@ JSObject::putProperty(JSContext *cx, jsid id,
     CHECK_SHAPE_CONSISTENCY(this);
     METER(puts);
 
-    if (!js_UpdateWatchpointsForShape(cx, this, shape)) {
+    const Shape *newShape = js_UpdateWatchpointsForShape(cx, this, shape);
+    if (!newShape)
         METER(wrapWatchFails);
-        return NULL;
-    }
-
-    return shape;
+    return newShape;
 }
 
 const Shape *
@@ -1109,11 +1107,12 @@ JSObject::changeProperty(JSContext *cx, const Shape *shape, uintN attrs, uintN m
         lastProp->shape = js_GenerateShape(cx);
         clearOwnShape();
 
-        if (!js_UpdateWatchpointsForShape(cx, this, shape)) {
+        shape = js_UpdateWatchpointsForShape(cx, this, shape);
+        if (!shape) {
             METER(wrapWatchFails);
             return NULL;
         }
-
+        JS_ASSERT(shape == mutableShape);
         newShape = mutableShape;
     } else if (shape == lastProp) {
         Shape child(shape->id, getter, setter, shape->slot, attrs, shape->flags, shape->shortid);

js/src/tests/js1_8_5/extensions/jstests.list

@@ -25,3 +25,4 @@ skip-if(!xulRuntime.shell) script clone-errors.js
 skip-if(!xulRuntime.shell) script clone-forge.js
 script set-property-non-extensible.js
 script recursion.js
+script regress-631723.js

js/src/tests/js1_8_5/extensions/regress-631723.js

@@ -0,0 +1,10 @@
+// Any copyright is dedicated to the Public Domain.
+// http://creativecommons.org/licenses/publicdomain/
+
+var o = {a:1, b:2};
+o.watch("p", function() { return 13; });
+delete o.p;
+o.p = 0;
+assertEq(o.p, 13);
+
+reportCompare(0, 0, 'ok');