Bug 1138499, part 1 - Assert some basic rules on property descriptors on entry to DefineProperty and exit from GetOwnPropertyDescriptor. r=Waldo.
authorJason Orendorff <jorendorff@mozilla.com>
Mon, 23 Mar 2015 14:32:27 -0500
changeset 237281 f9c99e8ce20747e7b233f475d2be424bcbad8399
parent 237280 725dbd169e90f0597e370217a1d45dd57f7e9d14
child 237282 034027f41aaf1c3a522e47dcdbafaf3525f898c7
push id57907
push userjorendorff@mozilla.com
push dateThu, 02 Apr 2015 15:02:59 +0000
treeherdermozilla-inbound@b3ef9fce0df5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersWaldo
bugs1138499
milestone40.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 1138499, part 1 - Assert some basic rules on property descriptors on entry to DefineProperty and exit from GetOwnPropertyDescriptor. r=Waldo.
js/src/jsapi-tests/testDefinePropertyIgnoredAttributes.cpp
js/src/jsapi.cpp
js/src/jsapi.h
js/src/jsobj.cpp
js/src/proxy/BaseProxyHandler.cpp
js/src/vm/NativeObject.cpp
--- a/js/src/jsapi-tests/testDefinePropertyIgnoredAttributes.cpp
+++ b/js/src/jsapi-tests/testDefinePropertyIgnoredAttributes.cpp
@@ -2,25 +2,16 @@
  * vim: set ts=8 sts=4 et sw=4 tw=99:
  */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "jsapi-tests/tests.h"
 
-static const unsigned IgnoreWithValue = JSPROP_IGNORE_ENUMERATE | JSPROP_IGNORE_READONLY |
-                               JSPROP_IGNORE_PERMANENT;
-static const unsigned IgnoreAll = IgnoreWithValue | JSPROP_IGNORE_VALUE;
-
-static const unsigned AllowConfigure = IgnoreAll & ~JSPROP_IGNORE_PERMANENT;
-static const unsigned AllowEnumerate = IgnoreAll & ~JSPROP_IGNORE_ENUMERATE;
-static const unsigned AllowWritable  = IgnoreAll & ~JSPROP_IGNORE_READONLY;
-static const unsigned ValueWithConfigurable = IgnoreWithValue & ~JSPROP_IGNORE_PERMANENT;
-
 static bool
 Getter(JSContext* cx, unsigned argc, JS::Value* vp)
 {
     JS::CallArgs args = CallArgsFromVp(argc, vp);
     args.rval().setBoolean(true);
     return true;
 }
 
