Revision: 6045
Author: [email protected]
Date: Thu Dec 16 04:21:08 2010
Log: Change Object.defineProperty to accept undefined as getters and
setters and to correctly accept overriding an accessor with a data property.
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
http://code.google.com/p/v8/source/detail?r=6045
Added:
/branches/bleeding_edge/test/mjsunit/regress/regress-687.js
Modified:
/branches/bleeding_edge/src/objects.cc
/branches/bleeding_edge/src/objects.h
/branches/bleeding_edge/src/runtime.cc
/branches/bleeding_edge/src/v8natives.js
/branches/bleeding_edge/test/mjsunit/object-define-property.js
=======================================
--- /dev/null
+++ /branches/bleeding_edge/test/mjsunit/regress/regress-687.js Thu Dec 16
04:21:08 2010
@@ -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);
=======================================
--- /branches/bleeding_edge/src/objects.cc Tue Dec 7 03:53:19 2010
+++ /branches/bleeding_edge/src/objects.cc Thu Dec 16 04:21:08 2010
@@ -3097,8 +3097,9 @@
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)) {
=======================================
--- /branches/bleeding_edge/src/objects.h Tue Dec 14 10:53:48 2010
+++ /branches/bleeding_edge/src/objects.h Thu Dec 16 04:21:08 2010
@@ -1368,7 +1368,7 @@
MUST_USE_RESULT MaybeObject* DefineAccessor(String* name,
bool is_getter,
- JSFunction* fun,
+ Object* fun,
PropertyAttributes
attributes);
Object* LookupAccessor(String* name, bool is_getter);
=======================================
--- /branches/bleeding_edge/src/runtime.cc Wed Dec 15 09:11:11 2010
+++ /branches/bleeding_edge/src/runtime.cc Thu Dec 16 04:21:08 2010
@@ -3500,7 +3500,8 @@
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 @@
}
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 @@
// 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
=======================================
--- /branches/bleeding_edge/src/v8natives.js Wed Dec 15 01:31:05 2010
+++ /branches/bleeding_edge/src/v8natives.js Thu Dec 16 04:21:08 2010
@@ -563,7 +563,7 @@
}
// 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 @@
}
%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);
}
}
=======================================
--- /branches/bleeding_edge/test/mjsunit/object-define-property.js Fri Dec
10 03:27:15 2010
+++ /branches/bleeding_edge/test/mjsunit/object-define-property.js Thu Dec
16 04:21:08 2010
@@ -74,7 +74,7 @@
// Descriptors.
var emptyDesc = {};
-var accessorConfigurable = {
+var accessorConfigurable = {
set: setter1,
get: getter1,
configurable: true
@@ -83,7 +83,7 @@
var accessorNoConfigurable = {
set: setter2,
get: getter2,
- configurable: false
+ configurable: false
};
var accessorOnlySet = {
@@ -234,7 +234,7 @@
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.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 @@
// 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 @@
// 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 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 @@
// 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 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 @@
assertEquals(undefined, o.x);
o.x = 37;
assertEquals(undefined, o.x);
-
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
