From 7317b71f02597143cf3155e5357673024598e173 Mon Sep 17 00:00:00 2001 From: "rossberg@chromium.org" Date: Fri, 7 Feb 2014 14:55:30 +0000 Subject: [PATCH] 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 --- src/bootstrapper.cc | 23 ++++---- src/runtime.cc | 9 ++- test/cctest/test-accessors.cc | 65 ++++++++++++++++++++++ test/mjsunit/harmony/object-observe.js | 49 +++++++++++++--- test/mjsunit/regress/regress-1530.js | 17 +++++- test/mjsunit/regress/regress-270142.js | 2 +- .../regress/regress-function-length-strict.js | 5 +- 7 files changed, 144 insertions(+), 26 deletions(-) diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc index ef802ba..62109b5 100644 --- 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 ro_attribs = static_cast( DONT_ENUM | DONT_DELETE | READ_ONLY); + PropertyAttributes roc_attribs = static_cast( + 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(attribs & ~READ_ONLY); - } + PropertyAttributes attribs = (prototypeMode == ADD_WRITEABLE_PROTOTYPE) + ? static_cast(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(DONT_ENUM | DONT_DELETE); PropertyAttributes ro_attribs = static_cast(DONT_ENUM | DONT_DELETE | READ_ONLY); + PropertyAttributes roc_attribs = + static_cast(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 --- 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 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 --- 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 map = + factory->NewMap(i::JS_OBJECT_TYPE, i::JSObject::kHeaderSize); + i::Handle instance_descriptors( + map->instance_descriptors()); + ASSERT(instance_descriptors->IsEmpty()); + + i::Handle descriptors = factory->NewDescriptorArray(0, 1); + i::DescriptorArray::WhitenessWitness witness(*descriptors); + map->set_instance_descriptors(*descriptors); + + i::Handle foreign = factory->NewForeign(&kCallbackDescriptor); + i::Handle name = + factory->InternalizeUtf8String(i::Vector("prop", 4)); + + // Want a non-writable and configurable property. + PropertyAttributes attribs = + static_cast(DONT_ENUM | READ_ONLY); + i::CallbacksDescriptor d(*name, *foreign, attribs); + map->AppendDescriptor(&d, witness); + + i::Handle 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 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 --- 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 --- 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 --- 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 --- 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); -- 2.7.4