From de512af0680d45990670568d4ea5784c9113f784 Mon Sep 17 00:00:00 2001 From: "ricow@chromium.org" Date: Mon, 14 Jun 2010 13:55:38 +0000 Subject: [PATCH] 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 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@4862 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/objects.h | 5 + src/runtime.cc | 27 +++- test/mjsunit/object-define-property.js | 153 +++++++++++++++++++++ .../{bugs/bug-619.js => regress/regress-619.js} | 5 +- 4 files changed, 183 insertions(+), 7 deletions(-) rename test/mjsunit/{bugs/bug-619.js => regress/regress-619.js} (93%) diff --git a/src/objects.h b/src/objects.h index 447bb1c..bfaab47 100644 --- a/src/objects.h +++ b/src/objects.h @@ -2151,6 +2151,11 @@ class Dictionary: public HashTable { // 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::EntryToIndex(entry)+1, value); } diff --git a/src/runtime.cc b/src/runtime.cc index 88786e8..b29a1ab 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -626,9 +626,9 @@ static Object* Runtime_GetOwnProperty(Arguments args) { 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: @@ -3849,11 +3849,29 @@ static Object* Runtime_DefineOrRedefineDataProperty(Arguments args) { int unchecked = flag->value(); RUNTIME_ASSERT((unchecked & ~(READ_ONLY | DONT_ENUM | DONT_DELETE)) == 0); + PropertyAttributes attr = static_cast(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(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 @@ static Object* Runtime_DefineOrRedefineDataProperty(Arguments args) { *obj_value, attr); } + return Runtime::SetObjectProperty(js_object, name, obj_value, attr); } diff --git a/test/mjsunit/object-define-property.js b/test/mjsunit/object-define-property.js index 46bfb34..b258aa7 100644 --- a/test/mjsunit/object-define-property.js +++ b/test/mjsunit/object-define-property.js @@ -714,3 +714,156 @@ try { } 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); + + diff --git a/test/mjsunit/bugs/bug-619.js b/test/mjsunit/regress/regress-619.js similarity index 93% rename from test/mjsunit/bugs/bug-619.js rename to test/mjsunit/regress/regress-619.js index ef8ba80..24bdbc1 100644 --- a/test/mjsunit/bugs/bug-619.js +++ b/test/mjsunit/regress/regress-619.js @@ -25,9 +25,8 @@ // (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. +// 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; -- 2.7.4