Commit 357afa38 authored by ricow@chromium.org's avatar ricow@chromium.org

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

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

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@6045 ce2b1a6d-e550-0410-aec6-3dcde31c8c00
parent e521db4a
...@@ -3097,8 +3097,9 @@ MaybeObject* JSObject::SetPropertyCallback(String* name, ...@@ -3097,8 +3097,9 @@ MaybeObject* JSObject::SetPropertyCallback(String* name,
MaybeObject* JSObject::DefineAccessor(String* name, MaybeObject* JSObject::DefineAccessor(String* name,
bool is_getter, bool is_getter,
JSFunction* fun, Object* fun,
PropertyAttributes attributes) { PropertyAttributes attributes) {
ASSERT(fun->IsJSFunction() || fun->IsUndefined());
// Check access rights if needed. // Check access rights if needed.
if (IsAccessCheckNeeded() && if (IsAccessCheckNeeded() &&
!Top::MayNamedAccess(this, name, v8::ACCESS_SET)) { !Top::MayNamedAccess(this, name, v8::ACCESS_SET)) {
......
...@@ -1368,7 +1368,7 @@ class JSObject: public HeapObject { ...@@ -1368,7 +1368,7 @@ class JSObject: public HeapObject {
MUST_USE_RESULT MaybeObject* DefineAccessor(String* name, MUST_USE_RESULT MaybeObject* DefineAccessor(String* name,
bool is_getter, bool is_getter,
JSFunction* fun, Object* fun,
PropertyAttributes attributes); PropertyAttributes attributes);
Object* LookupAccessor(String* name, bool is_getter); Object* LookupAccessor(String* name, bool is_getter);
......
...@@ -3500,7 +3500,8 @@ static MaybeObject* Runtime_DefineOrRedefineAccessorProperty(Arguments args) { ...@@ -3500,7 +3500,8 @@ static MaybeObject* Runtime_DefineOrRedefineAccessorProperty(Arguments args) {
CONVERT_ARG_CHECKED(JSObject, obj, 0); CONVERT_ARG_CHECKED(JSObject, obj, 0);
CONVERT_CHECKED(String, name, args[1]); CONVERT_CHECKED(String, name, args[1]);
CONVERT_CHECKED(Smi, flag_setter, args[2]); 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]); CONVERT_CHECKED(Smi, flag_attr, args[4]);
int unchecked = flag_attr->value(); int unchecked = flag_attr->value();
RUNTIME_ASSERT((unchecked & ~(READ_ONLY | DONT_ENUM | DONT_DELETE)) == 0); RUNTIME_ASSERT((unchecked & ~(READ_ONLY | DONT_ENUM | DONT_DELETE)) == 0);
...@@ -3556,7 +3557,7 @@ static MaybeObject* Runtime_DefineOrRedefineDataProperty(Arguments args) { ...@@ -3556,7 +3557,7 @@ static MaybeObject* Runtime_DefineOrRedefineDataProperty(Arguments args) {
} }
LookupResult result; LookupResult result;
js_object->LocalLookupRealNamedProperty(*name, &result); js_object->LookupRealNamedProperty(*name, &result);
// Take special care when attributes are different and there is already // Take special care when attributes are different and there is already
// a property. For simplicity we normalize the property which enables us // a property. For simplicity we normalize the property which enables us
...@@ -3564,7 +3565,8 @@ static MaybeObject* Runtime_DefineOrRedefineDataProperty(Arguments args) { ...@@ -3564,7 +3565,8 @@ static MaybeObject* Runtime_DefineOrRedefineDataProperty(Arguments args) {
// map. The current version of SetObjectProperty does not handle attributes // map. The current version of SetObjectProperty does not handle attributes
// correctly in the case where a property is a field and is reset with // correctly in the case where a property is a field and is reset with
// new attributes. // 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 // New attributes - normalize to avoid writing to instance descriptor
NormalizeProperties(js_object, CLEAR_INOBJECT_PROPERTIES, 0); NormalizeProperties(js_object, CLEAR_INOBJECT_PROPERTIES, 0);
// Use IgnoreAttributes version since a readonly property may be // Use IgnoreAttributes version since a readonly property may be
......
...@@ -563,7 +563,7 @@ function DefineOwnProperty(obj, p, desc, should_throw) { ...@@ -563,7 +563,7 @@ function DefineOwnProperty(obj, p, desc, should_throw) {
} }
// Step 7 // Step 7
if (desc.isConfigurable() || desc.isEnumerable() != current.isEnumerable()) if (desc.isConfigurable() || desc.isEnumerable() != current.isEnumerable())
throw MakeTypeError("redefine_disallowed", ["defineProperty"]); throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
// Step 9 // Step 9
if (IsDataDescriptor(current) != IsDataDescriptor(desc)) if (IsDataDescriptor(current) != IsDataDescriptor(desc))
...@@ -623,10 +623,12 @@ function DefineOwnProperty(obj, p, desc, should_throw) { ...@@ -623,10 +623,12 @@ function DefineOwnProperty(obj, p, desc, should_throw) {
} }
%DefineOrRedefineDataProperty(obj, p, value, flag); %DefineOrRedefineDataProperty(obj, p, value, flag);
} else { } 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); %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); %DefineOrRedefineAccessorProperty(obj, p, SETTER, desc.getSet(), flag);
} }
} }
......
...@@ -74,7 +74,7 @@ function getter3() {return val3; } ...@@ -74,7 +74,7 @@ function getter3() {return val3; }
// Descriptors. // Descriptors.
var emptyDesc = {}; var emptyDesc = {};
var accessorConfigurable = { var accessorConfigurable = {
set: setter1, set: setter1,
get: getter1, get: getter1,
configurable: true configurable: true
...@@ -83,7 +83,7 @@ var accessorConfigurable = { ...@@ -83,7 +83,7 @@ var accessorConfigurable = {
var accessorNoConfigurable = { var accessorNoConfigurable = {
set: setter2, set: setter2,
get: getter2, get: getter2,
configurable: false configurable: false
}; };
var accessorOnlySet = { var accessorOnlySet = {
...@@ -234,7 +234,7 @@ assertEquals(desc.value, undefined); ...@@ -234,7 +234,7 @@ assertEquals(desc.value, undefined);
assertEquals(1, obj1.setOnly = 1); assertEquals(1, obj1.setOnly = 1);
assertEquals(2, val3); 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. // an existing property with both a getter and a setter.
Object.defineProperty(obj1, "both", accessorConfigurable); Object.defineProperty(obj1, "both", accessorConfigurable);
...@@ -384,7 +384,7 @@ assertEquals(desc.get, undefined); ...@@ -384,7 +384,7 @@ assertEquals(desc.get, undefined);
assertEquals(desc.set, undefined); assertEquals(desc.set, undefined);
// Redefinition of an accessor defined using __defineGetter__ and // Redefinition of an accessor defined using __defineGetter__ and
// __defineSetter__. // __defineSetter__.
function get(){return this.x} function get(){return this.x}
function set(x){this.x=x}; function set(x){this.x=x};
...@@ -462,7 +462,7 @@ try { ...@@ -462,7 +462,7 @@ try {
// Test runtime calls to DefineOrRedefineDataProperty and // Test runtime calls to DefineOrRedefineDataProperty and
// DefineOrRedefineAccessorProperty - make sure we don't // DefineOrRedefineAccessorProperty - make sure we don't
// crash. // crash.
try { try {
%DefineOrRedefineAccessorProperty(0, 0, 0, 0, 0); %DefineOrRedefineAccessorProperty(0, 0, 0, 0, 0);
...@@ -511,7 +511,7 @@ try { ...@@ -511,7 +511,7 @@ try {
// Test that all possible differences in step 6 in DefineOwnProperty are // Test that all possible differences in step 6 in DefineOwnProperty are
// exercised, i.e., any difference in the given property descriptor and the // 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 properties should not return true, but throw an error if the
// existing configurable property is false. // existing configurable property is false.
var obj5 = {}; var obj5 = {};
// Enumerable will default to false. // Enumerable will default to false.
...@@ -727,7 +727,7 @@ var descElement = { value: 'foobar' }; ...@@ -727,7 +727,7 @@ var descElement = { value: 'foobar' };
var descElementNonConfigurable = { value: 'barfoo', configurable: false }; var descElementNonConfigurable = { value: 'barfoo', configurable: false };
var descElementNonWritable = { value: 'foofoo', writable: false }; var descElementNonWritable = { value: 'foofoo', writable: false };
var descElementNonEnumerable = { value: 'barbar', enumerable: false }; var descElementNonEnumerable = { value: 'barbar', enumerable: false };
var descElementAllFalse = { value: 'foofalse', var descElementAllFalse = { value: 'foofalse',
configurable: false, configurable: false,
writable: false, writable: false,
enumerable: false }; enumerable: false };
...@@ -790,7 +790,7 @@ assertFalse(desc.configurable); ...@@ -790,7 +790,7 @@ assertFalse(desc.configurable);
// Make sure that we can't redefine using direct access. // Make sure that we can't redefine using direct access.
obj6[15] ='overwrite'; obj6[15] ='overwrite';
assertEquals(obj6[15],'foobar'); assertEquals(obj6[15],'foobar');
// Repeat the above tests on an array. // Repeat the above tests on an array.
...@@ -805,7 +805,7 @@ var descElement = { value: 'foobar' }; ...@@ -805,7 +805,7 @@ var descElement = { value: 'foobar' };
var descElementNonConfigurable = { value: 'barfoo', configurable: false }; var descElementNonConfigurable = { value: 'barfoo', configurable: false };
var descElementNonWritable = { value: 'foofoo', writable: false }; var descElementNonWritable = { value: 'foofoo', writable: false };
var descElementNonEnumerable = { value: 'barbar', enumerable: false }; var descElementNonEnumerable = { value: 'barbar', enumerable: false };
var descElementAllFalse = { value: 'foofalse', var descElementAllFalse = { value: 'foofalse',
configurable: false, configurable: false,
writable: false, writable: false,
enumerable: false }; enumerable: false };
...@@ -898,4 +898,3 @@ Object.defineProperty(o, "x", { writable: false }); ...@@ -898,4 +898,3 @@ Object.defineProperty(o, "x", { writable: false });
assertEquals(undefined, o.x); assertEquals(undefined, o.x);
o.x = 37; o.x = 37;
assertEquals(undefined, o.x); assertEquals(undefined, o.x);
// 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);
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment