Bug 997894 - Part 2: Replace existing externally rooted PropDesc sites with Rooted<PropDesc>. (r=terrence)
authorEric Faust <efaustbmo@gmail.com>
Tue, 03 Jun 2014 12:05:48 -0700
changeset 207296 b9d6fb7448b3d8944b0e02cc9c74e7b388885049
parent 207295 d17bb70b871ad4d3ba9e9c20b6e5f178a5c69a90
child 207297 4058f94dc21adeb2590a113d31f367762d1df2b3
push id494
push userraliiev@mozilla.com
push dateMon, 25 Aug 2014 18:42:16 +0000
treeherdermozilla-release@a3cc3e46b571 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersterrence
bugs997894
milestone32.0a1
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 997894 - Part 2: Replace existing externally rooted PropDesc sites with Rooted<PropDesc>. (r=terrence)
js/src/jsobj.cpp
js/src/jsproxy.cpp
js/src/vm/Debugger.cpp
js/src/vm/ObjectImpl.cpp
js/src/vm/PropDesc.h
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -258,18 +258,17 @@ bool
 js::NewPropertyDescriptorObject(JSContext *cx, Handle<PropertyDescriptor> desc,
                                 MutableHandleValue vp)
 {
     if (!desc.object()) {
         vp.setUndefined();
         return true;
     }
 
-    /* We have our own property, so start creating the descriptor. */
-    AutoPropDescRooter d(cx);
+    Rooted<PropDesc> d(cx);
 
     d.initFromPropertyDescriptor(desc);
     if (!d.makeObject(cx))
         return false;
     vp.set(d.pd());
     return true;
 }
 