@@ -46,56 +37,64 @@ CheckDescriptor(JS::Handle<JSPropertyDes
 }
 
 BEGIN_TEST(testDefinePropertyIgnoredAttributes)
 {
     JS::RootedObject obj(cx, JS_NewPlainObject(cx));
     JS::Rooted<JSPropertyDescriptor> desc(cx);
     JS::RootedValue defineValue(cx);
 
-    // Try a getter. Allow it to fill in the defaults.
+    // Try a getter. Allow it to fill in the defaults. Because we're passing a
+    // JSNative, JS_DefineProperty will infer JSPROP_GETTER even though we
+    // aren't passing it.
     CHECK(JS_DefineProperty(cx, obj, "foo", defineValue,
-                            IgnoreAll | JSPROP_SHARED,
+                            JSPROP_IGNORE_ENUMERATE | JSPROP_IGNORE_PERMANENT | JSPROP_SHARED,
                             Getter));
 
     CHECK(JS_GetPropertyDescriptor(cx, obj, "foo", &desc));
 
     // Note that JSPROP_READONLY is meaningless for accessor properties.
     CHECK(CheckDescriptor(desc, AccessorDescriptor, false, true, false));
 
     // Install another configurable property, so we can futz with it.
     CHECK(JS_DefineProperty(cx, obj, "bar", defineValue,
-                            AllowConfigure | JSPROP_SHARED,
+                            JSPROP_IGNORE_ENUMERATE | JSPROP_SHARED,
                             Getter));
     CHECK(JS_GetPropertyDescriptor(cx, obj, "bar", &desc));
     CHECK(CheckDescriptor(desc, AccessorDescriptor, false, true, true));
 
-    // Rewrite the descriptor to now be enumerable, ensuring that the lack of
-    // configurablity stayed.
+    // Rewrite the descriptor to now be enumerable, leaving the configurability
+    // unchanged.
     CHECK(JS_DefineProperty(cx, obj, "bar", defineValue,
-                            AllowEnumerate |
-                            JSPROP_ENUMERATE |
-                            JSPROP_SHARED,
+                            JSPROP_IGNORE_PERMANENT | JSPROP_ENUMERATE | JSPROP_SHARED,
                             Getter));
     CHECK(JS_GetPropertyDescriptor(cx, obj, "bar", &desc));
     CHECK(CheckDescriptor(desc, AccessorDescriptor, true, true, true));
 
     // Now try the same game with a value property
     defineValue.setObject(*obj);
-    CHECK(JS_DefineProperty(cx, obj, "baz", defineValue, IgnoreWithValue));
+    CHECK(JS_DefineProperty(cx, obj, "baz", defineValue,
+                            JSPROP_IGNORE_ENUMERATE |
+                            JSPROP_IGNORE_READONLY |
+                            JSPROP_IGNORE_PERMANENT));
     CHECK(JS_GetPropertyDescriptor(cx, obj, "baz", &desc));
     CHECK(CheckDescriptor(desc, DataDescriptor, false, false, false));
 
     // Now again with a configurable property
-    CHECK(JS_DefineProperty(cx, obj, "quox", defineValue, ValueWithConfigurable));
-    CHECK(JS_GetPropertyDescriptor(cx, obj, "quox", &desc));
+    CHECK(JS_DefineProperty(cx, obj, "quux", defineValue,
+                            JSPROP_IGNORE_ENUMERATE | JSPROP_IGNORE_READONLY));
+    CHECK(JS_GetPropertyDescriptor(cx, obj, "quux", &desc));
     CHECK(CheckDescriptor(desc, DataDescriptor, false, false, true));
 
-    // Just make it writable. Leave the old value and everythign else alone.
+    // Just make it writable. Leave the old value and everything else alone.
     defineValue.setUndefined();
-    CHECK(JS_DefineProperty(cx, obj, "quox", defineValue, AllowWritable));
-    CHECK(JS_GetPropertyDescriptor(cx, obj, "quox", &desc));
+    CHECK(JS_DefineProperty(cx, obj, "quux", defineValue,
+                            JSPROP_IGNORE_ENUMERATE |
+                            JSPROP_IGNORE_PERMANENT |
+                            JSPROP_IGNORE_VALUE));
+
+    CHECK(JS_GetPropertyDescriptor(cx, obj, "quux", &desc));
     CHECK(CheckDescriptor(desc, DataDescriptor, false, true, true));
     CHECK_SAME(JS::ObjectValue(*obj), desc.value());
 
     return true;
 }
 END_TEST(testDefinePropertyIgnoredAttributes)
--- a/js/src/jsapi.cpp
+++ b/js/src/jsapi.cpp
@@ -3433,16 +3433,17 @@ JS_DefineFunctions(JSContext* cx, Handle
             if (!(flags & JSPROP_DEFINE_LATE))
                 continue;
             break;
           default:
             MOZ_ASSERT(behavior == DontDefineLateProperties);
             if (flags & JSPROP_DEFINE_LATE)
                 continue;
         }
+        flags &= ~JSPROP_DEFINE_LATE;
 
         /*
          * Define a generic arity N+1 static method for the arity N prototype
          * method if flags contains JSFUN_GENERIC_NATIVE.
          */
         if (flags & JSFUN_GENERIC_NATIVE) {
             // We require that any consumers using JSFUN_GENERIC_NATIVE stash
             // the prototype and constructor in the global slots before invoking
--- a/js/src/jsapi.h
+++ b/js/src/jsapi.h
@@ -2516,16 +2516,23 @@ class PropertyDescriptorOperations
         MOZ_ASSERT((bit & (bit - 1)) == 0);  // only a single bit
         return (desc()->attrs & bit) != 0;
     }
 
     bool hasAny(unsigned bits) const {
         return (desc()->attrs & bits) != 0;
     }
 
