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/




git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@6220 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/runtime.cc

@@ -3499,7 +3499,12 @@ static MaybeObject* Runtime_KeyedGetProperty(Arguments args) {
                                     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 @@ static MaybeObject* Runtime_DefineOrRedefineAccessorProperty(Arguments args) {
   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;

src/v8natives.js

@@ -545,10 +545,12 @@ function DefineOwnProperty(obj, p, desc, should_throw) {
   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,29 +563,35 @@ function DefineOwnProperty(obj, p, desc, should_throw) {
          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())
-        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())){
+    if (!current.isConfigurable()) {
+      // Step 7
+      if (desc.isConfigurable() ||
+          (desc.hasEnumerable() &&
+           desc.isEnumerable() != current.isEnumerable()))
         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"]);
+        }
       }
-      if (desc.hasGetter() && !SameValue(desc.getGet(),current.getGet()))
-        throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
     }
   }
 
@@ -607,7 +615,16 @@ function DefineOwnProperty(obj, p, desc, should_throw) {
   } 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 @@ function DefineOwnProperty(obj, p, desc, should_throw) {
     } 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);
+    // 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() &&
-        (IS_FUNCTION(desc.getSet()) || IS_UNDEFINED(desc.getSet()))) {
+    if (desc.hasSetter()) {
       %DefineOrRedefineAccessorProperty(obj, p, SETTER, desc.getSet(), flag);
     }
   }

test/mjsunit/object-define-property.js

@@ -749,14 +749,33 @@ assertTrue(desc.writable);
 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');
 assertEquals(desc.value, 'foofoo');
@@ -827,14 +846,33 @@ assertTrue(desc.writable);
 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');
 assertEquals(desc.value, 'foofoo');