Fixes issue 712 causing non-configurable accessors to be overwritable by using

Object.defineProperty with empty property descriptor.

The issue is fixed by implementing step 5 and 6 from DefineOwnProperty in the
specification (ES5 8.12.9).

This also fixes a bug in SameValue when used on boolean values (it
would priorly return a number - not a boolean).

Review URL: http://codereview.chromium.org/2131019

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@4708 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/runtime.js b/src/runtime.js
index 8e3883f..887436a 100644 (file)
--- a/src/runtime.js
+++ b/src/runtime.js
@@ -562,15 +562,14 @@ function SameValue(x, y) {
   if (IS_NULL_OR_UNDEFINED(x)) return true;
   if (IS_NUMBER(x)) {
     if (NUMBER_IS_NAN(x) && NUMBER_IS_NAN(y)) return true;
-    // x is +0 and y is -0 or vice versa
-    if (x === 0 && y === 0 && !%_IsSmi(x) && !%_IsSmi(y) &&
-        ((1 / x < 0 && 1 / y > 0) || (1 / x > 0 && 1 / y < 0))) {
+    // x is +0 and y is -0 or vice versa.
+    if (x === 0 && y === 0 && (1 / x) != (1 / y)) {
       return false;
     }
     return x == y;
   }
   if (IS_STRING(x)) return %StringEquals(x, y);
-  if (IS_BOOLEAN(x))return %NumberEquals(%ToNumber(x),%ToNumber(y));
+  if (IS_BOOLEAN(x)) return y == x;
 
   return %_ObjectEquals(x, y);
 }

diff --git a/src/v8natives.js b/src/v8natives.js
index 531bd0e..ecb8eee 100644 (file)
--- a/src/v8natives.js
+++ b/src/v8natives.js
@@ -434,6 +434,11 @@ PropertyDescriptor.prototype.isWritable = function() {
 }
 
 
+PropertyDescriptor.prototype.hasWritable = function() {
+  return this.hasWritable_;
+}
+
+
 PropertyDescriptor.prototype.setConfigurable = function(configurable) {
   this.configurable_ = configurable;
   this.hasConfigurable_ = true;
@@ -537,6 +542,22 @@ function DefineOwnProperty(obj, p, desc, should_throw) {
     throw MakeTypeError("define_disallowed", ["defineProperty"]);
 
   if (!IS_UNDEFINED(current) && !current.isConfigurable()) {
+      // Step 5 and 6
+     if ((!desc.hasEnumerable() || 
+          SameValue(desc.isEnumerable() && current.isEnumerable())) &&
+         (!desc.hasConfigurable() || 
+          SameValue(desc.isConfigurable(), current.isConfigurable())) &&
+         (!desc.hasWritable() || 
+          SameValue(desc.isWritable(), current.isWritable())) &&
+         (!desc.hasValue() ||
+          SameValue(desc.getValue(), current.getValue())) &&
+         (!desc.hasGetter() ||
+          SameValue(desc.getGet(), current.getGet())) &&
+         (!desc.hasSetter() ||
+          SameValue(desc.getSet(), current.getSet()))) {
+       return true;
+     }
+                       
     // Step 7
     if (desc.isConfigurable() ||  desc.isEnumerable() != current.isEnumerable())
       throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
@@ -673,8 +694,9 @@ function ObjectCreate(proto, properties) {
 // ES5 section 15.2.3.6.
 function ObjectDefineProperty(obj, p, attributes) {
   if ((!IS_SPEC_OBJECT_OR_NULL(obj) || IS_NULL_OR_UNDEFINED(obj)) &&
-      !IS_UNDETECTABLE(obj))
+      !IS_UNDETECTABLE(obj)) {
     throw MakeTypeError("obj_ctor_property_non_object", ["defineProperty"]);
+  }
   var name = ToString(p);
   var desc = ToPropertyDescriptor(attributes);
   DefineOwnProperty(obj, name, desc, true);

diff --git a/test/mjsunit/object-define-property.js b/test/mjsunit/object-define-property.js
index 43b1c7f..ba50767 100644 (file)
--- a/test/mjsunit/object-define-property.js
+++ b/test/mjsunit/object-define-property.js
@@ -53,36 +53,46 @@ try {
   assertTrue(/called on non-object/.test(e));
 }
 
-// Object
+// Object.
 var obj1 = {};
 
-// Values
+// Values.
 var val1 = 0;
 var val2 = 0;
 var val3 = 0;
 
-// Descriptors
+function setter1() {val1++; }
+function getter1() {return val1; }
+
+function setter2() {val2++; }
+function getter2() {return val2; }
+
+function setter3() {val3++; }
+function getter3() {return val3; }
+
+
+// Descriptors.
 var emptyDesc = {};
 
 var accessorConfigurable = { 
-    set: function() { val1++; },
-    get: function() { return val1; },
+    set: setter1,
+    get: getter1,
     configurable: true
 };
 
 var accessorNoConfigurable = {
-    set: function() { val2++; },
-    get: function() { return val2; },
+    set: setter2,
+    get: getter2,
     configurable: false 
 };
 
 var accessorOnlySet = {
-  set: function() { val3++; },
+  set: setter3,
   configurable: true
 };
 
 var accessorOnlyGet = {
-  get: function() { return val3; },
+  get: getter3,
   configurable: true
 };
 
@@ -200,7 +210,7 @@ assertEquals(2, val1);
 assertEquals(4, val2);
 assertEquals(4, obj1.bar);
 
-// Define an accessor that has only a setter
+// Define an accessor that has only a setter.
 Object.defineProperty(obj1, "setOnly", accessorOnlySet);
 desc = Object.getOwnPropertyDescriptor(obj1, "setOnly");
 assertTrue(desc.configurable);
@@ -212,7 +222,7 @@ assertEquals(desc.get, undefined);
 assertEquals(1, obj1.setOnly = 1);
 assertEquals(1, val3);
 
-// Add a getter - should not touch the setter
+// Add a getter - should not touch the setter.
 Object.defineProperty(obj1, "setOnly", accessorOnlyGet);
 desc = Object.getOwnPropertyDescriptor(obj1, "setOnly");
 assertTrue(desc.configurable);
@@ -256,7 +266,7 @@ obj1.foobar = 1001;
 assertEquals(obj1.foobar, 1000);
 
 
-// Redefine to writable descriptor - now writing to foobar should be allowed
+// Redefine to writable descriptor - now writing to foobar should be allowed.
 Object.defineProperty(obj1, "foobar", dataWritable);
 desc = Object.getOwnPropertyDescriptor(obj1, "foobar");
 assertEquals(obj1.foobar, 3000);
@@ -375,7 +385,7 @@ assertEquals(desc.set, undefined);
 
 
 // Redefinition of an accessor defined using __defineGetter__ and 
-// __defineSetter__
+// __defineSetter__.
 function get(){return this.x}
 function set(x){this.x=x};
 
@@ -442,7 +452,7 @@ assertEquals(1, obj4.bar = 1);
 assertEquals(5, val1);
 assertEquals(5, obj4.bar);
 
-// Make sure an error is thrown when trying to access to redefined function
+// Make sure an error is thrown when trying to access to redefined function.
 try {
   obj4.bar();
   assertTrue(false);
@@ -453,7 +463,7 @@ try {
 
 // Test runtime calls to DefineOrRedefineDataProperty and
 // DefineOrRedefineAccessorProperty - make sure we don't 
-// crash
+// crash.
 try {
   %DefineOrRedefineAccessorProperty(0, 0, 0, 0, 0);
 } catch (e) {
@@ -497,3 +507,210 @@ try {
 } catch (e) {
   assertTrue(/illegal access/.test(e));
 }
+
+// Test that all possible differences in step 6 in DefineOwnProperty are
+// exercised, i.e., any difference in the given property descriptor and the
+// existing properties should not return true, but throw an error if the
+// existing configurable property is false. 
+
+var obj5 = {};
+// Enumerable will default to false.
+Object.defineProperty(obj5, 'foo', accessorNoConfigurable);
+desc = Object.getOwnPropertyDescriptor(obj5, 'foo');
+// First, test that we are actually allowed to set the accessor if all
+// values are of the descriptor are the same as the existing one.
+Object.defineProperty(obj5, 'foo', accessorNoConfigurable);
+
+// Different setter.
+var descDifferent = {
+  configurable:false,
+  enumerable:false,
+  set: setter1,
+  get: getter2
+};
+
+try {
+  Object.defineProperty(obj5, 'foo', descDifferent);
+  assertTrue(false);
+} catch (e) {
+  assertTrue(/Cannot redefine property/.test(e));
+}
+
+// Different getter.
+descDifferent = {
+  configurable:false,
+  enumerable:false,
+  set: setter2,
+  get: getter1
+};
+
+try {
+  Object.defineProperty(obj5, 'foo', descDifferent);
+  assertTrue(false);
+} catch (e) {
+  assertTrue(/Cannot redefine property/.test(e));
+}
+
+// Different enumerable.
+descDifferent = {
+  configurable:false,
+  enumerable:true,
+  set: setter2,
+  get: getter2
+};
+
+try {
+  Object.defineProperty(obj5, 'foo', descDifferent);
+  assertTrue(false);
+} catch (e) {
+  assertTrue(/Cannot redefine property/.test(e));
+}
+
+// Different configurable.
+descDifferent = {
+  configurable:false,
+  enumerable:true,
+  set: setter2,
+  get: getter2
+};
+
+try {
+  Object.defineProperty(obj5, 'foo', descDifferent);
+  assertTrue(false);
+} catch (e) {
+  assertTrue(/Cannot redefine property/.test(e));
+}
+
+// No difference.
+descDifferent = {
+  configurable:false,
+  enumerable:false,
+  set: setter2,
+  get: getter2
+};
+// Make sure we can still redefine if all properties are the same.
+Object.defineProperty(obj5, 'foo', descDifferent);
+
+// Make sure that obj5 still holds the original values.
+desc = Object.getOwnPropertyDescriptor(obj5, 'foo');
+assertEquals(desc.get, getter2);
+assertEquals(desc.set, setter2);
+assertFalse(desc.enumerable);
+assertFalse(desc.configurable);
+
+
+// Also exercise step 6 on data property, writable and enumerable
+// defaults to false.
+Object.defineProperty(obj5, 'bar', dataNoConfigurable);
+
+// Test that redefinition with the same property descriptor is possible
+Object.defineProperty(obj5, 'bar', dataNoConfigurable);
+
+// Different value.
+descDifferent = {
+  configurable:false,
+  enumerable:false,
+  writable: false,
+  value: 1999
+};
+
+try {
+  Object.defineProperty(obj5, 'bar', descDifferent);
+  assertTrue(false);
+} catch (e) {
+  assertTrue(/Cannot redefine property/.test(e));
+}
+
+// Different writable.
+descDifferent = {
+  configurable:false,
+  enumerable:false,
+  writable: true,
+  value: 2000
+};
+
+try {
+  Object.defineProperty(obj5, 'bar', descDifferent);
+  assertTrue(false);
+} catch (e) {
+  assertTrue(/Cannot redefine property/.test(e));
+}
+
+
+// Different enumerable.
+descDifferent = {
+  configurable:false,
+  enumerable:true ,
+  writable:false,
+  value: 2000
+};
+
+try {
+  Object.defineProperty(obj5, 'bar', descDifferent);
+  assertTrue(false);
+} catch (e) {
+  assertTrue(/Cannot redefine property/.test(e));
+}
+
+
+// Different configurable.
+descDifferent = {
+  configurable:true,
+  enumerable:false,
+  writable:false,
+  value: 2000
+};
+
+try {
+  Object.defineProperty(obj5, 'bar', descDifferent);
+  assertTrue(false);
+} catch (e) {
+  assertTrue(/Cannot redefine property/.test(e));
+}
+
+// No difference.
+descDifferent = {
+  configurable:false,
+  enumerable:false,
+  writable:false,
+  value:2000
+};
+// Make sure we can still redefine if all properties are the same.
+Object.defineProperty(obj5, 'bar', descDifferent);
+
+// Make sure that obj5 still holds the original values.
+desc = Object.getOwnPropertyDescriptor(obj5, 'bar');
+assertEquals(desc.value, 2000);
+assertFalse(desc.writable);
+assertFalse(desc.enumerable);
+assertFalse(desc.configurable);
+
+
+// Make sure that we can't overwrite +0 with -0 and vice versa.
+var descMinusZero = {value: -0, configurable: false};
+var descPlusZero = {value: +0, configurable: false};
+
+Object.defineProperty(obj5, 'minuszero', descMinusZero);
+
+// Make sure we can redefine with -0.
+Object.defineProperty(obj5, 'minuszero', descMinusZero);
+
+try {
+  Object.defineProperty(obj5, 'minuszero', descPlusZero);
+  assertUnreachable();
+} catch (e) {
+  assertTrue(/Cannot redefine property/.test(e));
+}
+
+
+Object.defineProperty(obj5, 'pluszero', descPlusZero);
+
+// Make sure we can redefine with +0.
+Object.defineProperty(obj5, 'pluszero', descPlusZero);
+
+try {
+  Object.defineProperty(obj5, 'pluszero', descMinusZero);
+  assertUnreachable();
+} catch (e) {
+  assertTrue(/Cannot redefine property/.test(e));
+}

diff --git a/test/mjsunit/regress/regress-712.js b/test/mjsunit/regress/regress-712.js
new file mode 100644 (file)
index 0000000..b26b94a
--- /dev/null
+++ b/test/mjsunit/regress/regress-712.js
@@ -0,0 +1,38 @@
+// Copyright 2010 the V8 project authors. All rights reserved.
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+//       notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+//       copyright notice, this list of conditions and the following
+//       disclaimer in the documentation and/or other materials provided
+//       with the distribution.
+//     * Neither the name of Google Inc. nor the names of its
+//       contributors may be used to endorse or promote products derived
+//       from this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+// This regression test is used to ensure that Object.defineProperty
+// can't be called with an empty property descriptor on a non-configurable
+// existing property and override the existing property.
+// See: http://code.google.com/p/v8/issues/detail?id=712
+
+var obj = {};
+Object.defineProperty(obj, "x", { get: function() { return "42"; },
+                                  configurable: false });
+assertEquals(obj.x, "42");
+Object.defineProperty(obj, 'x', {});
+assertEquals(obj.x, "42");