+    bool hasAll(unsigned bits) const {
+        return (desc()->attrs & bits) == bits;
+    }
+
+    // Non-API attributes bit used internally for arguments objects.
+    enum { SHADOWABLE = JSPROP_INTERNAL_USE_BIT };
+
   public:
     // Descriptors with JSGetterOp/JSSetterOp are considered data
     // descriptors. It's complicated.
     bool isAccessorDescriptor() const { return hasAny(JSPROP_GETTER | JSPROP_SETTER); }
     bool isGenericDescriptor() const {
         return (desc()->attrs&
                 (JSPROP_GETTER | JSPROP_SETTER | JSPROP_IGNORE_READONLY | JSPROP_IGNORE_VALUE)) ==
                (JSPROP_IGNORE_READONLY | JSPROP_IGNORE_VALUE);
@@ -2563,16 +2570,68 @@ class PropertyDescriptorOperations
     bool isShared() const { return has(JSPROP_SHARED); }
 
     JS::HandleObject object() const {
         return JS::HandleObject::fromMarkedLocation(&desc()->obj);
     }
     unsigned attributes() const { return desc()->attrs; }
     JSGetterOp getter() const { return desc()->getter; }
     JSSetterOp setter() const { return desc()->setter; }
+
+    void assertValid() const {
+#ifdef DEBUG
+        MOZ_ASSERT((attributes() & ~(JSPROP_ENUMERATE | JSPROP_IGNORE_ENUMERATE |
+                                     JSPROP_PERMANENT | JSPROP_IGNORE_PERMANENT |
+                                     JSPROP_READONLY | JSPROP_IGNORE_READONLY |
+                                     JSPROP_IGNORE_VALUE |
+                                     JSPROP_GETTER |
+                                     JSPROP_SETTER |
+                                     JSPROP_SHARED |
+                                     JSPROP_REDEFINE_NONCONFIGURABLE |
+                                     SHADOWABLE)) == 0);
+        MOZ_ASSERT(!hasAll(JSPROP_IGNORE_ENUMERATE | JSPROP_ENUMERATE));
+        MOZ_ASSERT(!hasAll(JSPROP_IGNORE_PERMANENT | JSPROP_PERMANENT));
+        if (isAccessorDescriptor()) {
+            MOZ_ASSERT(has(JSPROP_SHARED));
+            MOZ_ASSERT(!has(JSPROP_READONLY));
+            MOZ_ASSERT(!has(JSPROP_IGNORE_READONLY));
+            MOZ_ASSERT(!has(JSPROP_IGNORE_VALUE));
+            MOZ_ASSERT(!has(SHADOWABLE));
+            MOZ_ASSERT(desc()->value.isUndefined());
+            MOZ_ASSERT_IF(!has(JSPROP_GETTER), !getter());
+            MOZ_ASSERT_IF(!has(JSPROP_SETTER), !setter());
+        } else {
+            MOZ_ASSERT(!hasAll(JSPROP_IGNORE_READONLY | JSPROP_READONLY));
+            MOZ_ASSERT_IF(has(JSPROP_IGNORE_VALUE), value().isUndefined());
+        }
+        MOZ_ASSERT(getter() != JS_PropertyStub);
+        MOZ_ASSERT(setter() != JS_StrictPropertyStub);
+#endif
+    }
+
+    void assertComplete() const {
+#ifdef DEBUG
+        assertValid();
+        MOZ_ASSERT((attributes() & ~(JSPROP_ENUMERATE |
+                                     JSPROP_PERMANENT |
+                                     JSPROP_READONLY |
+                                     JSPROP_GETTER |
+                                     JSPROP_SETTER |
+                                     JSPROP_SHARED |
+                                     JSPROP_REDEFINE_NONCONFIGURABLE |
+                                     SHADOWABLE)) == 0);
+#endif
+    }
+
+    void assertCompleteIfFound() const {
+#ifdef DEBUG
+        if (object())
+            assertComplete();
+#endif
+    }
 };
 
 template <typename Outer>
 class MutablePropertyDescriptorOperations : public PropertyDescriptorOperations<Outer>
 {
     JSPropertyDescriptor * desc() { return static_cast<Outer*>(this)->extractMutable(); }
 
   public:
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -3118,18 +3118,22 @@ js::PreventExtensions(JSContext* cx, Han
     ObjectOpResult result;
     return PreventExtensions(cx, obj, result) && result.checkStrict(cx, obj);
 }
 
 bool
 js::GetOwnPropertyDescriptor(JSContext* cx, HandleObject obj, HandleId id,
                              MutableHandle<PropertyDescriptor> desc)
 {
-    if (GetOwnPropertyOp op = obj->getOps()->getOwnPropertyDescriptor)
-        return op(cx, obj, id, desc);
+    if (GetOwnPropertyOp op = obj->getOps()->getOwnPropertyDescriptor) {
+        bool ok = op(cx, obj, id, desc);
+        if (ok)
+            desc.assertCompleteIfFound();
+        return ok;
+    }
 
     RootedShape shape(cx);
     if (!NativeLookupOwnProperty<CanGC>(cx, obj.as<NativeObject>(), id, &shape))
         return false;
     if (!shape) {
         desc.object().set(nullptr);
         return true;
     }
@@ -3169,23 +3173,25 @@ js::GetOwnPropertyDescriptor(JSContext* 
     }
 
     RootedValue value(cx);
     if (doGet && !GetProperty(cx, obj, obj, id, &value))
         return false;
 
     desc.value().set(value);
     desc.object().set(obj);
+    desc.assertComplete();
     return true;
 }
 
 bool
 js::DefineProperty(JSContext* cx, HandleObject obj, HandleId id, Handle<PropertyDescriptor> desc,
                    ObjectOpResult& result)
 {
+    desc.assertValid();
     if (DefinePropertyOp op = obj->getOps()->defineProperty)
         return op(cx, obj, id, desc, result);
     return NativeDefineProperty(cx, obj.as<NativeObject>(), id, desc, result);
 }
 
 bool
 js::DefineProperty(ExclusiveContext* cx, HandleObject obj, HandleId id, HandleValue value,
                    JSGetterOp getter, JSSetterOp setter, unsigned attrs,
@@ -3283,18 +3289,22 @@ js::SetImmutablePrototype(ExclusiveConte
 
 bool
 js::GetPropertyDescriptor(JSContext* cx, HandleObject obj, HandleId id,
                           MutableHandle<PropertyDescriptor> desc)
 {
     RootedObject pobj(cx);
 
     for (pobj = obj; pobj;) {
-        if (pobj->is<ProxyObject>())
-            return Proxy::getPropertyDescriptor(cx, pobj, id, desc);
+        if (pobj->is<ProxyObject>()) {
+            bool ok = Proxy::getPropertyDescriptor(cx, pobj, id, desc);
+            if (ok)
+                desc.assertCompleteIfFound();
+            return ok;
+        }
 
         if (!GetOwnPropertyDescriptor(cx, pobj, id, desc))
             return false;
 
         if (desc.object())
             return true;
 
         if (!GetPrototype(cx, pobj, &pobj))
--- a/js/src/proxy/BaseProxyHandler.cpp
+++ b/js/src/proxy/BaseProxyHandler.cpp
@@ -50,16 +50,17 @@ BaseProxyHandler::get(JSContext* cx, Han
 
     Rooted<PropertyDescriptor> desc(cx);
     if (!getPropertyDescriptor(cx, proxy, id, &desc))
         return false;
     if (!desc.object()) {
         vp.setUndefined();
         return true;
     }
+    desc.assertComplete();
     MOZ_ASSERT(desc.getter() != JS_PropertyStub);
     if (!desc.getter()) {
         vp.set(desc.value());
         return true;
     }
     if (desc.hasGetterObject())
         return InvokeGetter(cx, receiver, ObjectValue(*desc.getterObject()), vp);
     if (!desc.isShared())
@@ -79,16 +80,17 @@ BaseProxyHandler::set(JSContext* cx, Han
     // This method is not covered by any spec, but we follow ES6 draft rev 28
     // (2014 Oct 14) 9.1.9 fairly closely, adapting it slightly for
     // SpiderMonkey's particular foibles.
 
     // Steps 2-3.  (Step 1 is a superfluous assertion.)
     Rooted<PropertyDescriptor> ownDesc(cx);
     if (!getOwnPropertyDescriptor(cx, proxy, id, &ownDesc))
         return false;
+    ownDesc.assertCompleteIfFound();
 
     // The rest is factored out into a separate function with a weird name.
     // This algorithm continues just below.
     return SetPropertyIgnoringNamedGetter(cx, proxy, id, v, receiver, ownDesc, result);
 }
 
 bool
 js::SetPropertyIgnoringNamedGetter(JSContext* cx, HandleObject obj, HandleId id, HandleValue v,
@@ -180,16 +182,18 @@ BaseProxyHandler::getOwnEnumerableProper
         id = props[j];
         if (JSID_IS_SYMBOL(id))
             continue;
 
         AutoWaivePolicy policy(cx, proxy, id, BaseProxyHandler::GET);
         Rooted<PropertyDescriptor> desc(cx);
         if (!getOwnPropertyDescriptor(cx, proxy, id, &desc))
             return false;
+        desc.assertCompleteIfFound();
+
         if (desc.object() && desc.enumerable())
             props[i++].set(id);
     }
 
     MOZ_ASSERT(i <= props.length());
     props.resize(i);
 
     return true;
--- a/js/src/vm/NativeObject.cpp
+++ b/js/src/vm/NativeObject.cpp
@@ -1319,24 +1319,25 @@ CheckAccessorRedefinition(ExclusiveConte
     if (!cx->isJSContext())
         return false;
 
     return Throw(cx->asJSContext(), id, JSMSG_CANT_REDEFINE_PROP);
 }
 
 bool
 js::NativeDefineProperty(ExclusiveContext* cx, HandleNativeObject obj, HandleId id,
-                         Handle<JSPropertyDescriptor> desc,
+                         Handle<PropertyDescriptor> desc,
                          ObjectOpResult& result)
 {
+    desc.assertValid();
+
     GetterOp getter = desc.getter();
     SetterOp setter = desc.setter();
     unsigned attrs = desc.attributes();
-    MOZ_ASSERT(getter != JS_PropertyStub);
-    MOZ_ASSERT(setter != JS_StrictPropertyStub);
+
     MOZ_ASSERT(!(attrs & JSPROP_PROPOP_ACCESSORS));
 
     AutoRooterGetterSetter gsRoot(cx, attrs, &getter, &setter);
 
     RootedShape shape(cx);
     RootedValue updateValue(cx, desc.value());
     bool shouldDefine = true;