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

@@ -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

test/mjsunit/regress/regress-1530.js

+// 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: {} })");

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