Revision: 4862
Author: [email protected]
Date: Mon Jun 14 06:55:38 2010
Log: Add support for elements and array indices in Object.defineProperty
(fixes bug 619).
This also fixes a bug in GetOwnProperty in runtime.cc discovered by
the new test cases. That part of the code was not testable before
since we had no way of correctly defining properties on elements.
Review URL: http://codereview.chromium.org/2832001
http://code.google.com/p/v8/source/detail?r=4862
Added:
/branches/bleeding_edge/test/mjsunit/regress/regress-619.js
Deleted:
/branches/bleeding_edge/test/mjsunit/bugs/bug-619.js
Modified:
/branches/bleeding_edge/src/objects.h
/branches/bleeding_edge/src/runtime.cc
/branches/bleeding_edge/test/mjsunit/object-define-property.js
=======================================
--- /dev/null
+++ /branches/bleeding_edge/test/mjsunit/regress/regress-619.js Mon Jun 14
06:55:38 2010
@@ -0,0 +1,61 @@
+// 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.
+
+// Tests that Object.defineProperty works correctly on array indices.
+// Please see http://code.google.com/p/v8/issues/detail?id=619 for details.
+
+var obj = {};
+obj[1] = 42;
+assertEquals(42, obj[1]);
+Object.defineProperty(obj, '1', {value:10, writable:false});
+assertEquals(10, obj[1]);
+
+// We should not be able to override obj[1].
+obj[1] = 5;
+assertEquals(10, obj[1]);
+
+// Try on a range of numbers.
+for(var i = 0; i < 1024; i++) {
+ obj[i] = 42;
+}
+
+for(var i = 0; i < 1024; i++) {
+ Object.defineProperty(obj, i, {value: i, writable:false});
+}
+
+for(var i = 0; i < 1024; i++) {
+ assertEquals(i, obj[i]);
+}
+
+for(var i = 0; i < 1024; i++) {
+ obj[1] = 5;
+}
+
+for(var i = 0; i < 1024; i++) {
+ assertEquals(i, obj[i]);
+}
+
=======================================
--- /branches/bleeding_edge/test/mjsunit/bugs/bug-619.js Fri Feb 19
05:27:43 2010
+++ /dev/null
@@ -1,62 +0,0 @@
-// 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.
-
-// When this bug is corrected move to object-define-property and add
-// additional tests for configurable in the same manner as existing tests
-// there.
-
-var obj = {};
-obj[1] = 42;
-assertEquals(42, obj[1]);
-Object.defineProperty(obj, '1', {value:10, writable:false});
-assertEquals(10, obj[1]);
-
-// We should not be able to override obj[1].
-obj[1] = 5;
-assertEquals(10, obj[1]);
-
-// Try on a range of numbers.
-for(var i = 0; i < 1024; i++) {
- obj[i] = 42;
-}
-
-for(var i = 0; i < 1024; i++) {
- Object.defineProperty(obj, i, {value: i, writable:false});
-}
-
-for(var i = 0; i < 1024; i++) {
- assertEquals(i, obj[i]);
-}
-
-for(var i = 0; i < 1024; i++) {
- obj[1] = 5;
-}
-
-for(var i = 0; i < 1024; i++) {
- assertEquals(i, obj[i]);
-}
-
=======================================
--- /branches/bleeding_edge/src/objects.h Mon Jun 14 04:20:36 2010
+++ /branches/bleeding_edge/src/objects.h Mon Jun 14 06:55:38 2010
@@ -2151,6 +2151,11 @@
// Set the value for entry.
void ValueAtPut(int entry, Object* value) {
+ // Check that this value can actually be written.
+ PropertyDetails details = DetailsAt(entry);
+ // If a value has not been initilized we allow writing to it even if
+ // it is read only (a declared const that has not been initialized).
+ if (details.IsReadOnly() && !ValueAt(entry)->IsTheHole()) return;
this->set(HashTable<Shape, Key>::EntryToIndex(entry)+1, value);
}
=======================================
--- /branches/bleeding_edge/src/runtime.cc Thu Jun 10 02:02:16 2010
+++ /branches/bleeding_edge/src/runtime.cc Mon Jun 14 06:55:38 2010
@@ -626,9 +626,9 @@
PropertyDetails details = dictionary->DetailsAt(entry);
elms->set(IS_ACCESSOR_INDEX, Heap::false_value());
elms->set(VALUE_INDEX, dictionary->ValueAt(entry));
- elms->set(WRITABLE_INDEX, Heap::ToBoolean(!details.IsDontDelete()));
+ elms->set(WRITABLE_INDEX, Heap::ToBoolean(!details.IsReadOnly()));
elms->set(ENUMERABLE_INDEX, Heap::ToBoolean(!details.IsDontEnum()));
- elms->set(CONFIGURABLE_INDEX,
Heap::ToBoolean(!details.IsReadOnly()));
+ elms->set(CONFIGURABLE_INDEX,
Heap::ToBoolean(!details.IsDontDelete()));
return *desc;
} else {
// Elements that are stored as array elements always has:
@@ -3848,12 +3848,30 @@
CONVERT_CHECKED(Smi, flag, args[3]);
int unchecked = flag->value();
RUNTIME_ASSERT((unchecked & ~(READ_ONLY | DONT_ENUM | DONT_DELETE)) ==
0);
+
+ PropertyAttributes attr = static_cast<PropertyAttributes>(unchecked);
+
+ // Check if this is an element.
+ uint32_t index;
+ bool is_element = name->AsArrayIndex(&index);
+
+ // Special case for elements if any of the flags are true.
+ // If elements are in fast case we always implicitly assume that:
+ // DONT_DELETE: false, DONT_ENUM: false, READ_ONLY: false.
+ if (((unchecked & (DONT_DELETE | DONT_ENUM | READ_ONLY)) != 0) &&
+ is_element) {
+ // Normalize the elements to enable attributes on the property.
+ js_object->NormalizeElements();
+ NumberDictionary* dictionary = js_object->element_dictionary();
+ // Make sure that we never go back to fast case.
+ dictionary->set_requires_slow_elements();
+ PropertyDetails details = PropertyDetails(attr, NORMAL);
+ dictionary->Set(index, *obj_value, details);
+ }
LookupResult result;
js_object->LocalLookupRealNamedProperty(*name, &result);
- PropertyAttributes attr = static_cast<PropertyAttributes>(unchecked);
-
// Take special care when attributes are different and there is already
// a property. For simplicity we normalize the property which enables us
// to not worry about changing the instance_descriptor and creating a new
@@ -3869,6 +3887,7 @@
*obj_value,
attr);
}
+
return Runtime::SetObjectProperty(js_object, name, obj_value, attr);
}
=======================================
--- /branches/bleeding_edge/test/mjsunit/object-define-property.js Wed May
26 01:31:57 2010
+++ /branches/bleeding_edge/test/mjsunit/object-define-property.js Mon Jun
14 06:55:38 2010
@@ -714,3 +714,156 @@
} catch (e) {
assertTrue(/Cannot redefine property/.test(e));
}
+
+
+var obj6 = {};
+obj6[1] = 'foo';
+obj6[2] = 'bar';
+obj6[3] = '42';
+obj6[4] = '43';
+obj6[5] = '44';
+
+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',
+ configurable: false,
+ writable: false,
+ enumerable: false };
+
+
+// Redefine existing property.
+Object.defineProperty(obj6, '1', descElement);
+desc = Object.getOwnPropertyDescriptor(obj6, '1');
+assertEquals(desc.value, 'foobar');
+assertTrue(desc.writable);
+assertTrue(desc.enumerable);
+assertTrue(desc.configurable);
+
+// Redefine existing property with configurable: false.
+Object.defineProperty(obj6, '2', descElementNonConfigurable);
+desc = Object.getOwnPropertyDescriptor(obj6, '2');
+assertEquals(desc.value, 'barfoo');
+assertTrue(desc.writable);
+assertTrue(desc.enumerable);
+assertFalse(desc.configurable);
+
+// Ensure that we can't overwrite the non configurable element.
+try {
+ Object.defineProperty(obj6, '2', descElement);
+ assertUnreachable();
+} catch (e) {
+ assertTrue(/Cannot redefine property/.test(e));
+}
+
+Object.defineProperty(obj6, '3', descElementNonWritable);
+desc = Object.getOwnPropertyDescriptor(obj6, '3');
+assertEquals(desc.value, 'foofoo');
+assertFalse(desc.writable);
+assertTrue(desc.enumerable);
+assertTrue(desc.configurable);
+
+// Redefine existing property with configurable: false.
+Object.defineProperty(obj6, '4', descElementNonEnumerable);
+desc = Object.getOwnPropertyDescriptor(obj6, '4');
+assertEquals(desc.value, 'barbar');
+assertTrue(desc.writable);
+assertFalse(desc.enumerable);
+assertTrue(desc.configurable);
+
+// Redefine existing property with configurable: false.
+Object.defineProperty(obj6, '5', descElementAllFalse);
+desc = Object.getOwnPropertyDescriptor(obj6, '5');
+assertEquals(desc.value, 'foofalse');
+assertFalse(desc.writable);
+assertFalse(desc.enumerable);
+assertFalse(desc.configurable);
+
+// Define non existing property - all attributes should default to false.
+Object.defineProperty(obj6, '15', descElement);
+desc = Object.getOwnPropertyDescriptor(obj6, '15');
+assertEquals(desc.value, 'foobar');
+assertFalse(desc.writable);
+assertFalse(desc.enumerable);
+assertFalse(desc.configurable);
+
+// Make sure that we can't redefine using direct access.
+obj6[15] ='overwrite';
+assertEquals(obj6[15],'foobar');
+
+
+// Repeat the above tests on an array.
+var arr = new Array();
+arr[1] = 'foo';
+arr[2] = 'bar';
+arr[3] = '42';
+arr[4] = '43';
+arr[5] = '44';
+
+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',
+ configurable: false,
+ writable: false,
+ enumerable: false };
+
+
+// Redefine existing property.
+Object.defineProperty(arr, '1', descElement);
+desc = Object.getOwnPropertyDescriptor(arr, '1');
+assertEquals(desc.value, 'foobar');
+assertTrue(desc.writable);
+assertTrue(desc.enumerable);
+assertTrue(desc.configurable);
+
+// Redefine existing property with configurable: false.
+Object.defineProperty(arr, '2', descElementNonConfigurable);
+desc = Object.getOwnPropertyDescriptor(arr, '2');
+assertEquals(desc.value, 'barfoo');
+assertTrue(desc.writable);
+assertTrue(desc.enumerable);
+assertFalse(desc.configurable);
+
+// Ensure that we can't overwrite the non configurable element.
+try {
+ Object.defineProperty(arr, '2', descElement);
+ assertUnreachable();
+} catch (e) {
+ assertTrue(/Cannot redefine property/.test(e));
+}
+
+Object.defineProperty(arr, '3', descElementNonWritable);
+desc = Object.getOwnPropertyDescriptor(arr, '3');
+assertEquals(desc.value, 'foofoo');
+assertFalse(desc.writable);
+assertTrue(desc.enumerable);
+assertTrue(desc.configurable);
+
+// Redefine existing property with configurable: false.
+Object.defineProperty(arr, '4', descElementNonEnumerable);
+desc = Object.getOwnPropertyDescriptor(arr, '4');
+assertEquals(desc.value, 'barbar');
+assertTrue(desc.writable);
+assertFalse(desc.enumerable);
+assertTrue(desc.configurable);
+
+// Redefine existing property with configurable: false.
+Object.defineProperty(arr, '5', descElementAllFalse);
+desc = Object.getOwnPropertyDescriptor(arr, '5');
+assertEquals(desc.value, 'foofalse');
+assertFalse(desc.writable);
+assertFalse(desc.enumerable);
+assertFalse(desc.configurable);
+
+// Define non existing property - all attributes should default to false.
+Object.defineProperty(arr, '15', descElement);
+desc = Object.getOwnPropertyDescriptor(arr, '15');
+assertEquals(desc.value, 'foobar');
+assertFalse(desc.writable);
+assertFalse(desc.enumerable);
+assertFalse(desc.configurable);
+
+
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
