Allow PropertyOp-based data properties to be frozen at last-got value (591846, r=jwalden).
authorBrendan Eich <brendan@mozilla.org>
Tue, 25 Jan 2011 18:04:45 -0800
changeset 61685 3f23c77191005967f39559db98d92f0fa64cb1bf
parent 61459 787b1e101a5f6c0a338bf3009ee76082e1e3d2ed
child 61686 13ccf78a3eed6285c0ea28f0d3f493f1fe5c7a90
push idunknown
push userunknown
push dateunknown
reviewersjwalden
bugs591846
milestone2.0b11pre
Allow PropertyOp-based data properties to be frozen at last-got value (591846, r=jwalden).
js/src/jsobj.cpp
js/src/tests/js1_8_5/regress/jstests.list
js/src/tests/js1_8_5/regress/regress-591846.js
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -2108,47 +2108,35 @@ DefinePropertyOnObject(JSContext *cx, JS
              * Determine the current value of the property once, if the current
              * value might actually need to be used or preserved later.  NB: we
              * guard on whether the current property is a data descriptor to
              * avoid calling a getter; we won't need the value if it's not a
              * data descriptor.
              */
             if (shape->isDataDescriptor()) {
                 /*
-                 * Non-standard: if the property is non-configurable and is
-                 * represented by a native getter or setter, don't permit
-                 * redefinition.  We expose properties with native getter/setter
-                 * as though they were data properties, for the most part, but
-                 * in this particular case we must worry about integrity
-                 * concerns for JSAPI users who expected that
-                 * permanent+getter/setter means precisely controlled behavior.
-                 * If we permitted such redefinitions, such a property could be
-                 * "fixed" to some specific previous value, no longer varying
-                 * according to the intent of the native getter/setter for the
-                 * property.
+                 * We must rule out a non-configurable js::PropertyOp-guarded
+                 * property becoming a writable unguarded data property, since
+                 * such a property can have its value changed to one the getter
+                 * and setter preclude.
                  *
-                 * Other engines expose properties of this nature using ECMA
-                 * getter/setter pairs, but we can't because we use them even
-                 * for properties which ECMA specifies as being true data
-                 * descriptors ([].length, Function.length, /regex/.lastIndex,
-                 * &c.).  Longer-term perhaps we should convert such properties
-                 * to use data descriptors (at which point representing a
-                 * descriptor with native getter/setter as an accessor
-                 * descriptor would be fine) and take a small memory hit, but
-                 * for now we'll simply forbid their redefinition.
+                 * A desc lacking writable but with value is a data descriptor
+                 * and we must reject it as if it had writable: true if current
+                 * is writable.
                  */
                 if (!shape->configurable() &&
-                    (!shape->hasDefaultGetter() || !shape->hasDefaultSetter())) {
+                    (!shape->hasDefaultGetter() || !shape->hasDefaultSetter()) &&
+                    desc.isDataDescriptor() &&
+                    (desc.hasWritable ? desc.writable() : shape->writable()))
+                {
                     return Reject(cx, JSMSG_CANT_REDEFINE_PROP, throwError, desc.id, rval);
                 }
 
-                if (!js_NativeGet(cx, obj, obj2, shape, JSGET_NO_METHOD_BARRIER, &v)) {
-                    /* current was dropped when the failure occurred. */
+                if (!js_NativeGet(cx, obj, obj2, shape, JSGET_NO_METHOD_BARRIER, &v))
                     return JS_FALSE;
-                }
             }
 
             if (desc.isDataDescriptor()) {
                 if (!shape->isDataDescriptor())
                     break;
 
                 JSBool same;
                 if (desc.hasValue) {
--- a/js/src/tests/js1_8_5/regress/jstests.list
+++ b/js/src/tests/js1_8_5/regress/jstests.list
@@ -37,16 +37,17 @@ script regress-583429.js
 script regress-584355.js
 script regress-586482-1.js
 script regress-586482-2.js
 script regress-586482-3.js
 script regress-586482-4.js
 script regress-586482-5.js
 script regress-588339.js
 script regress-yarr-regexp.js
+script regress-591846.js
 script regress-592202-1.js
 script regress-592202-2.js
 script regress-592202-3.js
 script regress-592202-4.js
 script regress-592217.js
 script regress-592556-c35.js
 script regress-593256.js
 fails-if(!xulRuntime.shell) script regress-595230-1.js
new file mode 100644
--- /dev/null
+++ b/js/src/tests/js1_8_5/regress/regress-591846.js
@@ -0,0 +1,47 @@
+/*
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/
+ */
+
+function check(obj, name, value, readonly) {
+    // Start with a non-configurable, writable data property implemented via
+    // js::PropertyOp getter and setter.
+    var pd = Object.getOwnPropertyDescriptor(obj, name);
+
+    assertEq(pd.configurable, false, "non-configurable " + name);
+    assertEq(pd.writable, !readonly, "writable " + name);
+
+    try {
+        // Do not allow a transition from js::PropertyOp to writable js::Value
+        // data property.
+        Object.defineProperty(obj, name, {writable: true});
+
+        if (!readonly) {
+            // Do not allow the property denoted by name to become a true data
+            // property via a descriptor that preserves the native property's
+            // writable attribute.
+            Object.defineProperty(obj, name, {value: value});
+        }
+    } catch (e) {
+        assertEq('' + e, "TypeError: can't redefine non-configurable property '" + name + "'");
+    }
+
+    // Now make name non-writable.
+    Object.defineProperty(obj, name, {writable: false})
+
+    // Assert that the right getter result "stuck".
+    assertEq(obj[name], value);
+
+    // Test that it is operationally non-writable now.
+    obj[name] = "eek!";
+    assertEq(obj[name], value);
+}
+
+check(Object, 'caller', null, false);
+check(Object, 'arguments', null, false);
+
+// Reset RegExp.leftContext to the empty string.
+/x/.test('x');
+check(RegExp, 'leftContext', '', true);
+
+reportCompare(0, 0, "ok");