From e8175a3e9f58aa12ccae80576d4c45c21ca761dd Mon Sep 17 00:00:00 2001 From: "rossberg@chromium.org" Date: Fri, 7 Feb 2014 15:29:18 +0000 Subject: [PATCH] Revert "Make Function.length and Function.name configurable properties." Plenty of test failures on test262, Mozilla, Webkit. Will have to investigate. TBR=mstarzinger@chromium.org BUG= Review URL: https://codereview.chromium.org/139983003 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@19203 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, 26 insertions(+), 144 deletions(-) diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc index 62109b5..ef802ba 100644 --- a/src/bootstrapper.cc +++ b/src/bootstrapper.cc @@ -426,32 +426,31 @@ void Genesis::SetFunctionInstanceDescriptor( if (prototypeMode != DONT_ADD_PROTOTYPE) { prototype = factory()->NewForeign(&Accessors::FunctionPrototype); } - PropertyAttributes ro_attribs = static_cast( + PropertyAttributes 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, roc_attribs); + CallbacksDescriptor d(*factory()->length_string(), *length, attribs); map->AppendDescriptor(&d, witness); } { // Add name. - CallbacksDescriptor d(*factory()->name_string(), *name, roc_attribs); + CallbacksDescriptor d(*factory()->name_string(), *name, attribs); map->AppendDescriptor(&d, witness); } { // Add arguments. - CallbacksDescriptor d(*factory()->arguments_string(), *args, ro_attribs); + CallbacksDescriptor d(*factory()->arguments_string(), *args, attribs); map->AppendDescriptor(&d, witness); } { // Add caller. - CallbacksDescriptor d(*factory()->caller_string(), *caller, ro_attribs); + CallbacksDescriptor d(*factory()->caller_string(), *caller, attribs); map->AppendDescriptor(&d, witness); } if (prototypeMode != DONT_ADD_PROTOTYPE) { // Add prototype. - PropertyAttributes attribs = (prototypeMode == ADD_WRITEABLE_PROTOTYPE) - ? static_cast(ro_attribs & ~READ_ONLY) : ro_attribs; + if (prototypeMode == ADD_WRITEABLE_PROTOTYPE) { + attribs = static_cast(attribs & ~READ_ONLY); + } CallbacksDescriptor d(*factory()->prototype_string(), *prototype, attribs); map->AppendDescriptor(&d, witness); } @@ -569,16 +568,14 @@ 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, roc_attribs); + CallbacksDescriptor d(*factory()->length_string(), *length, ro_attribs); map->AppendDescriptor(&d, witness); } { // Add name. - CallbacksDescriptor d(*factory()->name_string(), *name, roc_attribs); + CallbacksDescriptor d(*factory()->name_string(), *name, ro_attribs); map->AppendDescriptor(&d, witness); } { // Add arguments. diff --git a/src/runtime.cc b/src/runtime.cc index d2727db..3596add 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -5068,14 +5068,11 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_DefineOrRedefineDataProperty) { if (callback->IsAccessorInfo()) { return isolate->heap()->undefined_value(); } - // 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. + // 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 && - !(attr & READ_ONLY)) { + if (callback->IsForeign() && lookup.GetAttributes() == attr) { Handle result_object = JSObject::SetPropertyWithCallback(js_object, callback, diff --git a/test/cctest/test-accessors.cc b/test/cctest/test-accessors.cc index 602f5ec..bda09f0 100644 --- a/test/cctest/test-accessors.cc +++ b/test/cctest/test-accessors.cc @@ -624,68 +624,3 @@ 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 6868cbf..fb15a1f 100644 --- a/test/mjsunit/harmony/object-observe.js +++ b/test/mjsunit/harmony/object-observe.js @@ -976,40 +976,16 @@ observer.assertNotCalled(); // Test all kinds of objects generically. -function TestObserveConfigurable(obj, prop, is_writable) { +function TestObserveConfigurable(obj, prop) { 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] = valueWhenDeleted; + obj[prop] = 3; delete obj[prop]; - // 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; obj[prop] = 4; // ignored obj[prop] = 5; Object.defineProperty(obj, prop, {value: 6}); @@ -1039,9 +1015,10 @@ function TestObserveConfigurable(obj, prop, is_writable) { delete obj[prop]; Object.defineProperty(obj, prop, {value: 11, configurable: true}); Object.deliverChangeRecords(observer.callback); - - var expected = [ - { object: obj, name: prop, type: "delete", oldValue: valueWhenDeleted }, + 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 }, { object: obj, name: prop, type: "add" }, { object: obj, name: prop, type: "update", oldValue: 4 }, { object: obj, name: prop, type: "update", oldValue: 5 }, @@ -1065,15 +1042,7 @@ function TestObserveConfigurable(obj, prop, is_writable) { { 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]; } @@ -1175,7 +1144,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, !desc || desc.writable); + TestObserveConfigurable(obj, prop); 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 6545900..db21144 100644 --- a/test/mjsunit/regress/regress-1530.js +++ b/test/mjsunit/regress/regress-1530.js @@ -62,21 +62,8 @@ 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 63f4d14..6e0865c 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"); - assertTrue(descriptor.configurable); + assertFalse(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 6f23df7..700f34a 100644 --- a/test/mjsunit/regress/regress-function-length-strict.js +++ b/test/mjsunit/regress/regress-function-length-strict.js @@ -37,8 +37,5 @@ var desc = Object.getOwnPropertyDescriptor(foo, 'length'); assertEquals(3, desc.value); assertFalse(desc.writable); assertFalse(desc.enumerable); -assertTrue(desc.configurable); +assertFalse(desc.configurable); assertThrows(function() { foo.length = 2; }, TypeError); - -assertDoesNotThrow(function () { Object.defineProperty(foo, 'length', {value: 4}); }); -assertEquals(4, foo.length); -- 2.7.4