From 04f0e33229fb916da0c8014a0e30ae3138d5c4e3 Mon Sep 17 00:00:00 2001 From: "mstarzinger@chromium.org" Date: Tue, 20 Dec 2011 08:49:51 +0000 Subject: [PATCH] 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. R=rossberg@chromium.org BUG=v8:1530 TEST=mjsunit/regress/regress-1530,test262/S15.3.3.1_A4 Review URL: http://codereview.chromium.org/8996008 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@10279 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/runtime.cc | 26 ++++++++++---- test/mjsunit/regress/regress-1530.js | 69 ++++++++++++++++++++++++++++++++++++ test/test262/test262.status | 3 -- 3 files changed, 89 insertions(+), 9 deletions(-) create mode 100644 test/mjsunit/regress/regress-1530.js diff --git a/src/runtime.cc b/src/runtime.cc index b0e1a05..544b210 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 diff --git a/test/mjsunit/regress/regress-1530.js b/test/mjsunit/regress/regress-1530.js new file mode 100644 index 0000000..db21144 --- /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: {} })"); diff --git a/test/test262/test262.status b/test/test262/test262.status index 5fa0ba8..1da988e 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 -- 2.7.4