Revision: 19200
Author: [email protected]
Date: Fri Feb 7 14:55:30 2014 UTC
Log: 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
[email protected], rossberg
BUG=v8:3045
Review URL: https://codereview.chromium.org/116083006
http://code.google.com/p/v8/source/detail?r=19200
Modified:
/branches/bleeding_edge/src/bootstrapper.cc
/branches/bleeding_edge/src/runtime.cc
/branches/bleeding_edge/test/cctest/test-accessors.cc
/branches/bleeding_edge/test/mjsunit/harmony/object-observe.js
/branches/bleeding_edge/test/mjsunit/regress/regress-1530.js
/branches/bleeding_edge/test/mjsunit/regress/regress-270142.js
/branches/bleeding_edge/test/mjsunit/regress/regress-function-length-strict.js
=======================================
--- /branches/bleeding_edge/src/bootstrapper.cc Wed Jan 29 14:18:55 2014 UTC
+++ /branches/bleeding_edge/src/bootstrapper.cc Fri Feb 7 14:55:30 2014 UTC
@@ -426,31 +426,32 @@
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 @@
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.
=======================================
--- /branches/bleeding_edge/src/runtime.cc Fri Feb 7 14:13:00 2014 UTC
+++ /branches/bleeding_edge/src/runtime.cc Fri Feb 7 14:55:30 2014 UTC
@@ -5068,11 +5068,14 @@
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,
=======================================
--- /branches/bleeding_edge/test/cctest/test-accessors.cc Thu Feb 6
10:50:07 2014 UTC
+++ /branches/bleeding_edge/test/cctest/test-accessors.cc Fri Feb 7
14:55:30 2014 UTC
@@ -624,3 +624,68 @@
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());
+}
=======================================
--- /branches/bleeding_edge/test/mjsunit/harmony/object-observe.js Tue Jan
14 12:04:10 2014 UTC
+++ /branches/bleeding_edge/test/mjsunit/harmony/object-observe.js Fri Feb
7 14:55:30 2014 UTC
@@ -976,16 +976,40 @@
// 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 @@
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 @@
{ 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 @@
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);
}
=======================================
--- /branches/bleeding_edge/test/mjsunit/regress/regress-1530.js Tue Dec 20
08:49:51 2011 UTC
+++ /branches/bleeding_edge/test/mjsunit/regress/regress-1530.js Fri Feb 7
14:55:30 2014 UTC
@@ -62,8 +62,21 @@
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: {} })");
=======================================
--- /branches/bleeding_edge/test/mjsunit/regress/regress-270142.js Fri Dec
20 12:06:11 2013 UTC
+++ /branches/bleeding_edge/test/mjsunit/regress/regress-270142.js Fri Feb
7 14:55:30 2014 UTC
@@ -39,7 +39,7 @@
function checkNameDescriptor(f) {
var descriptor = Object.getOwnPropertyDescriptor(f, "name");
- assertFalse(descriptor.configurable);
+ assertTrue(descriptor.configurable);
assertFalse(descriptor.enumerable);
assertFalse(descriptor.writable);
}
=======================================
---
/branches/bleeding_edge/test/mjsunit/regress/regress-function-length-strict.js
Tue Jun 18 07:51:50 2013 UTC
+++
/branches/bleeding_edge/test/mjsunit/regress/regress-function-length-strict.js
Fri Feb 7 14:55:30 2014 UTC
@@ -37,5 +37,8 @@
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);
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
