Reviewers: Mads Ager,
Description:
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).
Please review this at http://codereview.chromium.org/6518004/
SVN Base: http://v8.googlecode.com/svn/branches/bleeding_edge/
Affected files:
M src/runtime.cc
M src/v8natives.js
A test/mjsunit/regress/regress-crbug-72736.js
Index: src/runtime.cc
===================================================================
--- src/runtime.cc (revision 6761)
+++ src/runtime.cc (working copy)
@@ -3713,7 +3713,7 @@
attr);
}
- return Runtime::SetObjectProperty(js_object, name, obj_value, attr);
+ return Runtime::ForceSetObjectProperty(js_object, name, obj_value, attr);
}
Index: src/v8natives.js
===================================================================
--- src/v8natives.js (revision 6761)
+++ src/v8natives.js (working copy)
@@ -586,17 +586,20 @@
// 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 @@
}
// 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"]);
+ }
}
}
}
Index: test/mjsunit/regress/regress-crbug-72736.js
===================================================================
--- test/mjsunit/regress/regress-crbug-72736.js (revision 0)
+++ test/mjsunit/regress/regress-crbug-72736.js (revision 0)
@@ -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 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);
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
