From 8c54a373dd8585f8ef07f5178b00190faa0dd06d Mon Sep 17 00:00:00 2001 From: "mvstanton@chromium.org" Date: Wed, 28 May 2014 09:58:27 +0000 Subject: [PATCH] Changing the attributes of a data property implemented with ExecutableAccessorInfo turns the property into a field. Better to keep it as a callback, and correctly deal with the changed property attributes. R=ulan@chromium.org Review URL: https://codereview.chromium.org/262053011 git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@21558 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/accessors.cc | 15 +++++++++++ src/accessors.h | 5 ++++ src/objects-inl.h | 5 ++++ src/objects.cc | 52 +++++++++++++++++++++++++++++++----- src/objects.h | 12 ++++++++- src/runtime.cc | 35 +++++++++--------------- test/cctest/test-api.cc | 21 +++++++++++++++ test/mjsunit/regress/regress-3334.js | 13 +++++++++ 8 files changed, 129 insertions(+), 29 deletions(-) create mode 100644 test/mjsunit/regress/regress-3334.js diff --git a/src/accessors.cc b/src/accessors.cc index 6245613..cd55091 100644 --- a/src/accessors.cc +++ b/src/accessors.cc @@ -51,6 +51,21 @@ Handle Accessors::MakeAccessor( } +Handle Accessors::CloneAccessor( + Isolate* isolate, + Handle accessor) { + Factory* factory = isolate->factory(); + Handle info = factory->NewExecutableAccessorInfo(); + info->set_name(accessor->name()); + info->set_flag(accessor->flag()); + info->set_expected_receiver_type(accessor->expected_receiver_type()); + info->set_getter(accessor->getter()); + info->set_setter(accessor->setter()); + info->set_data(accessor->data()); + return info; +} + + template static C* FindInstanceOf(Isolate* isolate, Object* obj) { for (Object* cur = obj; !cur->IsNull(); cur = cur->GetPrototype(isolate)) { diff --git a/src/accessors.h b/src/accessors.h index d7296f2..ae748c5 100644 --- a/src/accessors.h +++ b/src/accessors.h @@ -86,6 +86,11 @@ class Accessors : public AllStatic { AccessorSetterCallback setter, PropertyAttributes attributes); + static Handle CloneAccessor( + Isolate* isolate, + Handle accessor); + + private: // Helper functions. static Handle FlattenNumber(Isolate* isolate, Handle value); diff --git a/src/objects-inl.h b/src/objects-inl.h index 9c76fae..29fcec6 100644 --- a/src/objects-inl.h +++ b/src/objects-inl.h @@ -6420,6 +6420,11 @@ bool AccessorInfo::IsCompatibleReceiver(Object* receiver) { } +void ExecutableAccessorInfo::clear_setter() { + set_setter(GetIsolate()->heap()->undefined_value(), SKIP_WRITE_BARRIER); +} + + void AccessorPair::set_access_flags(v8::AccessControl access_control) { int current = access_flags()->value(); current = BooleanBit::set(current, diff --git a/src/objects.cc b/src/objects.cc index 3d53f95..19dcf1b 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -4335,9 +4335,6 @@ MaybeHandle JSObject::SetPropertyForResult( // callback setter removed. The two lines looking up the LookupResult // result are also added. If one of the functions is changed, the other // should be. -// Note that this method cannot be used to set the prototype of a function -// because ConvertDescriptorToField() which is called in "case CALLBACKS:" -// doesn't handle function prototypes correctly. MaybeHandle JSObject::SetOwnPropertyIgnoreAttributes( Handle object, Handle name, @@ -4346,7 +4343,8 @@ MaybeHandle JSObject::SetOwnPropertyIgnoreAttributes( ValueType value_type, StoreMode mode, ExtensibilityCheck extensibility_check, - StoreFromKeyed store_from_keyed) { + StoreFromKeyed store_from_keyed, + ExecutableAccessorInfoHandling handling) { Isolate* isolate = object->GetIsolate(); // Make sure that the top context does not change when doing callbacks or @@ -4401,6 +4399,8 @@ MaybeHandle JSObject::SetOwnPropertyIgnoreAttributes( old_attributes = lookup.GetAttributes(); } + bool executed_set_prototype = false; + // Check of IsReadOnly removed from here in clone. if (lookup.IsTransition()) { Handle result; @@ -4425,8 +4425,48 @@ MaybeHandle JSObject::SetOwnPropertyIgnoreAttributes( } break; case CALLBACKS: - ConvertAndSetOwnProperty(&lookup, name, value, attributes); + { + Handle callback(lookup.GetCallbackObject(), isolate); + if (callback->IsExecutableAccessorInfo() && + handling == DONT_FORCE_FIELD) { + Handle result; + ASSIGN_RETURN_ON_EXCEPTION( + isolate, result, + JSObject::SetPropertyWithCallback(object, + name, + value, + handle(lookup.holder()), + callback, + STRICT), + Object); + + if (attributes != lookup.GetAttributes()) { + Handle new_data = + Accessors::CloneAccessor( + isolate, Handle::cast(callback)); + new_data->set_property_attributes(attributes); + if (attributes & READ_ONLY) { + // This way we don't have to introduce a lookup to the setter, + // simply make it unavailable to reflect the attributes. + new_data->clear_setter(); + } + + SetPropertyCallback(object, name, new_data, attributes); + } + if (is_observed) { + // If we are setting the prototype of a function and are observed, + // don't send change records because the prototype handles that + // itself. + executed_set_prototype = object->IsJSFunction() && + String::Equals(isolate->factory()->prototype_string(), + Handle::cast(name)) && + Handle::cast(object)->should_have_prototype(); + } + } else { + ConvertAndSetOwnProperty(&lookup, name, value, attributes); + } break; + } case NONEXISTENT: case HANDLER: case INTERCEPTOR: @@ -4434,7 +4474,7 @@ MaybeHandle JSObject::SetOwnPropertyIgnoreAttributes( } } - if (is_observed) { + if (is_observed && !executed_set_prototype) { if (lookup.IsTransition()) { EnqueueChangeRecord(object, "add", name, old_value); } else if (old_value->IsTheHole()) { diff --git a/src/objects.h b/src/objects.h index cde3bf7..64664a5 100644 --- a/src/objects.h +++ b/src/objects.h @@ -2154,6 +2154,13 @@ class JSObject: public JSReceiver { StrictMode strict_mode, StoreFromKeyed store_mode = MAY_BE_STORE_FROM_KEYED); + // SetLocalPropertyIgnoreAttributes converts callbacks to fields. We need to + // grant an exemption to ExecutableAccessor callbacks in some cases. + enum ExecutableAccessorInfoHandling { + DEFAULT_HANDLING, + DONT_FORCE_FIELD + }; + MUST_USE_RESULT static MaybeHandle SetOwnPropertyIgnoreAttributes( Handle object, Handle key, @@ -2162,7 +2169,8 @@ class JSObject: public JSReceiver { ValueType value_type = OPTIMAL_REPRESENTATION, StoreMode mode = ALLOW_AS_CONSTANT, ExtensibilityCheck extensibility_check = PERFORM_EXTENSIBILITY_CHECK, - StoreFromKeyed store_mode = MAY_BE_STORE_FROM_KEYED); + StoreFromKeyed store_mode = MAY_BE_STORE_FROM_KEYED, + ExecutableAccessorInfoHandling handling = DEFAULT_HANDLING); static inline Handle ExpectedTransitionKey(Handle map); static inline Handle ExpectedTransitionTarget(Handle map); @@ -10536,6 +10544,8 @@ class ExecutableAccessorInfo: public AccessorInfo { static const int kDataOffset = kSetterOffset + kPointerSize; static const int kSize = kDataOffset + kPointerSize; + inline void clear_setter(); + private: DISALLOW_IMPLICIT_CONSTRUCTORS(ExecutableAccessorInfo); }; diff --git a/src/runtime.cc b/src/runtime.cc index 68a730b..c5b60e9 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -5175,26 +5175,6 @@ RUNTIME_FUNCTION(Runtime_DefineOrRedefineDataProperty) { LookupResult lookup(isolate); js_object->LookupOwnRealNamedProperty(name, &lookup); - // Special case for callback properties. - if (lookup.IsPropertyCallbacks()) { - Handle callback(lookup.GetCallbackObject(), isolate); - // Avoid redefining 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. - ASSERT(!callback->IsForeign()); - if (callback->IsAccessorInfo() && - lookup.GetAttributes() == attr) { - Handle result_object; - ASSIGN_RETURN_FAILURE_ON_EXCEPTION( - isolate, result_object, - JSObject::SetPropertyWithCallback(js_object, name, obj_value, - handle(lookup.holder()), - callback, STRICT)); - return *result_object; - } - } - // Take special care when attributes are different and there is already // a property. For simplicity we normalize the property which enables us // to not worry about changing the instance_descriptor and creating a new @@ -5209,14 +5189,25 @@ RUNTIME_FUNCTION(Runtime_DefineOrRedefineDataProperty) { // we don't have to check for null. js_object = Handle(JSObject::cast(js_object->GetPrototype())); } - JSObject::NormalizeProperties(js_object, CLEAR_INOBJECT_PROPERTIES, 0); + + if (attr != lookup.GetAttributes() || + (lookup.IsPropertyCallbacks() && + !lookup.GetCallbackObject()->IsAccessorInfo())) { + JSObject::NormalizeProperties(js_object, CLEAR_INOBJECT_PROPERTIES, 0); + } + // Use IgnoreAttributes version since a readonly property may be // overridden and SetProperty does not allow this. Handle result; ASSIGN_RETURN_FAILURE_ON_EXCEPTION( isolate, result, JSObject::SetOwnPropertyIgnoreAttributes( - js_object, name, obj_value, attr)); + js_object, name, obj_value, attr, + Object::OPTIMAL_REPRESENTATION, + ALLOW_AS_CONSTANT, + JSReceiver::PERFORM_EXTENSIBILITY_CHECK, + JSReceiver::MAY_BE_STORE_FROM_KEYED, + JSObject::DONT_FORCE_FIELD)); return *result; } diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index 6b55d7a..df5938d 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -2004,6 +2004,27 @@ THREADED_TEST(EmptyInterceptorDoesNotShadowAccessors) { } +THREADED_TEST(ExecutableAccessorIsPreservedOnAttributeChange) { + v8::Isolate* isolate = CcTest::isolate(); + v8::HandleScope scope(isolate); + LocalContext env; + v8::Local res = CompileRun("var a = []; a;"); + i::Handle a(v8::Utils::OpenHandle(v8::Object::Cast(*res))); + CHECK(a->map()->instance_descriptors()->IsFixedArray()); + CHECK_GT(i::FixedArray::cast(a->map()->instance_descriptors())->length(), 0); + CompileRun("Object.defineProperty(a, 'length', { writable: false });"); + CHECK_EQ(i::FixedArray::cast(a->map()->instance_descriptors())->length(), 0); + // But we should still have an ExecutableAccessorInfo. + i::Isolate* i_isolate = reinterpret_cast(isolate); + i::LookupResult lookup(i_isolate); + i::Handle name(v8::Utils::OpenHandle(*v8_str("length"))); + a->LookupOwnRealNamedProperty(name, &lookup); + CHECK(lookup.IsPropertyCallbacks()); + i::Handle callback(lookup.GetCallbackObject(), i_isolate); + CHECK(callback->IsExecutableAccessorInfo()); +} + + THREADED_TEST(EmptyInterceptorBreakTransitions) { v8::HandleScope scope(CcTest::isolate()); Handle templ = FunctionTemplate::New(CcTest::isolate()); diff --git a/test/mjsunit/regress/regress-3334.js b/test/mjsunit/regress/regress-3334.js new file mode 100644 index 0000000..301155d --- /dev/null +++ b/test/mjsunit/regress/regress-3334.js @@ -0,0 +1,13 @@ +// Copyright 2014 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +function foo(){} +Object.defineProperty(foo, "prototype", { value: 2 }); +assertEquals(2, foo.prototype); + +function bar(){} +Object.defineProperty(bar, "prototype", { value: 2, writable: false }); +assertEquals(2, bar.prototype); +assertThrows(function() { "use strict"; bar.prototype = 10; }, TypeError); +assertEquals(false, Object.getOwnPropertyDescriptor(bar,"prototype").writable); -- 2.7.4