Make Function.length and Function.name configurable properties.

ES6 makes the Function object properties "length" and "name"
configurable; switch the implementation over to follow that.

Doing so exposed a problem in the handling of non-writable, but
configurable properties backed by foreign callback accessors
internally. As an optimization, if such an accessor property is
re-defined with a new value, its setter was passed the new value
directly, keeping the property as an accessor property. However, this
is not correct should the property be non-writable, as its setter will
then simply ignore the updated value. Adjust the enabling logic for
this optimization accordingly, along with adding a test.

LOG=N
R=rossberg@chromium.org, rossberg
BUG=v8:3045

Review URL: https://codereview.chromium.org/116083006

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

diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc
index ef802ba..62109b5 100644 (file)
--- a/src/bootstrapper.cc
+++ b/src/bootstrapper.cc
@@ -426,31 +426,32 @@ void Genesis::SetFunctionInstanceDescriptor(
   if (prototypeMode != DONT_ADD_PROTOTYPE) {
     prototype = factory()->NewForeign(&Accessors::FunctionPrototype);
   }
-  PropertyAttributes attribs = static_cast<PropertyAttributes>(
+  PropertyAttributes ro_attribs = static_cast<PropertyAttributes>(
       DONT_ENUM | DONT_DELETE | READ_ONLY);
+  PropertyAttributes roc_attribs = static_cast<PropertyAttributes>(
+      DONT_ENUM | READ_ONLY);
   map->set_instance_descriptors(*descriptors);
 
   {  // Add length.
-    CallbacksDescriptor d(*factory()->length_string(), *length, attribs);
+    CallbacksDescriptor d(*factory()->length_string(), *length, roc_attribs);
     map->AppendDescriptor(&d, witness);
   }
   {  // Add name.
-    CallbacksDescriptor d(*factory()->name_string(), *name, attribs);
+    CallbacksDescriptor d(*factory()->name_string(), *name, roc_attribs);
     map->AppendDescriptor(&d, witness);
   }
   {  // Add arguments.
-    CallbacksDescriptor d(*factory()->arguments_string(), *args, attribs);
+    CallbacksDescriptor d(*factory()->arguments_string(), *args, ro_attribs);
     map->AppendDescriptor(&d, witness);
   }
   {  // Add caller.
-    CallbacksDescriptor d(*factory()->caller_string(), *caller, attribs);
+    CallbacksDescriptor d(*factory()->caller_string(), *caller, ro_attribs);
     map->AppendDescriptor(&d, witness);
   }
   if (prototypeMode != DONT_ADD_PROTOTYPE) {
     // Add prototype.
-    if (prototypeMode == ADD_WRITEABLE_PROTOTYPE) {
-      attribs = static_cast<PropertyAttributes>(attribs & ~READ_ONLY);
-    }
+    PropertyAttributes attribs = (prototypeMode == ADD_WRITEABLE_PROTOTYPE)
+        ? static_cast<PropertyAttributes>(ro_attribs & ~READ_ONLY) : ro_attribs;
     CallbacksDescriptor d(*factory()->prototype_string(), *prototype, attribs);
     map->AppendDescriptor(&d, witness);
   }
@@ -568,14 +569,16 @@ void Genesis::SetStrictFunctionInstanceDescriptor(
       static_cast<PropertyAttributes>(DONT_ENUM | DONT_DELETE);
   PropertyAttributes ro_attribs =
       static_cast<PropertyAttributes>(DONT_ENUM | DONT_DELETE | READ_ONLY);
+  PropertyAttributes roc_attribs =
+      static_cast<PropertyAttributes>(DONT_ENUM | READ_ONLY);
   map->set_instance_descriptors(*descriptors);
 
   {  // Add length.
-    CallbacksDescriptor d(*factory()->length_string(), *length, ro_attribs);
+    CallbacksDescriptor d(*factory()->length_string(), *length, roc_attribs);
     map->AppendDescriptor(&d, witness);
   }
   {  // Add name.
-    CallbacksDescriptor d(*factory()->name_string(), *name, ro_attribs);
+    CallbacksDescriptor d(*factory()->name_string(), *name, roc_attribs);
     map->AppendDescriptor(&d, witness);
   }
   {  // Add arguments.

diff --git a/src/runtime.cc b/src/runtime.cc
index 3596add..d2727db 100644 (file)
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -5068,11 +5068,14 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_DefineOrRedefineDataProperty) {
     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.
+    // Provided a read-only property isn't being reconfigured, 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() && lookup.GetAttributes() == attr) {
+    if (callback->IsForeign() &&
+        lookup.GetAttributes() == attr &&
+        !(attr & READ_ONLY)) {
       Handle<Object> result_object =
           JSObject::SetPropertyWithCallback(js_object,
                                             callback,

diff --git a/test/cctest/test-accessors.cc b/test/cctest/test-accessors.cc
index bda09f0..602f5ec 100644 (file)
--- a/test/cctest/test-accessors.cc
+++ b/test/cctest/test-accessors.cc
@@ -624,3 +624,68 @@ THREADED_TEST(GlobalObjectAccessor) {
   CHECK(v8::Utils::OpenHandle(*CompileRun("getter()"))->IsJSGlobalProxy());
   CHECK(v8::Utils::OpenHandle(*CompileRun("set_value"))->IsJSGlobalProxy());
 }
+
+
+static i::MaybeObject* ZeroAccessorGet(i::Isolate*, i::Object*, void*) {
+  return i::Smi::FromInt(0);
+}
+
+
+static i::MaybeObject* ReadOnlySetAccessor(
+    i::Isolate* isolate, i::JSObject*, i::Object* value, void*) {
+  return value;
+}
+
+
+const i::AccessorDescriptor kCallbackDescriptor = {
+  ZeroAccessorGet,
+  ReadOnlySetAccessor,
+  0
+};
+
+
+THREADED_TEST(RedefineReadOnlyConfigurableForeignCallbackAccessor) {
+  // Verify that property redefinition over foreign callbacks-backed
+  // properties works as expected if the property is non-writable,
+  // but writable. Such a property can be redefined without first
+  // making the property writable (ES5.1 - 8.12.9.10.b)
+  // (bug 3045.)
+  LocalContext env;
+  v8::Isolate* isolate = env->GetIsolate();
+  i::Factory* factory = CcTest::i_isolate()->factory();
+
+  v8::HandleScope scope(isolate);
+
+  i::Handle<i::Map> map =
+      factory->NewMap(i::JS_OBJECT_TYPE, i::JSObject::kHeaderSize);
+  i::Handle<i::DescriptorArray> instance_descriptors(
+      map->instance_descriptors());
+  ASSERT(instance_descriptors->IsEmpty());
+
+  i::Handle<i::DescriptorArray> descriptors = factory->NewDescriptorArray(0, 1);
+  i::DescriptorArray::WhitenessWitness witness(*descriptors);
+  map->set_instance_descriptors(*descriptors);
+
+  i::Handle<i::Foreign> foreign = factory->NewForeign(&kCallbackDescriptor);
+  i::Handle<i::String> name =
+      factory->InternalizeUtf8String(i::Vector<const char>("prop", 4));
+
+  // Want a non-writable and configurable property.
+  PropertyAttributes attribs =
+      static_cast<PropertyAttributes>(DONT_ENUM | READ_ONLY);
+  i::CallbacksDescriptor d(*name, *foreign, attribs);
+  map->AppendDescriptor(&d, witness);
+
+  i::Handle<i::Object> object = factory->NewJSObjectFromMap(map);
+
+  // Put the object on the global object.
+  env->Global()->Set(v8::String::NewFromUtf8(CcTest::isolate(), "Foreign"),
+                     v8::Utils::ToLocal(object));
+
+  // ..and redefine the property through JavaScript, returning its value.
+  const char* script =
+      "Object.defineProperty(Foreign, 'prop', {value: 2}); Foreign.prop";
+  v8::Handle<v8::Value> result = v8::Script::Compile(
+      v8::String::NewFromUtf8(CcTest::isolate(), script))->Run();
+  CHECK_EQ(2, result->Int32Value());
+}

diff --git a/test/mjsunit/harmony/object-observe.js b/test/mjsunit/harmony/object-observe.js
index fb15a1f..6868cbf 100644 (file)
--- a/test/mjsunit/harmony/object-observe.js
+++ b/test/mjsunit/harmony/object-observe.js
@@ -976,16 +976,40 @@ observer.assertNotCalled();
 
 
 // Test all kinds of objects generically.
-function TestObserveConfigurable(obj, prop) {
+function TestObserveConfigurable(obj, prop, is_writable) {
   reset();
+  var valueWhenDeleted = is_writable ? 3 : obj[prop];
   Object.observe(obj, observer.callback);
   Object.unobserve(obj, observer.callback);
   obj[prop] = 1;
   Object.observe(obj, observer.callback);
   obj[prop] = 2;
-  obj[prop] = 3;
+  obj[prop] = valueWhenDeleted;
   delete obj[prop];
-  obj[prop] = 4;
+  // If the deleted obj[prop] exposed another 'prop' along the
+  // prototype chain, only update it if it doesn't have an
+  // (inheritable) accessor or it is a read-only data property. If
+  // either of those is true, then instead create an own property with
+  // the descriptor that [[Put]] mandates for a new property (ES-5.1,
+  // 8.12.5.6)
+  var createOwnProperty = false;
+  for (var o = Object.getPrototypeOf(obj); o; o = Object.getPrototypeOf(o)) {
+    var desc = Object.getOwnPropertyDescriptor(o, prop);
+    if (desc) {
+      if (!desc.writable)
+        createOwnProperty = true;
+      break;
+    }
+  }
+  if (createOwnProperty)
+    Object.defineProperty(obj, prop, {
+      value: 4,
+      writable: true,
+      enumerable: true,
+      configurable: true
+    });
+  else
+    obj[prop] = 4;
   obj[prop] = 4;  // ignored
   obj[prop] = 5;
   Object.defineProperty(obj, prop, {value: 6});
@@ -1015,10 +1039,9 @@ function TestObserveConfigurable(obj, prop) {
   delete obj[prop];
   Object.defineProperty(obj, prop, {value: 11, configurable: true});
   Object.deliverChangeRecords(observer.callback);
-  observer.assertCallbackRecords([
-    { object: obj, name: prop, type: "update", oldValue: 1 },
-    { object: obj, name: prop, type: "update", oldValue: 2 },
-    { object: obj, name: prop, type: "delete", oldValue: 3 },
+
+  var expected = [
+    { object: obj, name: prop, type: "delete", oldValue: valueWhenDeleted },
     { object: obj, name: prop, type: "add" },
     { object: obj, name: prop, type: "update", oldValue: 4 },
     { object: obj, name: prop, type: "update", oldValue: 5 },
@@ -1042,7 +1065,15 @@ function TestObserveConfigurable(obj, prop) {
     { object: obj, name: prop, type: "update", oldValue: 12 },
     { object: obj, name: prop, type: "delete", oldValue: 36 },
     { object: obj, name: prop, type: "add" },
-  ]);
+  ];
+
+  if (is_writable) {
+    expected.unshift(
+      { object: obj, name: prop, type: "update", oldValue: 1 },
+      { object: obj, name: prop, type: "update", oldValue: 2 });
+  }
+
+  observer.assertCallbackRecords(expected);
   Object.unobserve(obj, observer.callback);
   delete obj[prop];
 }
@@ -1144,7 +1175,7 @@ for (var i in objects) for (var j in properties) {
   var desc = Object.getOwnPropertyDescriptor(obj, prop);
   print("***", typeof obj, stringifyNoThrow(obj), prop);
   if (!desc || desc.configurable)
-    TestObserveConfigurable(obj, prop);
+    TestObserveConfigurable(obj, prop, !desc || desc.writable);
   else if (desc.writable)
     TestObserveNonConfigurable(obj, prop, desc);
 }

diff --git a/test/mjsunit/regress/regress-1530.js b/test/mjsunit/regress/regress-1530.js
index db21144..6545900 100644 (file)
--- a/test/mjsunit/regress/regress-1530.js
+++ b/test/mjsunit/regress/regress-1530.js
@@ -62,8 +62,21 @@ assertSame(new f().foo, 'other');
 assertSame(Object.getPrototypeOf(new f()), z);
 assertSame(Object.getOwnPropertyDescriptor(f, 'prototype').value, z);
 
+// Verify that 'name' is (initially) non-writable, but configurable.
+var fname = f.name;
+f.name = z;
+assertSame(fname, f.name);
+Object.defineProperty(f, 'name', {value: 'other'});
+assertSame('other', f.name);
+
+// Verify same for 'length', another configurable and non-writable property.
+assertEquals(0, Object.getOwnPropertyDescriptor(f, 'length').value);
+assertDoesNotThrow(function () { Object.defineProperty(f, 'length', {writable: true}); });
+f.length = 3;
+assertEquals(3, Object.getOwnPropertyDescriptor(f, 'length').value);
+f.length = "untyped";
+assertSame("untyped", Object.getOwnPropertyDescriptor(f, 'length').value);
+
 // 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/mjsunit/regress/regress-270142.js b/test/mjsunit/regress/regress-270142.js
index 6e0865c..63f4d14 100644 (file)
--- a/test/mjsunit/regress/regress-270142.js
+++ b/test/mjsunit/regress/regress-270142.js
@@ -39,7 +39,7 @@ function g(x) {
 
 function checkNameDescriptor(f) {
   var descriptor = Object.getOwnPropertyDescriptor(f, "name");
-  assertFalse(descriptor.configurable);
+  assertTrue(descriptor.configurable);
   assertFalse(descriptor.enumerable);
   assertFalse(descriptor.writable);
 }

diff --git a/test/mjsunit/regress/regress-function-length-strict.js b/test/mjsunit/regress/regress-function-length-strict.js
index 700f34a..6f23df7 100644 (file)
--- a/test/mjsunit/regress/regress-function-length-strict.js
+++ b/test/mjsunit/regress/regress-function-length-strict.js
@@ -37,5 +37,8 @@ var desc = Object.getOwnPropertyDescriptor(foo, 'length');
 assertEquals(3, desc.value);
 assertFalse(desc.writable);
 assertFalse(desc.enumerable);
-assertFalse(desc.configurable);
+assertTrue(desc.configurable);
 assertThrows(function() { foo.length = 2; }, TypeError);
+
+assertDoesNotThrow(function () { Object.defineProperty(foo, 'length', {value: 4}); });
+assertEquals(4, foo.length);