Change Object.defineProperty to accept undefined as getters and setters and to correc...

In the past we only accepted functions as argument for setting an
accessor. Since one should be able to set an accessor to undefined
this had to be changed to take either.

In addition, we did not lookup properties in the prototype chain,
causing us to call the setter of an existing accessor up the prototype
chain when trying to replace an existing accessor (that was not local)
with a data property.

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

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

diff --git a/src/objects.cc b/src/objects.cc
index 399ab09..96f5c4b 100644 (file)
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -3097,8 +3097,9 @@ MaybeObject* JSObject::SetPropertyCallback(String* name,
 
 MaybeObject* JSObject::DefineAccessor(String* name,
                                       bool is_getter,
-                                      JSFunction* fun,
+                                      Object* fun,
                                       PropertyAttributes attributes) {
+  ASSERT(fun->IsJSFunction() || fun->IsUndefined());
   // Check access rights if needed.
   if (IsAccessCheckNeeded() &&
       !Top::MayNamedAccess(this, name, v8::ACCESS_SET)) {

diff --git a/src/objects.h b/src/objects.h
index 2b82a2c..498ee45 100644 (file)
--- a/src/objects.h
+++ b/src/objects.h
@@ -1368,7 +1368,7 @@ class JSObject: public HeapObject {
 
   MUST_USE_RESULT MaybeObject* DefineAccessor(String* name,
                                               bool is_getter,
-                                              JSFunction* fun,
+                                              Object* fun,
                                               PropertyAttributes attributes);
   Object* LookupAccessor(String* name, bool is_getter);
 

diff --git a/src/runtime.cc b/src/runtime.cc
index 7f292c6..0fd2f8b 100644 (file)
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -3500,7 +3500,8 @@ static MaybeObject* Runtime_DefineOrRedefineAccessorProperty(Arguments args) {
   CONVERT_ARG_CHECKED(JSObject, obj, 0);
   CONVERT_CHECKED(String, name, args[1]);
   CONVERT_CHECKED(Smi, flag_setter, args[2]);
-  CONVERT_CHECKED(JSFunction, fun, args[3]);
+  Object* fun = args[3];
+  RUNTIME_ASSERT(fun->IsJSFunction() || fun->IsUndefined());
   CONVERT_CHECKED(Smi, flag_attr, args[4]);
   int unchecked = flag_attr->value();
   RUNTIME_ASSERT((unchecked & ~(READ_ONLY | DONT_ENUM | DONT_DELETE)) == 0);
@@ -3556,7 +3557,7 @@ static MaybeObject* Runtime_DefineOrRedefineDataProperty(Arguments args) {
   }
 
   LookupResult result;
-  js_object->LocalLookupRealNamedProperty(*name, &result);
+  js_object->LookupRealNamedProperty(*name, &result);
 
   // Take special care when attributes are different and there is already
   // a property. For simplicity we normalize the property which enables us
@@ -3564,7 +3565,8 @@ static MaybeObject* Runtime_DefineOrRedefineDataProperty(Arguments args) {
   // map. The current version of SetObjectProperty does not handle attributes
   // correctly in the case where a property is a field and is reset with
   // new attributes.
-  if (result.IsProperty() && attr != result.GetAttributes()) {
+  if (result.IsProperty() &&
+      (attr != result.GetAttributes() || result.type() == CALLBACKS)) {
     // New attributes - normalize to avoid writing to instance descriptor
     NormalizeProperties(js_object, CLEAR_INOBJECT_PROPERTIES, 0);
     // Use IgnoreAttributes version since a readonly property may be

diff --git a/src/v8natives.js b/src/v8natives.js
index 4a1fd35..9fd2162 100644 (file)
--- a/src/v8natives.js
+++ b/src/v8natives.js
@@ -563,7 +563,7 @@ function DefineOwnProperty(obj, p, desc, should_throw) {
     }
 
     // Step 7
-    if (desc.isConfigurable() ||  desc.isEnumerable() != current.isEnumerable())
+    if (desc.isConfigurable() || desc.isEnumerable() != current.isEnumerable())
       throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
     // Step 9
     if (IsDataDescriptor(current) != IsDataDescriptor(desc))
@@ -623,10 +623,12 @@ function DefineOwnProperty(obj, p, desc, should_throw) {
     }
     %DefineOrRedefineDataProperty(obj, p, value, flag);
   } else {
-    if (desc.hasGetter() && IS_FUNCTION(desc.getGet())) {
+    if (desc.hasGetter() &&
+        (IS_FUNCTION(desc.getGet()) || IS_UNDEFINED(desc.getGet()))) {
        %DefineOrRedefineAccessorProperty(obj, p, GETTER, desc.getGet(), flag);
     }
-    if (desc.hasSetter() && IS_FUNCTION(desc.getSet())) {
+    if (desc.hasSetter() &&
+        (IS_FUNCTION(desc.getSet()) || IS_UNDEFINED(desc.getSet()))) {
       %DefineOrRedefineAccessorProperty(obj, p, SETTER, desc.getSet(), flag);
     }
   }

diff --git a/test/mjsunit/object-define-property.js b/test/mjsunit/object-define-property.js
index 780c720..d24a4e5 100644 (file)
--- a/test/mjsunit/object-define-property.js
+++ b/test/mjsunit/object-define-property.js
@@ -74,7 +74,7 @@ function getter3() {return val3; }
 // Descriptors.
 var emptyDesc = {};
 
-var accessorConfigurable = { 
+var accessorConfigurable = {
     set: setter1,
     get: getter1,
     configurable: true
@@ -83,7 +83,7 @@ var accessorConfigurable = {
 var accessorNoConfigurable = {
     set: setter2,
     get: getter2,
-    configurable: false 
+    configurable: false
 };
 
 var accessorOnlySet = {
@@ -234,7 +234,7 @@ assertEquals(desc.value, undefined);
 assertEquals(1, obj1.setOnly = 1);
 assertEquals(2, val3);
 
-// The above should also work if redefining just a getter or setter on 
+// The above should also work if redefining just a getter or setter on
 // an existing property with both a getter and a setter.
 Object.defineProperty(obj1, "both", accessorConfigurable);
 
@@ -384,7 +384,7 @@ assertEquals(desc.get, undefined);
 assertEquals(desc.set, undefined);
 
 
-// Redefinition of an accessor defined using __defineGetter__ and 
+// Redefinition of an accessor defined using __defineGetter__ and
 // __defineSetter__.
 function get(){return this.x}
 function set(x){this.x=x};
@@ -462,7 +462,7 @@ try {
 
 
 // Test runtime calls to DefineOrRedefineDataProperty and
-// DefineOrRedefineAccessorProperty - make sure we don't 
+// DefineOrRedefineAccessorProperty - make sure we don't
 // crash.
 try {
   %DefineOrRedefineAccessorProperty(0, 0, 0, 0, 0);
@@ -511,7 +511,7 @@ try {
 // 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. 
+// existing configurable property is false.
 
 var obj5 = {};
 // Enumerable will default to false.
@@ -727,7 +727,7 @@ var descElement = { value: 'foobar' };
 var descElementNonConfigurable = { value: 'barfoo', configurable: false };
 var descElementNonWritable = { value: 'foofoo', writable: false };
 var descElementNonEnumerable = { value: 'barbar', enumerable: false };
-var descElementAllFalse = { value: 'foofalse', 
+var descElementAllFalse = { value: 'foofalse',
                             configurable: false,
                             writable: false,
                             enumerable: false };
@@ -790,7 +790,7 @@ assertFalse(desc.configurable);
 
 // Make sure that we can't redefine using direct access.
 obj6[15] ='overwrite';
-assertEquals(obj6[15],'foobar'); 
+assertEquals(obj6[15],'foobar');
 
 
 // Repeat the above tests on an array.
@@ -805,7 +805,7 @@ var descElement = { value: 'foobar' };
 var descElementNonConfigurable = { value: 'barfoo', configurable: false };
 var descElementNonWritable = { value: 'foofoo', writable: false };
 var descElementNonEnumerable = { value: 'barbar', enumerable: false };
-var descElementAllFalse = { value: 'foofalse', 
+var descElementAllFalse = { value: 'foofalse',
                             configurable: false,
                             writable: false,
                             enumerable: false };
@@ -898,4 +898,3 @@ Object.defineProperty(o, "x", { writable: false });
 assertEquals(undefined, o.x);
 o.x = 37;
 assertEquals(undefined, o.x);
-

diff --git a/test/mjsunit/regress/regress-687.js b/test/mjsunit/regress/regress-687.js
new file mode 100644 (file)
index 0000000..a917a44
--- /dev/null
+++ b/test/mjsunit/regress/regress-687.js
@@ -0,0 +1,75 @@
+// Copyright 2009 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 includes a number of cases where we did not correctly
+// update a accessor property to a data property using Object.defineProperty.
+
+var obj = { get value() {}, set value (v) { throw "Error";} };
+assertDoesNotThrow(
+    Object.defineProperty(obj, "value",
+                          { value: 5, writable:true, configurable: true }));
+var desc = Object.getOwnPropertyDescriptor(obj, "value");
+assertEquals(obj.value, 5);
+assertTrue(desc.configurable);
+assertTrue(desc.enumerable);
+assertTrue(desc.writable);
+assertEquals(desc.get, undefined);
+assertEquals(desc.set, undefined);
+
+
+var proto = {
+  get value() {},
+  set value(v) { Object.defineProperty(this, "value", {value: v}); }
+};
+
+var create = Object.create(proto);
+
+assertEquals(create.value, undefined);
+assertDoesNotThrow(create.value = 4);
+assertEquals(create.value, 4);
+
+// These tests where provided in bug 959, but are all related to the this issue.
+var obj1 = {};
+Object.defineProperty(obj1, 'p', {get: undefined, set: undefined});
+assertTrue("p" in obj1);
+desc = Object.getOwnPropertyDescriptor(obj1, "p");
+assertFalse(desc.configurable);
+assertFalse(desc.enumerable);
+assertEquals(desc.value, undefined);
+assertEquals(desc.get, undefined);
+assertEquals(desc.set, undefined);
+
+
+var obj2 = { get p() {}};
+Object.defineProperty(obj2, 'p', {get: undefined})
+assertTrue("p" in obj2);
+desc = Object.getOwnPropertyDescriptor(obj2, "p");
+assertTrue(desc.configurable);
+assertTrue(desc.enumerable);
+assertEquals(desc.value, undefined);
+assertEquals(desc.get, undefined);
+assertEquals(desc.set, undefined);