Use ForceSetObjectProperty in DefineOrRedefineDataProperty (fixes crbug 72736).

The current version uses SetObjectProperty which will not set the
value in case this is a readonly property. The spec explictly says
that a configurable but non writable property can have its value
changed with Object.defineProperty (because the same thing can be
accomplished by doing 3 calls (set writable to true, update the value,
set writable to false).

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

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

diff --git a/src/runtime.cc b/src/runtime.cc
index ef7a4ac..599c8ed 100644 (file)
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -3713,7 +3713,7 @@ static MaybeObject* Runtime_DefineOrRedefineDataProperty(Arguments args) {
                                                        attr);
   }
 
-  return Runtime::SetObjectProperty(js_object, name, obj_value, attr);
+  return Runtime::ForceSetObjectProperty(js_object, name, obj_value, attr);
 }
 
 

diff --git a/src/v8natives.js b/src/v8natives.js
index 83b00b0..884b6f4 100644 (file)
--- a/src/v8natives.js
+++ b/src/v8natives.js
@@ -586,17 +586,20 @@ function DefineOwnProperty(obj, p, desc, should_throw) {
       // Step 7
       if (desc.isConfigurable() ||
           (desc.hasEnumerable() &&
-           desc.isEnumerable() != current.isEnumerable()))
+           desc.isEnumerable() != current.isEnumerable())) {
         throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
+      }
       // Step 8
       if (!IsGenericDescriptor(desc)) {
         // Step 9a
-        if (IsDataDescriptor(current) != IsDataDescriptor(desc))
+        if (IsDataDescriptor(current) != IsDataDescriptor(desc)) {
           throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
+        }
         // Step 10a
         if (IsDataDescriptor(current) && IsDataDescriptor(desc)) {
-          if (!current.isWritable() && desc.isWritable())
+          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"]);
@@ -604,11 +607,12 @@ function DefineOwnProperty(obj, p, desc, should_throw) {
         }
         // Step 11
         if (IsAccessorDescriptor(desc) && IsAccessorDescriptor(current)) {
-          if (desc.hasSetter() && !SameValue(desc.getSet(), current.getSet())){
+          if (desc.hasSetter() && !SameValue(desc.getSet(), current.getSet())) {
             throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
           }
-          if (desc.hasGetter() && !SameValue(desc.getGet(),current.getGet()))
+          if (desc.hasGetter() && !SameValue(desc.getGet(),current.getGet())) {
             throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
+          }
         }
       }
     }

diff --git a/test/mjsunit/regress/regress-crbug-72736.js b/test/mjsunit/regress/regress-crbug-72736.js
new file mode 100644 (file)
index 0000000..4b4b145
--- /dev/null
+++ b/test/mjsunit/regress/regress-crbug-72736.js
@@ -0,0 +1,37 @@
+// Copyright 2011 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.
+
+// See http://crbug.com/72736
+
+// This tests that Object.defineProperty actually allows to change the value of
+// a non-writable property if configurable is true.
+
+var obj = {};
+Object.defineProperty(obj, 'foo', { value: 10, configurable: true });
+assertEquals(obj.foo, 10);
+Object.defineProperty(obj, 'foo', { value: 20, configurable: true });
+assertEquals(obj.foo, 20);