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

@@ -2151,6 +2151,11 @@ class Dictionary: public HashTable<Shape, Key> {
 
   // 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);
   }
 

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<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 @@ static Object* Runtime_DefineOrRedefineDataProperty(Arguments args) {
                                                           *obj_value,
                                                           attr);
   }
+
   return Runtime::SetObjectProperty(js_object, name, obj_value, attr);
 }
 

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);
+
+

test/mjsunit/bugs/bug-619.js → 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;