Reviewers: rossberg,
Message:
Added new patch set. Landed.
http://codereview.chromium.org/8996008/diff/1/src/runtime.cc
File src/runtime.cc (right):
http://codereview.chromium.org/8996008/diff/1/src/runtime.cc#newcode4333
src/runtime.cc:4333: // in defineProperty. Firefox disagrees here, and
actually changes the value.
On 2011/12/19 16:47:44, rossberg wrote:
80 characters
Done.
http://codereview.chromium.org/8996008/diff/1/src/runtime.cc#newcode4346
src/runtime.cc:4346: kNonStrictMode);
On 2011/12/19 16:47:44, rossberg wrote:
This should be KStrictMode, I think.
Done. On this code path it doesn't actually matter. The mode is only
considered for JavaScript accessors, but not for API nor foreign
callbacks.
http://codereview.chromium.org/8996008/diff/1/test/mjsunit/regress/regress-1530.js
File test/mjsunit/regress/regress-1530.js (right):
http://codereview.chromium.org/8996008/diff/1/test/mjsunit/regress/regress-1530.js#newcode28
test/mjsunit/regress/regress-1530.js:28: // Test that redefining the
'prototype' property of a function object
On 2011/12/19 16:47:44, rossberg wrote:
Maybe test other magic properties of functions, too?
Done. All other properties of function objects for which we use foreign
callbacks are non-writable, but I added checks that
Object.defineProperty() throws on them as expected.
Description:
Fix handling of foreign callbacks in DefineOwnProperty.
We use foreign callbacks to make some properties shadow internal values
but still behave as data properties from within JavaScript. This means
when a value is passed to Object.defineProperty() on such a property,
it should update the internal value instead of redefinind the property
and destroying the shadowing.
[email protected]
BUG=v8:1530
TEST=mjsunit/regress/regress-1530,test262/S15.3.3.1_A4
Committed: http://code.google.com/p/v8/source/detail?r=10279
Please review this at http://codereview.chromium.org/8996008/
SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge
Affected files:
M src/runtime.cc
A test/mjsunit/regress/regress-1530.js
M test/test262/test262.status
Index: src/runtime.cc
diff --git a/src/runtime.cc b/src/runtime.cc
index
b0e1a057da4b4384cedff7372fd0a04775c14988..544b210c0b6f1520ecb2a161608fe81970cb8fd9
100644
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -4326,12 +4326,26 @@ RUNTIME_FUNCTION(MaybeObject*,
Runtime_DefineOrRedefineDataProperty) {
LookupResult result(isolate);
js_object->LocalLookupRealNamedProperty(*name, &result);
- // To be compatible with safari we do not change the value on API objects
- // in defineProperty. Firefox disagrees here, and actually changes the
value.
- if (result.IsProperty() &&
- (result.type() == CALLBACKS) &&
- result.GetCallbackObject()->IsAccessorInfo()) {
- return isolate->heap()->undefined_value();
+ // Special case for callback properties.
+ if (result.IsProperty() && result.type() == CALLBACKS) {
+ Object* callback = result.GetCallbackObject();
+ // To be compatible with Safari we do not change the value on API
objects
+ // in Object.defineProperty(). Firefox disagrees here, and actually
changes
+ // the value.
+ if (callback->IsAccessorInfo()) {
+ return isolate->heap()->undefined_value();
+ }
+ // Avoid redefining foreign callback as data property, just use the
stored
+ // setter to update the value instead.
+ // TODO(mstarzinger): So far this only works if property attributes
don't
+ // change, this should be fixed once we cleanup the underlying code.
+ if (callback->IsForeign() && result.GetAttributes() == attr) {
+ return js_object->SetPropertyWithCallback(callback,
+ *name,
+ *obj_value,
+ result.holder(),
+ kStrictMode);
+ }
}
// Take special care when attributes are different and there is already
Index: test/mjsunit/regress/regress-1530.js
diff --git a/test/mjsunit/regress/regress-1530.js
b/test/mjsunit/regress/regress-1530.js
new file mode 100644
index
0000000000000000000000000000000000000000..db2114450e4133371bc9045e0c409f5154333d01
--- /dev/null
+++ b/test/mjsunit/regress/regress-1530.js
@@ -0,0 +1,69 @@
+// 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.
+
+// Test that redefining the 'prototype' property of a function object
+// does actually set the internal value and does not screw up any
+// shadowing between said property and the internal value.
+
+var f = function() {};
+
+// Verify that normal assignment of 'prototype' property works properly
+// and updates the internal value.
+var x = { foo: 'bar' };
+f.prototype = x;
+assertSame(f.prototype, x);
+assertSame(f.prototype.foo, 'bar');
+assertSame(new f().foo, 'bar');
+assertSame(Object.getPrototypeOf(new f()), x);
+assertSame(Object.getOwnPropertyDescriptor(f, 'prototype').value, x);
+
+// Verify that 'prototype' behaves like a data property when it comes to
+// redefining with Object.defineProperty() and the internal value gets
+// updated.
+var y = { foo: 'baz' };
+Object.defineProperty(f, 'prototype', { value: y, writable: true });
+assertSame(f.prototype, y);
+assertSame(f.prototype.foo, 'baz');
+assertSame(new f().foo, 'baz');
+assertSame(Object.getPrototypeOf(new f()), y);
+assertSame(Object.getOwnPropertyDescriptor(f, 'prototype').value, y);
+
+// Verify that the previous redefinition didn't screw up callbacks and
+// the internal value still gets updated.
+var z = { foo: 'other' };
+f.prototype = z;
+assertSame(f.prototype, z);
+assertSame(f.prototype.foo, 'other');
+assertSame(new f().foo, 'other');
+assertSame(Object.getPrototypeOf(new f()), z);
+assertSame(Object.getOwnPropertyDescriptor(f, 'prototype').value, z);
+
+// Verify that non-writability of other properties is respected.
+assertThrows("Object.defineProperty(f, 'name', { value: {} })");
+assertThrows("Object.defineProperty(f, 'length', { value: {} })");
+assertThrows("Object.defineProperty(f, 'caller', { value: {} })");
+assertThrows("Object.defineProperty(f, 'arguments', { value: {} })");
Index: test/test262/test262.status
diff --git a/test/test262/test262.status b/test/test262/test262.status
index
5fa0ba8518af10a175afa461870b4f4d7c0aae07..1da988efc196a7c79cb9d2865297d67a9640fbec
100644
--- a/test/test262/test262.status
+++ b/test/test262/test262.status
@@ -39,9 +39,6 @@ S8.7_A5_T2: FAIL
# V8 Bug: http://code.google.com/p/v8/issues/detail?id=1624
S10.4.2.1_A1: FAIL
-# V8 Bug: http://code.google.com/p/v8/issues/detail?id=1530
-S15.3.3.1_A4: FAIL
-
# V8 Bug: http://code.google.com/p/v8/issues/detail?id=1475
15.2.3.6-4-405: FAIL
15.2.3.6-4-410: FAIL
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
