Revert 6220 (generic descriptor support in Object.defineOwnProperty)

This change caused a webkit failure in http/tests/security/xss-DENIED-defineProperty.html.

 I will look into this and reapply when I find a solution.

Review URL: http://codereview.chromium.org/6134005

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

diff --git a/src/runtime.cc b/src/runtime.cc
index 2cb144f5a0a9e8575e8f3f89a3d517fdff151284..2aa443122c9713e5f11ba00e0b540bd3b3c26445 100644 (file)
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -3499,12 +3499,7 @@ 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;
@@ -3536,12 +3531,6 @@ 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;

diff --git a/src/v8natives.js b/src/v8natives.js
index b4cb7cd3daad88aae864bbde0bde21be075efc77..233f8b4de99dce9da64cc04147b1fee661acc461 100644 (file)
--- a/src/v8natives.js
+++ b/src/v8natives.js
@@ -545,12 +545,10 @@ function DefineOwnProperty(obj, p, desc, should_throw) {
   if (IS_UNDEFINED(current) && !extensible)
     throw MakeTypeError("define_disallowed", ["defineProperty"]);
 
-  if (!IS_UNDEFINED(current)) {
+  if (!IS_UNDEFINED(current) && !current.isConfigurable()) {
     // Step 5 and 6
-    if ((IsGenericDescriptor(desc) ||
-         IsDataDescriptor(desc) == IsDataDescriptor(current)) &&
-        (!desc.hasEnumerable() ||
-         SameValue(desc.isEnumerable(), current.isEnumerable())) &&
+    if ((!desc.hasEnumerable() ||
+         SameValue(desc.isEnumerable() && current.isEnumerable())) &&
         (!desc.hasConfigurable() ||
          SameValue(desc.isConfigurable(), current.isConfigurable())) &&
         (!desc.hasWritable() ||
@@ -563,35 +561,29 @@ function DefineOwnProperty(obj, p, desc, should_throw) {
          SameValue(desc.getSet(), current.getSet()))) {
       return true;
     }
-    if (!current.isConfigurable()) {
-      // Step 7
-      if (desc.isConfigurable() ||
-          (desc.hasEnumerable() &&
-           desc.isEnumerable() != current.isEnumerable()))
+
+    // 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())){
         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"]);
     }
   }
 
@@ -615,16 +607,7 @@ function DefineOwnProperty(obj, p, desc, should_throw) {
   } else
     flag |= DONT_DELETE;
 
-  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 (IsDataDescriptor(desc) || IsGenericDescriptor(desc)) {
     if (desc.hasWritable()) {
       flag |= desc.isWritable() ? 0 : READ_ONLY;
     } else if (!IS_UNDEFINED(current)) {
@@ -632,30 +615,20 @@ 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) && IsDataDescriptor(current)) {
+    } else if (!IS_UNDEFINED(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 {
-    // 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.hasGetter() &&
+        (IS_FUNCTION(desc.getGet()) || IS_UNDEFINED(desc.getGet()))) {
+       %DefineOrRedefineAccessorProperty(obj, p, GETTER, desc.getGet(), flag);
     }
-    if (desc.hasSetter()) {
+    if (desc.hasSetter() &&
+        (IS_FUNCTION(desc.getSet()) || IS_UNDEFINED(desc.getSet()))) {
       %DefineOrRedefineAccessorProperty(obj, p, SETTER, desc.getSet(), flag);
     }
   }

diff --git a/test/mjsunit/object-define-property.js b/test/mjsunit/object-define-property.js
index 89b8622d2253d7877139ad464b195962180e1b5e..d24a4e5a398c0d84a85052283daf0f0516d4f87f 100644 (file)
--- a/test/mjsunit/object-define-property.js
+++ b/test/mjsunit/object-define-property.js
@@ -749,33 +749,14 @@ assertTrue(desc.writable);
 assertTrue(desc.enumerable);
 assertFalse(desc.configurable);
 
-// Can use defineProperty to change the value of a non
-// configurable property.
+// Ensure that we can't overwrite the non configurable element.
 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');
@@ -846,33 +827,14 @@ assertTrue(desc.writable);
 assertTrue(desc.enumerable);
 assertFalse(desc.configurable);
 
-// Can use defineProperty to change the value of a non
-// configurable property of an array.
+// Ensure that we can't overwrite the non configurable element.
 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');