Bug 631723 - Make js_UpdateWatchpointsForShape return the new shape (and fix a few coding style nits). r=jimb.
authorJason Orendorff <jorendorff@mozilla.com>
Tue, 08 Feb 2011 15:45:12 -0600
changeset 62586 206a4c1c8ad8570502fff9ef6a7b464fb9b42f75
parent 62585 cec3c87c99b18c45081edfe5d64d1b92e7b72c2b
child 62587 80914cc8ceda18733dfed5ea79556abbf2c41867
push id18793
push usercleary@mozilla.com
push dateTue, 15 Feb 2011 20:07:31 +0000
treeherdermozilla-central@cdde780d7503 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjimb
bugs631723
milestone2.0b12pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 631723 - Make js_UpdateWatchpointsForShape return the new shape (and fix a few coding style nits). r=jimb.
js/src/jit-test/tests/jaeger/bug630366.js
js/src/jsdbgapi.cpp
js/src/jsdbgapiinlines.h
js/src/jsscope.cpp
js/src/tests/js1_8_5/extensions/jstests.list
js/src/tests/js1_8_5/extensions/regress-631723.js
new file mode 100644
--- /dev/null
+++ b/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;
+}
--- a/js/src/jsdbgapi.cpp
+++ b/js/src/jsdbgapi.cpp
@@ -820,69 +820,69 @@ WrapWatchedSetter(JSContext *cx, jsid id
 
     wrapper = js_NewFunction(cx, NULL, js_watch_set_wrapper, 1, 0,
                              setter ? CastAsObject(setter)->getParent() : NULL, atom);
     if (!wrapper)
         return NULL;
     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
      * watchpoint-wrapped shape may correspond to more than one non-watchpoint shape: we
      * 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
      * own watchpoint-aware shapes. Those functions report those changes back to the
      * watchpoint code, just as they do user-level changes. So if this change is
      * installing a watchpoint-aware shape, it's something we asked for ourselves, and can
      * 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);
 }
 
 /*
  * Return the underlying setter for |shape| on |obj|, seeing through any
  * watchpoint-wrapping. Note that we need |obj| to disambiguate, since a single
  * watchpoint-wrapped shape may correspond to more than one non-watchpoint shape; see the
--- a/js/src/jsdbgapiinlines.h
+++ b/js/src/jsdbgapiinlines.h
@@ -41,28 +41,28 @@
 #ifndef jsdbgapiinlines_h___
 #define jsdbgapiinlines_h___
 
 #include "jsdbgapi.h"
 #include "jscntxt.h"
 
 #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);
 }
 
 #endif /* JS_HAS_OBJ_WATCHPOINT && __cplusplus */
 
 #endif /* jsdbgapiinlines_h__ */
--- a/js/src/jsscope.cpp
+++ b/js/src/jsscope.cpp
@@ -763,20 +763,19 @@ JSObject::addProperty(JSContext *cx, jsi
     Shape **spp = nativeSearch(id, true);
     JS_ASSERT(!SHAPE_FETCH(spp));
     const Shape *shape = addPropertyInternal(cx, id, getter, setter, slot, attrs, 
                                              flags, shortid, spp);
     if (!shape)
         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;
 }
 
 const Shape *
 JSObject::addPropertyInternal(JSContext *cx, jsid id,
                               PropertyOp getter, StrictPropertyOp setter,
                               uint32 slot, uintN attrs,
@@ -891,23 +890,24 @@ JSObject::putProperty(JSContext *cx, jsi
          * You can't add properties to a non-extensible object, but you can change
          * attributes of properties in such objects.
          */
         if (!isExtensible()) {
             reportNotExtensible(cx);
             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)) {
+        if (!newShape)
+            return NULL;
+        newShape = js_UpdateWatchpointsForShape(cx, this, newShape);
+        if (!newShape)
             METER(wrapWatchFails);
-            return NULL;
-        }
-        return new_shape;
+        return newShape;
     }
 
     /* Property exists: search must have returned a valid *spp. */
     JS_ASSERT(!SHAPE_IS_REMOVED(*spp));
 
     if (!CheckCanChangeAttrs(cx, this, shape, &attrs))
         return NULL;
     
@@ -1033,22 +1033,20 @@ JSObject::putProperty(JSContext *cx, jsi
             getSlotRef(oldSlot).setUndefined();
 #endif
         JS_ATOMIC_INCREMENT(&cx->runtime->propertyRemovals);
     }
 
     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 *
 JSObject::changeProperty(JSContext *cx, const Shape *shape, uintN attrs, uintN mask,
                          PropertyOp getter, StrictPropertyOp setter)
 {
     JS_ASSERT_IF(inDictionaryMode(), !lastProp->frozen());
     JS_ASSERT(!JSID_IS_VOID(shape->id));
@@ -1104,21 +1102,22 @@ JSObject::changeProperty(JSContext *cx, 
         mutableShape->attrs = uint8(attrs);
 
         updateFlags(shape);
 
         /* See the corresponding code in putProperty. */
         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);
 
         newShape = getChildProperty(cx, shape->parent, child);
 #ifdef DEBUG
         if (newShape) {
             JS_ASSERT(newShape == lastProp);
--- a/js/src/tests/js1_8_5/extensions/jstests.list
+++ b/js/src/tests/js1_8_5/extensions/jstests.list
@@ -20,8 +20,9 @@ skip-if(!xulRuntime.shell) script clone-
 skip-if(!xulRuntime.shell) script clone-regexp.js
 skip-if(!xulRuntime.shell) script clone-leaf-object.js
 skip-if(!xulRuntime.shell) script clone-object.js
 skip-if(!xulRuntime.shell) script clone-typed-array.js
 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
new file mode 100644
--- /dev/null
+++ b/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');