@@ -1057,41 +1056,37 @@ js::DefineProperty(JSContext *cx, Handle
 
     return DefinePropertyOnObject(cx, obj, id, desc, throwError, rval);
 }
 
 bool
 js::DefineOwnProperty(JSContext *cx, HandleObject obj, HandleId id, HandleValue descriptor,
                       bool *bp)
 {
-    AutoPropDescArrayRooter descs(cx);
-    PropDesc *desc = descs.append();
-    if (!desc || !desc->initialize(cx, descriptor))
+    Rooted<PropDesc> desc(cx);
+    if (!desc.initialize(cx, descriptor))
         return false;
 
     bool rval;
-    if (!DefineProperty(cx, obj, id, *desc, true, &rval))
+    if (!DefineProperty(cx, obj, id, desc, true, &rval))
         return false;
     *bp = !!rval;
     return true;
 }
 
 bool
 js::DefineOwnProperty(JSContext *cx, HandleObject obj, HandleId id,
                       Handle<PropertyDescriptor> descriptor, bool *bp)
 {
-    AutoPropDescArrayRooter descs(cx);
-    PropDesc *desc = descs.append();
-    if (!desc)
-        return false;
-
-    desc->initFromPropertyDescriptor(descriptor);
+    Rooted<PropDesc> desc(cx);
+
+    desc.initFromPropertyDescriptor(descriptor);
 
     bool rval;
-    if (!DefineProperty(cx, obj, id, *desc, true, &rval))
+    if (!DefineProperty(cx, obj, id, desc, true, &rval))
         return false;
     *bp = !!rval;
     return true;
 }
 
 
 bool
 js::ReadPropertyDescriptors(JSContext *cx, HandleObject props, bool checkAccessors,
--- a/js/src/jsproxy.cpp
+++ b/js/src/jsproxy.cpp
@@ -673,27 +673,26 @@ Trap2(JSContext *cx, HandleObject handle
     argv[1].set(v);
     return Trap(cx, handler, fval, 2, argv.begin(), rval);
 }
 
 static bool
 ParsePropertyDescriptorObject(JSContext *cx, HandleObject obj, const Value &v,
                               MutableHandle<PropertyDescriptor> desc, bool complete = false)
 {
-    AutoPropDescArrayRooter descs(cx);
-    PropDesc *d = descs.append();
-    if (!d || !d->initialize(cx, v))
+    Rooted<PropDesc> d(cx);
+    if (!d.initialize(cx, v))
         return false;
     if (complete)
-        d->complete();
+        d.complete();
     desc.object().set(obj);
-    desc.value().set(d->hasValue() ? d->value() : UndefinedValue());
-    desc.setAttributes(d->attributes());
-    desc.setGetter(d->getter());
-    desc.setSetter(d->setter());
+    desc.value().set(d.hasValue() ? d.value() : UndefinedValue());
+    desc.setAttributes(d.attributes());
+    desc.setGetter(d.getter());
+    desc.setSetter(d.setter());
     return true;
 }
 
 static bool
 IndicatePropertyNotFound(MutableHandle<PropertyDescriptor> desc)
 {
     desc.object().set(nullptr);
     return true;
@@ -1096,28 +1095,28 @@ class ScriptedDirectProxyHandler : publi
     static ScriptedDirectProxyHandler singleton;
 };
 
 // This variable exists solely to provide a unique address for use as an identifier.
 static const char sScriptedDirectProxyHandlerFamily = 0;
 
 // Aux.2 FromGenericPropertyDescriptor(Desc)
 static bool
-FromGenericPropertyDescriptor(JSContext *cx, PropDesc *desc, MutableHandleValue rval)
+FromGenericPropertyDescriptor(JSContext *cx, MutableHandle<PropDesc> desc, MutableHandleValue rval)
 {
     // Aux.2 step 1
-    if (desc->isUndefined()) {
+    if (desc.isUndefined()) {
         rval.setUndefined();
         return true;
     }
 
     // steps 3-9
-    if (!desc->makeObject(cx))
+    if (!desc.makeObject(cx))
         return false;
-    rval.set(desc->pd());
+    rval.set(desc.pd());
     return true;
 }
 
 /*
  * Aux.3 NormalizePropertyDescriptor(Attributes)
  *
  * NOTE: to minimize code duplication, the code for this function is shared with
  * that for Aux.4 NormalizeAndCompletePropertyDescriptor (see below). The
@@ -1126,35 +1125,34 @@ FromGenericPropertyDescriptor(JSContext 
 static bool
 NormalizePropertyDescriptor(JSContext *cx, MutableHandleValue vp, bool complete = false)
 {
     // Aux.4 step 1
     if (complete && vp.isUndefined())
         return true;
 
     // Aux.3 steps 1-2 / Aux.4 steps 2-3
-    AutoPropDescArrayRooter descs(cx);
-    PropDesc *desc = descs.append();
-    if (!desc || !desc->initialize(cx, vp.get()))
+    Rooted<PropDesc> desc(cx);
+    if (!desc.initialize(cx, vp.get()))
         return false;
     if (complete)
-        desc->complete();
+        desc.complete();
     JS_ASSERT(vp.isObject()); // due to desc->initialize
     RootedObject attributes(cx, &vp.toObject());
 
     /*
      * Aux.3 step 3 / Aux.4 step 4
      *
      * NOTE: Aux.4 step 4 actually specifies FromPropertyDescriptor here.
      * However, the way FromPropertyDescriptor is implemented (PropDesc::
      * makeObject) is actually closer to FromGenericPropertyDescriptor,
      * and is in fact used to implement the latter, so we might as well call it
      * directly.
      */
-    if (!FromGenericPropertyDescriptor(cx, desc, vp))
+    if (!FromGenericPropertyDescriptor(cx, &desc, vp))
         return false;
     if (vp.isUndefined())
         return true;
     RootedObject descObj(cx, &vp.toObject());
 
     // Aux.3 steps 4-5 / Aux.4 steps 5-6
     AutoIdVector props(cx);
     if (!GetPropertyNames(cx, attributes, 0, &props))
@@ -1198,114 +1196,114 @@ IsDataDescriptor(const PropertyDescripto
 static inline bool
 IsAccessorDescriptor(const PropertyDescriptor &desc)
 {
     return desc.obj && desc.attrs & (JSPROP_GETTER | JSPROP_SETTER);
 }
 
 // Aux.5 ValidateProperty(O, P, Desc)
 static bool
-ValidateProperty(JSContext *cx, HandleObject obj, HandleId id, PropDesc *desc, bool *bp)
+ValidateProperty(JSContext *cx, HandleObject obj, HandleId id, Handle<PropDesc> desc, bool *bp)
 {
     // step 1
     Rooted<PropertyDescriptor> current(cx);
     if (!GetOwnPropertyDescriptor(cx, obj, id, &current))
         return false;
 
     /*
      * steps 2-4 are redundant since ValidateProperty is never called unless
      * target.[[HasOwn]](P) is true
      */
     JS_ASSERT(current.object());
 
     // step 5
-    if (!desc->hasValue() && !desc->hasWritable() && !desc->hasGet() && !desc->hasSet() &&
-        !desc->hasEnumerable() && !desc->hasConfigurable())
+    if (!desc.hasValue() && !desc.hasWritable() && !desc.hasGet() && !desc.hasSet() &&
+        !desc.hasEnumerable() && !desc.hasConfigurable())
     {
         *bp = true;
         return true;
     }
 
     // step 6
-    if ((!desc->hasWritable() || desc->writable() == !current.isReadonly()) &&
-        (!desc->hasGet() || desc->getter() == current.getter()) &&
-        (!desc->hasSet() || desc->setter() == current.setter()) &&
-        (!desc->hasEnumerable() || desc->enumerable() == current.isEnumerable()) &&
-        (!desc->hasConfigurable() || desc->configurable() == !current.isPermanent()))
+    if ((!desc.hasWritable() || desc.writable() == !current.isReadonly()) &&
+        (!desc.hasGet() || desc.getter() == current.getter()) &&
+        (!desc.hasSet() || desc.setter() == current.setter()) &&
+        (!desc.hasEnumerable() || desc.enumerable() == current.isEnumerable()) &&
+        (!desc.hasConfigurable() || desc.configurable() == !current.isPermanent()))
     {
-        if (!desc->hasValue()) {
+        if (!desc.hasValue()) {
             *bp = true;
             return true;
         }
         bool same = false;
-        if (!SameValue(cx, desc->value(), current.value(), &same))
+        if (!SameValue(cx, desc.value(), current.value(), &same))
             return false;
         if (same) {
             *bp = true;
             return true;
         }
     }
 
     // step 7
     if (current.isPermanent()) {
-        if (desc->hasConfigurable() && desc->configurable()) {
+        if (desc.hasConfigurable() && desc.configurable()) {
             *bp = false;
             return true;
         }
 
-        if (desc->hasEnumerable() &&
-            desc->enumerable() != current.isEnumerable())
+        if (desc.hasEnumerable() &&
+            desc.enumerable() != current.isEnumerable())
         {
             *bp = false;
             return true;
         }
     }
 
     // step 8
-    if (desc->isGenericDescriptor()) {
+    if (desc.isGenericDescriptor()) {
         *bp = true;
         return true;
     }
 
     // step 9
-    if (IsDataDescriptor(current) != desc->isDataDescriptor()) {
+    if (IsDataDescriptor(current) != desc.isDataDescriptor()) {
         *bp = !current.isPermanent();
         return true;
     }
 
     // step 10
     if (IsDataDescriptor(current)) {
-        JS_ASSERT(desc->isDataDescriptor()); // by step 9
+        JS_ASSERT(desc.isDataDescriptor()); // by step 9
         if (current.isPermanent() && current.isReadonly()) {
-            if (desc->hasWritable() && desc->writable()) {
+            if (desc.hasWritable() && desc.writable()) {
                 *bp = false;
                 return true;
             }
 
-            if (desc->hasValue()) {
+            if (desc.hasValue()) {
                 bool same;
-                if (!SameValue(cx, desc->value(), current.value(), &same))
+                if (!SameValue(cx, desc.value(), current.value(), &same))
                     return false;
                 if (!same) {
                     *bp = false;
                     return true;
                 }
             }
         }
 
         *bp = true;
         return true;
     }
 
     // steps 11-12
     JS_ASSERT(IsAccessorDescriptor(current)); // by step 10
-    JS_ASSERT(desc->isAccessorDescriptor()); // by step 9
+    JS_ASSERT(desc.isAccessorDescriptor()); // by step 9
     *bp = (!current.isPermanent() ||
-           ((!desc->hasSet() || desc->setter() == current.setter()) &&
-            (!desc->hasGet() || desc->getter() == current.getter())));
+           ((!desc.hasSet() || desc.setter() == current.setter()) &&
+            (!desc.hasGet() || desc.getter() == current.getter())));
     return true;
 }
 
 // Aux.6 IsSealed(O, P)
 static bool
 IsSealed(JSContext* cx, HandleObject obj, HandleId id, bool *bp)
 {
     // step 1
@@ -1424,35 +1422,34 @@ TrapGetOwnProperty(JSContext *cx, Handle
     bool extensible;
     if (!JSObject::isExtensible(cx, target, &extensible))
         return false;
     if (!extensible && !isFixed) {
         JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_CANT_REPORT_NEW);
         return false;
     }
 
-    AutoPropDescArrayRooter descs(cx);
-    PropDesc *desc = descs.append();
-    if (!desc || !desc->initialize(cx, trapResult))
+    Rooted<PropDesc> desc(cx);
+    if (!desc.initialize(cx, trapResult))
         return false;
 
     /* step 10 */
     if (isFixed) {
         bool valid;
         if (!ValidateProperty(cx, target, id, desc, &valid))
             return false;
 
         if (!valid) {
             JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_CANT_REPORT_INVALID);
             return false;
         }
     }
 
     // step 11
-    if (!desc->configurable() && !isFixed) {
+    if (!desc.configurable() && !isFixed) {
         JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_CANT_REPORT_NE_AS_NC);
         return false;
     }
 
     // step 12
     rval.set(trapResult);
     return true;
 }
@@ -1508,32 +1505,31 @@ TrapDefineOwnProperty(JSContext *cx, Han
         bool extensible;
         if (!JSObject::isExtensible(cx, target, &extensible))
             return false;
         if (!extensible && !isFixed) {
             JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_CANT_DEFINE_NEW);
             return false;
         }
 
-        AutoPropDescArrayRooter descs(cx);
-        PropDesc *desc = descs.append();
-        if (!desc || !desc->initialize(cx, normalizedDesc))
+        Rooted<PropDesc> desc(cx);
+        if (!desc.initialize(cx, normalizedDesc))
             return false;
 
         if (isFixed) {
             bool valid;
             if (!ValidateProperty(cx, target, id, desc, &valid))
                 return false;
             if (!valid) {
                 JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_CANT_DEFINE_INVALID);
                 return false;
             }
         }
 
-        if (!desc->configurable() && !isFixed) {
+        if (!desc.configurable() && !isFixed) {
             JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_CANT_DEFINE_NE_AS_NC);
             return false;
         }
 
         vp.set(BooleanValue(true));
         return true;
     }
 
@@ -1748,21 +1744,20 @@ ScriptedDirectProxyHandler::getOwnProper
     return ParsePropertyDescriptorObject(cx, proxy, v, desc, true);
 }
 
 bool
 ScriptedDirectProxyHandler::defineProperty(JSContext *cx, HandleObject proxy, HandleId id,
                                            MutableHandle<PropertyDescriptor> desc)
 {
     // step 1
-    AutoPropDescArrayRooter descs(cx);
-    PropDesc *d = descs.append();
-    d->initFromPropertyDescriptor(desc);
+    Rooted<PropDesc> d(cx);
+    d.initFromPropertyDescriptor(desc);
     RootedValue v(cx);
-    if (!FromGenericPropertyDescriptor(cx, d, &v))
+    if (!FromGenericPropertyDescriptor(cx, &d, &v))
         return false;
 
     // step 2
     return TrapDefineOwnProperty(cx, proxy, id, &v);
 }
 
 bool
 ScriptedDirectProxyHandler::getOwnPropertyNames(JSContext *cx, HandleObject proxy,
--- a/js/src/vm/Debugger.cpp
+++ b/js/src/vm/Debugger.cpp
@@ -5269,44 +5269,37 @@ DebuggerObject_defineProperty(JSContext 
 {
     THIS_DEBUGOBJECT_OWNER_REFERENT(cx, argc, vp, "defineProperty", args, dbg, obj);
     REQUIRE_ARGC("Debugger.Object.defineProperty", 2);
 
     RootedId id(cx);
     if (!ValueToId<CanGC>(cx, args[0], &id))
         return false;
 
-    AutoPropDescArrayRooter descs(cx);
-    if (!descs.reserve(3)) // desc, unwrappedDesc, rewrappedDesc
-        return false;
-    PropDesc *desc = descs.append();
-    if (!desc || !desc->initialize(cx, args[1], false))
+    Rooted<PropDesc> desc(cx);
+    if (!desc.initialize(cx, args[1], false))
         return false;
-    desc->clearPd();
-
-    PropDesc *unwrappedDesc = descs.append();
-    if (!unwrappedDesc || !desc->unwrapDebuggerObjectsInto(cx, dbg, obj, unwrappedDesc))
+    desc.clearPd();
+
+    if (!desc.get().unwrapDebuggerObjectsInto(cx, dbg, obj, desc.address()))
         return false;
-    if (!unwrappedDesc->checkGetter(cx) || !unwrappedDesc->checkSetter(cx))
+    if (!desc.checkGetter(cx) || !desc.checkSetter(cx))
         return false;
 
     {
-        PropDesc *rewrappedDesc = descs.append();
-        if (!rewrappedDesc)
-            return false;
         RootedId wrappedId(cx);
 
         Maybe<AutoCompartment> ac;
         ac.construct(cx, obj);
-        if (!unwrappedDesc->wrapInto(cx, obj, id, wrappedId.address(), rewrappedDesc))
+        if (!desc.get().wrapInto(cx, obj, id, wrappedId.address(), desc.address()))
             return false;
 
         ErrorCopier ec(ac, dbg->toJSObject());
         bool dummy;
-        if (!DefineProperty(cx, obj, wrappedId, *rewrappedDesc, true, &dummy))
+        if (!DefineProperty(cx, obj, wrappedId, desc, true, &dummy))
             return false;
     }
 
     args.rval().setUndefined();
     return true;
 }
 
 static bool
--- a/js/src/vm/ObjectImpl.cpp
+++ b/js/src/vm/ObjectImpl.cpp
@@ -375,24 +375,15 @@ js::ObjectImpl::markChildren(JSTracer *t
 
     if (shape_->isNative()) {
         MarkObjectSlots(trc, obj, 0, obj->slotSpan());
         gc::MarkArraySlots(trc, obj->getDenseInitializedLength(), obj->getDenseElements(), "objectElements");
     }
 }
 
 void
-AutoPropDescRooter::trace(JSTracer *trc)
-{
-    gc::MarkValueRoot(trc, &propDesc.pd_, "AutoPropDescRooter pd");
-    gc::MarkValueRoot(trc, &propDesc.value_, "AutoPropDescRooter value");
-    gc::MarkValueRoot(trc, &propDesc.get_, "AutoPropDescRooter get");
-    gc::MarkValueRoot(trc, &propDesc.set_, "AutoPropDescRooter set");
-}
-
-void
 PropDesc::trace(JSTracer *trc)
 {
     gc::MarkValueRoot(trc, &pd_, "PropDesc pd");
     gc::MarkValueRoot(trc, &value_, "PropDesc value");
     gc::MarkValueRoot(trc, &get_, "PropDesc get");
     gc::MarkValueRoot(trc, &set_, "PropDesc set");
 }
--- a/js/src/vm/PropDesc.h
+++ b/js/src/vm/PropDesc.h
@@ -61,17 +61,16 @@ struct PropDesc {
         attrs(0),
         hasGet_(false), hasSet_(false),
         hasValue_(true), hasWritable_(false), hasEnumerable_(false), hasConfigurable_(false),
         isUndefined_(false)
     {
     }
 
   public:
-    friend class AutoPropDescRooter;
     friend void JS::AutoGCRooter::trace(JSTracer *trc);
     friend struct GCMethods<PropDesc>;
 
     void trace(JSTracer *trc);
     static ThingRootKind rootKind() { return THING_ROOT_PROP_DESC; }
 
     enum Enumerability { Enumerable = true, NonEnumerable = false };
     enum Configurability { Configurable = true, NonConfigurable = false };
@@ -228,74 +227,16 @@ struct PropDesc {
 
     bool unwrapDebuggerObjectsInto(JSContext *cx, Debugger *dbg, HandleObject obj,
                                    PropDesc *unwrapped) const;
 
     bool wrapInto(JSContext *cx, HandleObject obj, const jsid &id, jsid *wrappedId,
                   PropDesc *wrappedDesc) const;
 };
 
-class AutoPropDescRooter : private JS::CustomAutoRooter
-{
-  public:
-    explicit AutoPropDescRooter(JSContext *cx
-                                MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
-      : CustomAutoRooter(cx)
-    {
-        MOZ_GUARD_OBJECT_NOTIFIER_INIT;
-    }
-
-    PropDesc& getPropDesc() { return propDesc; }
-
-    void initFromPropertyDescriptor(Handle<JSPropertyDescriptor> desc) {
-        propDesc.initFromPropertyDescriptor(desc);
-    }
-
-    bool makeObject(JSContext *cx) {
-        return propDesc.makeObject(cx);
-    }
-
-    void setUndefined() { propDesc.setUndefined(); }
-    bool isUndefined() const { return propDesc.isUndefined(); }
-
-    bool hasGet() const { return propDesc.hasGet(); }
-    bool hasSet() const { return propDesc.hasSet(); }
-    bool hasValue() const { return propDesc.hasValue(); }
-    bool hasWritable() const { return propDesc.hasWritable(); }
-    bool hasEnumerable() const { return propDesc.hasEnumerable(); }
-    bool hasConfigurable() const { return propDesc.hasConfigurable(); }
-
-    Value pd() const { return propDesc.pd(); }
-    void clearPd() { propDesc.clearPd(); }
-
-    uint8_t attributes() const { return propDesc.attributes(); }
-
-    bool isAccessorDescriptor() const { return propDesc.isAccessorDescriptor(); }
-    bool isDataDescriptor() const { return propDesc.isDataDescriptor(); }
-    bool isGenericDescriptor() const { return propDesc.isGenericDescriptor(); }
-    bool configurable() const { return propDesc.configurable(); }
-    bool enumerable() const { return propDesc.enumerable(); }
-    bool writable() const { return propDesc.writable(); }
-
-    HandleValue value() const { return propDesc.value(); }
-    JSObject *getterObject() const { return propDesc.getterObject(); }
-    JSObject *setterObject() const { return propDesc.setterObject(); }
-    HandleValue getterValue() const { return propDesc.getterValue(); }
-    HandleValue setterValue() const { return propDesc.setterValue(); }
-
-    JSPropertyOp getter() const { return propDesc.getter(); }
-    JSStrictPropertyOp setter() const { return propDesc.setter(); }
-
-  private:
-    virtual void trace(JSTracer *trc);
-
-    PropDesc propDesc;
-    MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
-};
-
 } /* namespace js */
 
 namespace JS {
 
 template <typename Outer>
 class PropDescOperations
 {
     const js::PropDesc * desc() const { return static_cast<const Outer*>(this)->extract(); }