Bug 1037770 - Stop erroneously defaulting property values for Object.defineProperty on a proxy. (r=jorendorff)
authorEric Faust <efaustbmo@gmail.com>
Tue, 19 Aug 2014 16:14:08 -0700
changeset 214798 28ccb18dc241c697fe7d995fff6174081b93e2b4
parent 214797 c181933a82736758370afcea2a5a733b223688ab
child 214799 94d8ee70f149522edb0de6327d61164b1dff80a4
push idunknown
push userunknown
push dateunknown
reviewersjorendorff
bugs1037770
milestone34.0a1
Bug 1037770 - Stop erroneously defaulting property values for Object.defineProperty on a proxy. (r=jorendorff)
js/src/jsapi-tests/moz.build
js/src/jsapi-tests/testDefinePropertyIgnoredAttributes.cpp
js/src/jsapi.h
js/src/jsobj.cpp
js/src/tests/ecma_6/Proxy/regress-bug1037770.js
--- a/js/src/jsapi-tests/moz.build
+++ b/js/src/jsapi-tests/moz.build
@@ -19,16 +19,17 @@ UNIFIED_SOURCES += [
     'testCloneScript.cpp',
     'testConservativeGC.cpp',
     'testContexts.cpp',
     'testCustomIterator.cpp',
     'testDebugger.cpp',
     'testDeepFreeze.cpp',
     'testDefineGetterSetterNonEnumerable.cpp',
     'testDefineProperty.cpp',
+    'testDefinePropertyIgnoredAttributes.cpp',
     'testEnclosingFunction.cpp',
     'testErrorCopying.cpp',
     'testException.cpp',
     'testExternalStrings.cpp',
     'testFindSCCs.cpp',
     'testFreshGlobalEvalRedefinition.cpp',
     'testFuncCallback.cpp',
     'testFunctionProperties.cpp',
new file mode 100644
--- /dev/null
+++ b/js/src/jsapi-tests/testDefinePropertyIgnoredAttributes.cpp
@@ -0,0 +1,94 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
+ * 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;
+}
+
+static bool
+CheckDescriptor(JS::Handle<JSPropertyDescriptor> desc, bool enumerable,
+                bool writable, bool configurable)
+{
+    if (!desc.object())
+        return false;
+    if (desc.isEnumerable() != enumerable)
+        return false;
+    if (desc.isReadonly() == writable)
+        return false;
+    if (desc.isPermanent() == configurable)
+        return false;
+    return true;
+}
+
+BEGIN_TEST(testDefinePropertyIgnoredAttributes)
+{
+    JS::RootedObject obj(cx, JS_NewObject(cx, nullptr, JS::NullPtr(), JS::NullPtr()));
+    JS::Rooted<JSPropertyDescriptor> desc(cx);
+    JS::RootedValue defineValue(cx);
+
+    // Try a getter. Allow it to fill in the defaults.
+    CHECK(JS_DefineProperty(cx, obj, "foo", defineValue,
+                            IgnoreAll | JSPROP_NATIVE_ACCESSORS, (JSPropertyOp)Getter));
+
+    CHECK(JS_GetPropertyDescriptor(cx, obj, "foo", &desc));
+
+    // Note that since JSPROP_READONLY means nothing for accessor properties, we will actually
+    // claim to be writable, since the flag is not included in the mask.
+    CHECK(CheckDescriptor(desc, false, true, false));
+
+    // Install another configurable property, so we can futz with it.
+    CHECK(JS_DefineProperty(cx, obj, "bar", defineValue,
+                            AllowConfigure | JSPROP_NATIVE_ACCESSORS, (JSPropertyOp)Getter));
+    CHECK(JS_GetPropertyDescriptor(cx, obj, "bar", &desc));
+    CHECK(CheckDescriptor(desc, false, true, true));
+
+    // Rewrite the descriptor to now be enumerable, ensuring that the lack of
+    // configurablity stayed.
+    CHECK(JS_DefineProperty(cx, obj, "bar", defineValue,
+                            AllowEnumerate |
+                            JSPROP_ENUMERATE |
+                            JSPROP_NATIVE_ACCESSORS,
+                            (JSPropertyOp)Getter));
+    CHECK(JS_GetPropertyDescriptor(cx, obj, "bar", &desc));
+    CHECK(CheckDescriptor(desc, 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_GetPropertyDescriptor(cx, obj, "baz", &desc));
+    CHECK(CheckDescriptor(desc, 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(CheckDescriptor(desc, false, false, true));
+
+    // Just make it writable. Leave the old value and everythign else alone.
+    defineValue.setUndefined();
+    CHECK(JS_DefineProperty(cx, obj, "quox", defineValue, AllowWritable));
+    CHECK(JS_GetPropertyDescriptor(cx, obj, "quox", &desc));
+    CHECK(CheckDescriptor(desc, false, true, true));
+    CHECK_SAME(ObjectValue(*obj), desc.value());
+
+    return true;
+}
+END_TEST(testDefinePropertyIgnoredAttributes)
--- a/js/src/jsapi.h
+++ b/js/src/jsapi.h
@@ -889,17 +889,22 @@ class MOZ_STACK_CLASS SourceBufferHolder
     size_t length_;
     bool ownsChars_;
 };
 
 } /* namespace JS */
 
 /************************************************************************/
 
-/* Property attributes, set in JSPropertySpec and passed to API functions. */
+/* Property attributes, set in JSPropertySpec and passed to API functions.
+ *
+ * NB: The data structure in which some of these values are stored only uses
+ *     a uint8_t to store the relevant information. Proceed with caution if
+ *     trying to reorder or change the the first byte worth of flags.
+ */
 #define JSPROP_ENUMERATE        0x01    /* property is visible to for/in loop */
 #define JSPROP_READONLY         0x02    /* not settable: assignment is no-op.
                                            This flag is only valid when neither
                                            JSPROP_GETTER nor JSPROP_SETTER is
                                            set. */
 #define JSPROP_PERMANENT        0x04    /* property cannot be deleted */
 #define JSPROP_NATIVE_ACCESSORS 0x08    /* set in JSPropertyDescriptor.flags
                                            if getters/setters are JSNatives */
@@ -913,16 +918,28 @@ class MOZ_STACK_CLASS SourceBufferHolder
 #define JSPROP_INDEX            0x80    /* name is actually (int) index */
 
 #define JSFUN_STUB_GSOPS       0x200    /* use JS_PropertyStub getter/setter
                                            instead of defaulting to class gsops
                                            for property holding function */
 
 #define JSFUN_CONSTRUCTOR      0x400    /* native that can be called as a ctor */
 
+#define JSPROP_IGNORE_ENUMERATE 0x1000  /* ignore the value in JSPROP_ENUMERATE.
+                                           This flag only valid when defining over
+                                           an existing property. */
+#define JSPROP_IGNORE_READONLY  0x2000  /* ignore the value in JSPROP_READONLY.
+                                           This flag only valid when defining over
+                                           an existing property. */
+#define JSPROP_IGNORE_PERMANENT 0x4000  /* ignore the value in JSPROP_PERMANENT.
+                                           This flag only valid when defining over
+                                           an existing property. */
+#define JSPROP_IGNORE_VALUE     0x8000  /* ignore the Value in the descriptor. Nothing was
+                                           specified when passed to Object.defineProperty
+                                           from script. */
 
 /*
  * Specify a generic native prototype methods, i.e., methods of a class
  * prototype that are exposed as static methods taking an extra leading
  * argument: the generic |this| parameter.
  *
  * If you set this flag in a JSFunctionSpec struct's flags initializer, then
  * that struct must live at least as long as the native static method object
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -307,36 +307,48 @@ PropDesc::initFromPropertyDescriptor(Han
         hasValue_ = false;
         value_.setUndefined();
         hasWritable_ = false;
     } else {
         hasGet_ = false;
         get_.setUndefined();
         hasSet_ = false;
         set_.setUndefined();
-        hasValue_ = true;
-        value_ = desc.value();
-        hasWritable_ = true;
-    }
-    hasEnumerable_ = true;
-    hasConfigurable_ = true;
+        hasValue_ = !(desc.attributes() & JSPROP_IGNORE_VALUE);
+        value_ = hasValue_ ? desc.value() : UndefinedValue();
+        hasWritable_ = !(desc.attributes() & JSPROP_IGNORE_READONLY);
+    }
+    hasEnumerable_ = !(desc.attributes() & JSPROP_IGNORE_ENUMERATE);
+    hasConfigurable_ = !(desc.attributes() & JSPROP_IGNORE_PERMANENT);
 }
 
 void
 PropDesc::populatePropertyDescriptor(HandleObject obj, MutableHandle<PropertyDescriptor> desc) const
 {
     if (isUndefined()) {
         desc.object().set(nullptr);
         return;
     }
 
     desc.value().set(hasValue() ? value() : UndefinedValue());
     desc.setGetter(getter());
     desc.setSetter(setter());
-    desc.setAttributes(attributes());
+
+    // Make sure we honor the "has" notions in some way.
+    unsigned attrs = attributes();
+    if (!hasEnumerable())
+        attrs |= JSPROP_IGNORE_ENUMERATE;
+    if (!hasWritable())
+        attrs |= JSPROP_IGNORE_READONLY;
+    if (!hasConfigurable())
+        attrs |= JSPROP_IGNORE_PERMANENT;
+    if (!hasValue())
+        attrs |= JSPROP_IGNORE_VALUE;
+    desc.setAttributes(attrs);
+
     desc.object().set(obj);
 }
 
 bool
 PropDesc::makeObject(JSContext *cx, MutableHandleObject obj)
 {
     MOZ_ASSERT(!isUndefined());
 
@@ -649,33 +661,91 @@ Reject(JSContext *cx, HandleId id, unsig
 {
     if (throwError)
         return Throw(cx, id, errorNumber);
 
     *rval = false;
     return true;
 }
 
-// See comments on CheckDefineProperty in jsobj.h.
+static unsigned
+ApplyAttributes(unsigned attrs, bool enumerable, bool writable, bool configurable)
+{
+    /*
+     * Respect the fact that some callers may want to preserve existing attributes as much as
+     * possible, or install defaults otherwise.
+     */
+    if (attrs & JSPROP_IGNORE_ENUMERATE) {
+        attrs &= ~JSPROP_IGNORE_ENUMERATE;
+        if (enumerable)
+            attrs |= JSPROP_ENUMERATE;
+        else
+            attrs &= ~JSPROP_ENUMERATE;
+    }
+    if (attrs & JSPROP_IGNORE_READONLY) {
+        attrs &= ~JSPROP_IGNORE_READONLY;
+        // Only update the writability if it's relevant
+        if (!(attrs & (JSPROP_GETTER | JSPROP_SETTER))) {
+            if (!writable)
+                attrs |= JSPROP_READONLY;
+            else
+                attrs &= ~JSPROP_READONLY;
+        }
+    }
+    if (attrs & JSPROP_IGNORE_PERMANENT) {
+        attrs &= ~JSPROP_IGNORE_PERMANENT;
+        if (!configurable)
+            attrs |= JSPROP_PERMANENT;
+        else
+            attrs &= ~JSPROP_PERMANENT;
+    }
+    return attrs;
+}
+
+static unsigned
+ApplyOrDefaultAttributes(unsigned attrs, const Shape *shape = nullptr)
+{
+    bool enumerable = shape ? shape->enumerable() : false;
+    bool writable = shape ? shape->writable() : false;
+    bool configurable = shape ? shape->configurable() : false;
+    return ApplyAttributes(attrs, enumerable, writable, configurable);
+}
+
+static unsigned
+ApplyOrDefaultAttributes(unsigned attrs, Handle<PropertyDescriptor> desc)
+{
+    bool present = !!desc.object();
+    bool enumerable = present ? desc.isEnumerable() : false;
+    bool writable = present ? !desc.isReadonly() : false;
+    bool configurable = present ? !desc.isPermanent() : false;
+    return ApplyAttributes(attrs, enumerable, writable, configurable);
+}
+
+// See comments on CheckDefineProperty in jsfriendapi.h.
 //
 // DefinePropertyOnObject has its own implementation of these checks.
 //
 JS_FRIEND_API(bool)
 js::CheckDefineProperty(JSContext *cx, HandleObject obj, HandleId id, HandleValue value,
                         unsigned attrs, PropertyOp getter, StrictPropertyOp setter)
 {
     if (!obj->isNative())
         return true;
 
     // ES5 8.12.9 Step 1. Even though we know obj is native, we use generic
     // APIs for shorter, more readable code.
     Rooted<PropertyDescriptor> desc(cx);
     if (!GetOwnPropertyDescriptor(cx, obj, id, &desc))
         return false;
 
+    // Appropriately handle the potential for ignored attributes. Since the proxy code calls us
+    // directly, these might flow in legitimately. Ensure that we compare against the values that
+    // are intended.
+    attrs = ApplyOrDefaultAttributes(attrs, desc) & ~JSPROP_IGNORE_VALUE;
+
     // This does not have to check obj's extensibility when !desc.obj (steps
     // 2-3) because the low-level methods JSObject::{add,put}Property check
     // for that.
     if (desc.object() && desc.isPermanent()) {
         // Steps 6-11, skipping step 10.a.ii. Prohibit redefining a permanent
         // property with different metadata, except to make a writable property
         // non-writable.
         if ((getter != desc.getter() && !(getter == JS_PropertyStub && !desc.getter())) ||
@@ -4066,78 +4136,120 @@ NativeLookupOwnProperty(ExclusiveContext
 bool
 js::DefineNativeProperty(ExclusiveContext *cx, HandleObject obj, HandleId id, HandleValue value,
                          PropertyOp getter, StrictPropertyOp setter, unsigned attrs)
 {
     JS_ASSERT(!(attrs & JSPROP_NATIVE_ACCESSORS));
 
     AutoRooterGetterSetter gsRoot(cx, attrs, &getter, &setter);
 
+    RootedShape shape(cx);
+    RootedValue updateValue(cx, value);
+    bool shouldDefine = true;
+
     /*
      * If defining a getter or setter, we must check for its counterpart and
      * update the attributes and property ops.  A getter or setter is really
      * only half of a property.
      */
-    RootedShape shape(cx);
     if (attrs & (JSPROP_GETTER | JSPROP_SETTER)) {
-        /*
-         * If we are defining a getter whose setter was already defined, or
-         * vice versa, finish the job via obj->changeProperty.
-         */
         if (!NativeLookupOwnProperty(cx, obj, id, &shape))
             return false;
         if (shape) {
+            /*
+             * If we are defining a getter whose setter was already defined, or
+             * vice versa, finish the job via obj->changeProperty.
+             */
             if (IsImplicitDenseOrTypedArrayElement(shape)) {
                 if (obj->is<TypedArrayObject>()) {
                     /* Ignore getter/setter properties added to typed arrays. */
                     return true;
                 }
                 if (!JSObject::sparsifyDenseElement(cx, obj, JSID_TO_INT(id)))
                     return false;
                 shape = obj->nativeLookup(cx, id);
             }
             if (shape->isAccessorDescriptor()) {
+                attrs = ApplyOrDefaultAttributes(attrs, shape);
                 shape = JSObject::changeProperty<SequentialExecution>(cx, obj, shape, attrs,
                                                                       JSPROP_GETTER | JSPROP_SETTER,
                                                                       (attrs & JSPROP_GETTER)
                                                                       ? getter
                                                                       : shape->getter(),
                                                                       (attrs & JSPROP_SETTER)
                                                                       ? setter
                                                                       : shape->setter());
                 if (!shape)
                     return false;
-            } else {
-                shape = nullptr;
+                shouldDefine = false;
             }
         }
+    } else if (!(attrs & JSPROP_IGNORE_VALUE)) {
+        /*
+         * We might still want to ignore redefining some of our attributes, if the
+         * request came through a proxy after Object.defineProperty(), but only if we're redefining
+         * a data property.
+         * FIXME: All this logic should be removed when Proxies use PropDesc, but we need to
+         *        remove JSPropertyOp getters and setters first.
+         */
+        shape = obj->nativeLookup(cx, id);
+        if (shape && shape->isDataDescriptor())
+            attrs = ApplyOrDefaultAttributes(attrs, shape);
+    } else {
+        /*
+         * We have been asked merely to update some attributes by a caller of
+         * Object.defineProperty, laundered through the proxy system, and returning here. We can
+         * get away with just using JSObject::changeProperty here.
+         */
+        if (!NativeLookupOwnProperty(cx, obj, id, &shape))
+            return false;
+
+        if (shape) {
+            attrs = ApplyOrDefaultAttributes(attrs, shape);
+
+            /* Keep everything from the shape that isn't the things we're changing */
+            unsigned attrMask = ~(JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT);
+            shape = JSObject::changeProperty<SequentialExecution>(cx, obj, shape, attrs, attrMask,
+                                                                  shape->getter(), shape->setter());
+            if (!shape)
+                return false;
+            if (shape->hasSlot())
+                updateValue = obj->getSlot(shape->slot());
+            shouldDefine = false;
+        }
     }
 
     /*
      * Purge the property cache of any properties named by id that are about
      * to be shadowed in obj's scope chain.
      */
     if (!PurgeScopeChain(cx, obj, id))
         return false;
 
     /* Use the object's class getter and setter by default. */
     const Class *clasp = obj->getClass();
     if (!getter && !(attrs & JSPROP_GETTER))
         getter = clasp->getProperty;
     if (!setter && !(attrs & JSPROP_SETTER))
         setter = clasp->setProperty;
 
-    if (!shape) {
+    if (shouldDefine) {
+        // Handle the default cases here. Anyone that wanted to set non-default attributes has
+        // cleared the IGNORE flags by now. Since we can never get here with JSPROP_IGNORE_VALUE
+        // relevant, just clear it.
+        attrs = ApplyOrDefaultAttributes(attrs) & ~JSPROP_IGNORE_VALUE;
         return DefinePropertyOrElement<SequentialExecution>(cx, obj, id, getter, setter,
                                                             attrs, value, false, false);
     }
 
-    JS_ALWAYS_TRUE(UpdateShapeTypeAndValue<SequentialExecution>(cx, obj, shape, value));
-
-    return CallAddPropertyHook<SequentialExecution>(cx, clasp, obj, shape, value);
+    MOZ_ASSERT(shape);
+
+    JS_ALWAYS_TRUE(UpdateShapeTypeAndValue<SequentialExecution>(cx, obj, shape, updateValue));
+
+    return CallAddPropertyHook<SequentialExecution>(cx, clasp, obj, shape, updateValue);
 }
 
 /*
  * Call obj's resolve hook.
  *
  * cx, id, and flags are the parameters initially passed to the ongoing lookup;
  * objp and propp are its out parameters. obj is an object along the prototype
  * chain from where the lookup started.
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_6/Proxy/regress-bug1037770.js
@@ -0,0 +1,7 @@
+foo = 1;
+Object.defineProperty(this, "foo", {writable:false});
+foo = 2;
+assertEq(foo, 1);
+
+if (typeof reportCompare === "function")
+    reportCompare(true, true);