Revision: 6220
Author: [email protected]
Date: Fri Jan 7 03:49:09 2011
Log: Landing for Peter Hallam
First cut at bug 992
Fixes JS portion of DefineOwnProperty when there is
an existing property and the new descriptor is generic.
Makes code follow spec steps more closely.
Fixes typo for check for unchanged enumerable in step 6.
Adds regression test.
Codereview url: http://codereview.chromium.org/6035014/
http://code.google.com/p/v8/source/detail?r=6220
Modified:
/branches/bleeding_edge/src/runtime.cc
/branches/bleeding_edge/src/v8natives.js
/branches/bleeding_edge/test/mjsunit/object-define-property.js
=======================================
--- /branches/bleeding_edge/src/runtime.cc Fri Jan 7 02:51:44 2011
+++ /branches/bleeding_edge/src/runtime.cc Fri Jan 7 03:49:09 2011
@@ -3499,7 +3499,12 @@
args.at<Object>(1));
}
-
+// Implements part of 8.12.9 DefineOwnProperty.
+// There are 3 cases that lead here:
+// Step 4b - define a new accessor property.
+// Steps 9c & 12 - replace an existing data property with an accessor
property.
+// Step 12 - update an existing accessor property with an accessor or
generic
+// descriptor.
static MaybeObject* Runtime_DefineOrRedefineAccessorProperty(Arguments
args) {
ASSERT(args.length() == 5);
HandleScope scope;
@@ -3531,6 +3536,12 @@
return obj->DefineAccessor(name, flag_setter->value() == 0, fun, attr);
}
+// Implements part of 8.12.9 DefineOwnProperty.
+// There are 3 cases that lead here:
+// Step 4a - define a new data property.
+// Steps 9b & 12 - replace an existing accessor property with a data
property.
+// Step 12 - update an existing data property with a data or generic
+// descriptor.
static MaybeObject* Runtime_DefineOrRedefineDataProperty(Arguments args) {
ASSERT(args.length() == 4);
HandleScope scope;
=======================================
--- /branches/bleeding_edge/src/v8natives.js Wed Jan 5 05:52:00 2011
+++ /branches/bleeding_edge/src/v8natives.js Fri Jan 7 03:49:09 2011
@@ -545,10 +545,12 @@
if (IS_UNDEFINED(current) && !extensible)
throw MakeTypeError("define_disallowed", ["defineProperty"]);
- if (!IS_UNDEFINED(current) && !current.isConfigurable()) {
+ if (!IS_UNDEFINED(current)) {
// Step 5 and 6
- if ((!desc.hasEnumerable() ||
- SameValue(desc.isEnumerable() && current.isEnumerable())) &&
+ if ((IsGenericDescriptor(desc) ||
+ IsDataDescriptor(desc) == IsDataDescriptor(current)) &&
+ (!desc.hasEnumerable() ||
+ SameValue(desc.isEnumerable(), current.isEnumerable())) &&
(!desc.hasConfigurable() ||
SameValue(desc.isConfigurable(), current.isConfigurable())) &&
(!desc.hasWritable() ||
@@ -561,30 +563,36 @@
SameValue(desc.getSet(), current.getSet()))) {
return true;
}
-
- // Step 7
- if (desc.isConfigurable() || desc.isEnumerable() !=
current.isEnumerable())
- throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
- // Step 9
- if (IsDataDescriptor(current) != IsDataDescriptor(desc))
- throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
- // Step 10
- if (IsDataDescriptor(current) && IsDataDescriptor(desc)) {
- if (!current.isWritable() && desc.isWritable())
+ if (!current.isConfigurable()) {
+ // Step 7
+ if (desc.isConfigurable() ||
+ (desc.hasEnumerable() &&
+ desc.isEnumerable() != current.isEnumerable()))
throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
- if (!current.isWritable() && desc.hasValue() &&
- !SameValue(desc.getValue(), current.getValue())) {
- throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
+ // Step 8
+ if (!IsGenericDescriptor(desc)) {
+ // Step 9a
+ if (IsDataDescriptor(current) != IsDataDescriptor(desc))
+ throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
+ // Step 10a
+ if (IsDataDescriptor(current) && IsDataDescriptor(desc)) {
+ if (!current.isWritable() && desc.isWritable())
+ throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
+ if (!current.isWritable() && desc.hasValue() &&
+ !SameValue(desc.getValue(), current.getValue())) {
+ throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
+ }
+ }
+ // Step 11
+ if (IsAccessorDescriptor(desc) && IsAccessorDescriptor(current)) {
+ if (desc.hasSetter() && !SameValue(desc.getSet(),
current.getSet())){
+ throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
+ }
+ if (desc.hasGetter()
&& !SameValue(desc.getGet(),current.getGet()))
+ throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
+ }
}
}
- // Step 11
- if (IsAccessorDescriptor(desc) && IsAccessorDescriptor(current)) {
- if (desc.hasSetter() && !SameValue(desc.getSet(), current.getSet())){
- throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
- }
- if (desc.hasGetter() && !SameValue(desc.getGet(),current.getGet()))
- throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
- }
}
// Send flags - enumerable and configurable are common - writable is
@@ -607,7 +615,16 @@
} else
flag |= DONT_DELETE;
- if (IsDataDescriptor(desc) || IsGenericDescriptor(desc)) {
+ if (IsDataDescriptor(desc) ||
+ (IsGenericDescriptor(desc) &&
+ (IS_UNDEFINED(current) || IsDataDescriptor(current)))) {
+ // There are 3 cases that lead here:
+ // Step 4a - defining a new data property.
+ // Steps 9b & 12 - replacing an existing accessor property with a data
+ // property.
+ // Step 12 - updating an existing data property with a data or generic
+ // descriptor.
+
if (desc.hasWritable()) {
flag |= desc.isWritable() ? 0 : READ_ONLY;
} else if (!IS_UNDEFINED(current)) {
@@ -615,20 +632,30 @@
} else {
flag |= READ_ONLY;
}
+
var value = void 0; // Default value is undefined.
if (desc.hasValue()) {
value = desc.getValue();
- } else if (!IS_UNDEFINED(current)) {
+ } else if (!IS_UNDEFINED(current) && IsDataDescriptor(current)) {
value = current.getValue();
}
+
%DefineOrRedefineDataProperty(obj, p, value, flag);
+ } else if (IsGenericDescriptor(desc)) {
+ // Step 12 - updating an existing accessor property with a generic
+ // descriptor. Changing flags only.
+ %DefineOrRedefineAccessorProperty(obj, p, GETTER, current.getGet(),
flag);
} else {
- 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()) || IS_UNDEFINED(desc.getSet()))) {
+ // There are 3 cases that lead here:
+ // Step 4b - defining a new accessor property.
+ // Steps 9c & 12 - replacing an existing data property with an accessor
+ // property.
+ // Step 12 - updating an existing accessor property with an accessor
+ // descriptor.
+ if (desc.hasGetter()) {
+ %DefineOrRedefineAccessorProperty(obj, p, GETTER, desc.getGet(),
flag);
+ }
+ if (desc.hasSetter()) {
%DefineOrRedefineAccessorProperty(obj, p, SETTER, desc.getSet(),
flag);
}
}
=======================================
--- /branches/bleeding_edge/test/mjsunit/object-define-property.js Thu Dec
16 04:21:08 2010
+++ /branches/bleeding_edge/test/mjsunit/object-define-property.js Fri Jan
7 03:49:09 2011
@@ -749,13 +749,32 @@
assertTrue(desc.enumerable);
assertFalse(desc.configurable);
-// Ensure that we can't overwrite the non configurable element.
+// Can use defineProperty to change the value of a non
+// configurable property.
try {
Object.defineProperty(obj6, '2', descElement);
+ desc = Object.getOwnPropertyDescriptor(obj6, '2');
+ assertEquals(desc.value, 'foobar');
+} catch (e) {
assertUnreachable();
+}
+
+// Ensure that we can't change the descriptor of a
+// non configurable property.
+try {
+ var descAccessor = { get: function() { return 0; } };
+ Object.defineProperty(obj6, '2', descAccessor);
+ assertUnreachable();
} catch (e) {
assertTrue(/Cannot redefine property/.test(e));
}
+
+Object.defineProperty(obj6, '2', descElementNonWritable);
+desc = Object.getOwnPropertyDescriptor(obj6, '2');
+assertEquals(desc.value, 'foofoo');
+assertFalse(desc.writable);
+assertTrue(desc.enumerable);
+assertFalse(desc.configurable);
Object.defineProperty(obj6, '3', descElementNonWritable);
desc = Object.getOwnPropertyDescriptor(obj6, '3');
@@ -827,13 +846,32 @@
assertTrue(desc.enumerable);
assertFalse(desc.configurable);
-// Ensure that we can't overwrite the non configurable element.
+// Can use defineProperty to change the value of a non
+// configurable property of an array.
try {
Object.defineProperty(arr, '2', descElement);
+ desc = Object.getOwnPropertyDescriptor(arr, '2');
+ assertEquals(desc.value, 'foobar');
+} catch (e) {
assertUnreachable();
+}
+
+// Ensure that we can't change the descriptor of a
+// non configurable property.
+try {
+ var descAccessor = { get: function() { return 0; } };
+ Object.defineProperty(arr, '2', descAccessor);
+ assertUnreachable();
} catch (e) {
assertTrue(/Cannot redefine property/.test(e));
}
+
+Object.defineProperty(arr, '2', descElementNonWritable);
+desc = Object.getOwnPropertyDescriptor(arr, '2');
+assertEquals(desc.value, 'foofoo');
+assertFalse(desc.writable);
+assertTrue(desc.enumerable);
+assertFalse(desc.configurable);
Object.defineProperty(arr, '3', descElementNonWritable);
desc = Object.getOwnPropertyDescriptor(arr, '3');
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
