From d06afb3ce0ece07eb29d44a110d849cb8deada88 Mon Sep 17 00:00:00 2001 From: "verwaest@chromium.org" Date: Mon, 23 Jun 2014 09:02:16 +0000 Subject: [PATCH] Remove AccessControl from AccessorPairs, as it's an invalid usecase of AllCan* BUG= R=dcarney@chromium.org Review URL: https://codereview.chromium.org/332863003 git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@21918 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/api.cc | 10 ++-- src/apinatives.js | 8 +-- src/factory.cc | 1 - src/objects-debug.cc | 1 - src/objects-inl.h | 23 --------- src/objects-printer.cc | 2 - src/objects.cc | 31 ++++-------- src/objects.h | 22 ++------- src/runtime.cc | 18 ++----- src/runtime.h | 2 +- test/cctest/test-api.cc | 49 ++----------------- .../runtime-gen/setaccessorproperty.js | 3 +- tools/generate-runtime-tests.py | 3 +- 13 files changed, 36 insertions(+), 137 deletions(-) diff --git a/src/api.cc b/src/api.cc index a6ea44eddc..9397c40116 100644 --- a/src/api.cc +++ b/src/api.cc @@ -833,6 +833,8 @@ void Template::SetAccessorProperty( v8::Local setter, v8::PropertyAttribute attribute, v8::AccessControl access_control) { + // TODO(verwaest): Remove |access_control|. + ASSERT_EQ(v8::DEFAULT, access_control); i::Isolate* isolate = Utils::OpenHandle(this)->GetIsolate(); ENTER_V8(isolate); ASSERT(!name.IsEmpty()); @@ -844,8 +846,7 @@ void Template::SetAccessorProperty( name, getter, setter, - v8::Integer::New(v8_isolate, attribute), - v8::Integer::New(v8_isolate, access_control)}; + v8::Integer::New(v8_isolate, attribute)}; TemplateSet(isolate, this, kSize, data); } @@ -3446,6 +3447,8 @@ void Object::SetAccessorProperty(Local name, Handle setter, PropertyAttribute attribute, AccessControl settings) { + // TODO(verwaest): Remove |settings|. + ASSERT_EQ(v8::DEFAULT, settings); i::Isolate* isolate = Utils::OpenHandle(this)->GetIsolate(); ON_BAILOUT(isolate, "v8::Object::SetAccessorProperty()", return); ENTER_V8(isolate); @@ -3457,8 +3460,7 @@ void Object::SetAccessorProperty(Local name, v8::Utils::OpenHandle(*name), getter_i, setter_i, - static_cast(attribute), - settings); + static_cast(attribute)); } diff --git a/src/apinatives.js b/src/apinatives.js index d4835affe9..9bb52e2b7a 100644 --- a/src/apinatives.js +++ b/src/apinatives.js @@ -94,14 +94,14 @@ function ConfigureTemplateInstance(obj, data) { var attributes = properties[i + 3]; var value = Instantiate(prop_data, name); %SetProperty(obj, name, value, attributes); - } else if (length == 5) { + } else if (length == 4 || length == 5) { + // TODO(verwaest): The 5th value used to be access_control. Remove once + // the bindings are updated. var name = properties[i + 1]; var getter = properties[i + 2]; var setter = properties[i + 3]; var attribute = properties[i + 4]; - var access_control = properties[i + 5]; - %SetAccessorProperty( - obj, name, getter, setter, attribute, access_control); + %SetAccessorProperty(obj, name, getter, setter, attribute); } else { throw "Bad properties array"; } diff --git a/src/factory.cc b/src/factory.cc index 3d373fbb5b..1996e736f0 100644 --- a/src/factory.cc +++ b/src/factory.cc @@ -151,7 +151,6 @@ Handle Factory::NewAccessorPair() { Handle::cast(NewStruct(ACCESSOR_PAIR_TYPE)); accessors->set_getter(*the_hole_value(), SKIP_WRITE_BARRIER); accessors->set_setter(*the_hole_value(), SKIP_WRITE_BARRIER); - accessors->set_access_flags(Smi::FromInt(0), SKIP_WRITE_BARRIER); return accessors; } diff --git a/src/objects-debug.cc b/src/objects-debug.cc index 5ff74d2273..344fe711bd 100644 --- a/src/objects-debug.cc +++ b/src/objects-debug.cc @@ -878,7 +878,6 @@ void AccessorPair::AccessorPairVerify() { CHECK(IsAccessorPair()); VerifyPointer(getter()); VerifyPointer(setter()); - VerifySmiField(kAccessFlagsOffset); } diff --git a/src/objects-inl.h b/src/objects-inl.h index 0a26298453..b0da6f8a2a 100644 --- a/src/objects-inl.h +++ b/src/objects-inl.h @@ -5181,7 +5181,6 @@ ACCESSORS(Box, value, Object, kValueOffset) ACCESSORS(AccessorPair, getter, Object, kGetterOffset) ACCESSORS(AccessorPair, setter, Object, kSetterOffset) -ACCESSORS_TO_SMI(AccessorPair, access_flags, kAccessFlagsOffset) ACCESSORS(AccessCheckInfo, named_callback, Object, kNamedCallbackOffset) ACCESSORS(AccessCheckInfo, indexed_callback, Object, kIndexedCallbackOffset) @@ -6587,28 +6586,6 @@ void ExecutableAccessorInfo::clear_setter() { } -void AccessorPair::set_access_flags(v8::AccessControl access_control) { - int current = access_flags()->value(); - current = BooleanBit::set(current, - kAllCanReadBit, - access_control & ALL_CAN_READ); - current = BooleanBit::set(current, - kAllCanWriteBit, - access_control & ALL_CAN_WRITE); - set_access_flags(Smi::FromInt(current)); -} - - -bool AccessorPair::all_can_read() { - return BooleanBit::get(access_flags(), kAllCanReadBit); -} - - -bool AccessorPair::all_can_write() { - return BooleanBit::get(access_flags(), kAllCanWriteBit); -} - - template void Dictionary::SetEntry(int entry, Handle key, diff --git a/src/objects-printer.cc b/src/objects-printer.cc index f55374347a..e86059b719 100644 --- a/src/objects-printer.cc +++ b/src/objects-printer.cc @@ -1010,8 +1010,6 @@ void AccessorPair::AccessorPairPrint(FILE* out) { getter()->ShortPrint(out); PrintF(out, "\n - setter: "); setter()->ShortPrint(out); - PrintF(out, "\n - flag: "); - access_flags()->ShortPrint(out); } diff --git a/src/objects.cc b/src/objects.cc index 0e5d9a26a2..f7f261458a 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -575,8 +575,6 @@ static bool FindAllCanReadHolder(LookupIterator* it) { Handle accessors = it->GetAccessors(); if (accessors->IsAccessorInfo()) { if (AccessorInfo::cast(*accessors)->all_can_read()) return true; - } else if (accessors->IsAccessorPair()) { - if (AccessorPair::cast(*accessors)->all_can_read()) return true; } } } @@ -619,8 +617,6 @@ static bool FindAllCanWriteHolder(LookupResult* result, Object* callback_obj = result->GetCallbackObject(); if (callback_obj->IsAccessorInfo()) { if (AccessorInfo::cast(callback_obj)->all_can_write()) return true; - } else if (callback_obj->IsAccessorPair()) { - if (AccessorPair::cast(callback_obj)->all_can_write()) return true; } } if (!check_prototype) break; @@ -6440,8 +6436,7 @@ void JSObject::DefineElementAccessor(Handle object, uint32_t index, Handle getter, Handle setter, - PropertyAttributes attributes, - v8::AccessControl access_control) { + PropertyAttributes attributes) { switch (object->GetElementsKind()) { case FAST_SMI_ELEMENTS: case FAST_ELEMENTS: @@ -6498,7 +6493,6 @@ void JSObject::DefineElementAccessor(Handle object, Isolate* isolate = object->GetIsolate(); Handle accessors = isolate->factory()->NewAccessorPair(); accessors->SetComponents(*getter, *setter); - accessors->set_access_flags(access_control); SetElementCallback(object, index, accessors, attributes); } @@ -6529,13 +6523,11 @@ void JSObject::DefinePropertyAccessor(Handle object, Handle name, Handle getter, Handle setter, - PropertyAttributes attributes, - v8::AccessControl access_control) { + PropertyAttributes attributes) { // We could assert that the property is configurable here, but we would need // to do a lookup, which seems to be a bit of overkill. bool only_attribute_changes = getter->IsNull() && setter->IsNull(); if (object->HasFastProperties() && !only_attribute_changes && - access_control == v8::DEFAULT && (object->map()->NumberOfOwnDescriptors() <= kMaxNumberOfDescriptors)) { bool getterOk = getter->IsNull() || DefineFastAccessor(object, name, ACCESSOR_GETTER, getter, attributes); @@ -6546,7 +6538,6 @@ void JSObject::DefinePropertyAccessor(Handle object, Handle accessors = CreateAccessorPairFor(object, name); accessors->SetComponents(*getter, *setter); - accessors->set_access_flags(access_control); SetPropertyCallback(object, name, accessors, attributes); } @@ -6647,8 +6638,7 @@ void JSObject::DefineAccessor(Handle object, Handle name, Handle getter, Handle setter, - PropertyAttributes attributes, - v8::AccessControl access_control) { + PropertyAttributes attributes) { Isolate* isolate = object->GetIsolate(); // Check access rights if needed. if (object->IsAccessCheckNeeded() && @@ -6666,8 +6656,7 @@ void JSObject::DefineAccessor(Handle object, name, getter, setter, - attributes, - access_control); + attributes); return; } @@ -6704,11 +6693,9 @@ void JSObject::DefineAccessor(Handle object, } if (is_element) { - DefineElementAccessor( - object, index, getter, setter, attributes, access_control); + DefineElementAccessor(object, index, getter, setter, attributes); } else { - DefinePropertyAccessor( - object, name, getter, setter, attributes, access_control); + DefinePropertyAccessor(object, name, getter, setter, attributes); } if (is_observed) { @@ -6784,8 +6771,10 @@ bool JSObject::DefineFastAccessor(Handle object, ASSERT(target->NumberOfOwnDescriptors() == object->map()->NumberOfOwnDescriptors()); // This works since descriptors are sorted in order of addition. - ASSERT(object->map()->instance_descriptors()-> - GetKey(descriptor_number) == *name); + ASSERT(Name::Equals( + handle(object->map()->instance_descriptors()->GetKey( + descriptor_number)), + name)); return TryAccessorTransition(object, target, descriptor_number, component, accessor, attributes); } diff --git a/src/objects.h b/src/objects.h index d990dd3335..8fd12947fd 100644 --- a/src/objects.h +++ b/src/objects.h @@ -2228,8 +2228,7 @@ class JSObject: public JSReceiver { Handle name, Handle getter, Handle setter, - PropertyAttributes attributes, - v8::AccessControl access_control = v8::DEFAULT); + PropertyAttributes attributes); // Defines an AccessorInfo property on the given object. MUST_USE_RESULT static MaybeHandle SetAccessor( @@ -2823,16 +2822,14 @@ class JSObject: public JSReceiver { uint32_t index, Handle getter, Handle setter, - PropertyAttributes attributes, - v8::AccessControl access_control); + PropertyAttributes attributes); static Handle CreateAccessorPairFor(Handle object, Handle name); static void DefinePropertyAccessor(Handle object, Handle name, Handle getter, Handle setter, - PropertyAttributes attributes, - v8::AccessControl access_control); + PropertyAttributes attributes); // Try to define a single accessor paying attention to map transitions. // Returns false if this was not possible and we have to use the slow case. @@ -10647,17 +10644,10 @@ class ExecutableAccessorInfo: public AccessorInfo { // * undefined: considered an accessor by the spec, too, strangely enough // * the hole: an accessor which has not been set // * a pointer to a map: a transition used to ensure map sharing -// access_flags provides the ability to override access checks on access check -// failure. class AccessorPair: public Struct { public: DECL_ACCESSORS(getter, Object) DECL_ACCESSORS(setter, Object) - DECL_ACCESSORS(access_flags, Smi) - - inline void set_access_flags(v8::AccessControl access_control); - inline bool all_can_read(); - inline bool all_can_write(); DECLARE_CAST(AccessorPair) @@ -10694,13 +10684,9 @@ class AccessorPair: public Struct { static const int kGetterOffset = HeapObject::kHeaderSize; static const int kSetterOffset = kGetterOffset + kPointerSize; - static const int kAccessFlagsOffset = kSetterOffset + kPointerSize; - static const int kSize = kAccessFlagsOffset + kPointerSize; + static const int kSize = kSetterOffset + kPointerSize; private: - static const int kAllCanReadBit = 0; - static const int kAllCanWriteBit = 1; - // Strangely enough, in addition to functions and harmony proxies, the spec // requires us to consider undefined as a kind of accessor, too: // var obj = {}; diff --git a/src/runtime.cc b/src/runtime.cc index c3a564a247..c0b3c4097b 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -1899,14 +1899,6 @@ static bool CheckAccessException(Object* callback, (access_type == v8::ACCESS_GET && info->all_can_read()) || (access_type == v8::ACCESS_SET && info->all_can_write()); } - if (callback->IsAccessorPair()) { - AccessorPair* info = AccessorPair::cast(callback); - return - (access_type == v8::ACCESS_HAS && - (info->all_can_read() || info->all_can_write())) || - (access_type == v8::ACCESS_GET && info->all_can_read()) || - (access_type == v8::ACCESS_SET && info->all_can_write()); - } return false; } @@ -1959,8 +1951,8 @@ static void CheckPropertyAccess(Handle obj, } // Access check callback denied the access, but some properties - // can have a special permissions which override callbacks descision - // (currently see v8::AccessControl). + // can have a special permissions which override callbacks decision + // (see v8::AccessControl). // API callbacks can have per callback access exceptions. if (lookup.IsFound() && lookup.type() == INTERCEPTOR) { lookup.holder()->LookupOwnRealNamedProperty(name, &lookup); @@ -2175,13 +2167,12 @@ static Handle InstantiateAccessorComponent(Isolate* isolate, RUNTIME_FUNCTION(Runtime_SetAccessorProperty) { HandleScope scope(isolate); - ASSERT(args.length() == 6); + ASSERT(args.length() == 5); CONVERT_ARG_HANDLE_CHECKED(JSObject, object, 0); CONVERT_ARG_HANDLE_CHECKED(Name, name, 1); CONVERT_ARG_HANDLE_CHECKED(Object, getter, 2); CONVERT_ARG_HANDLE_CHECKED(Object, setter, 3); CONVERT_SMI_ARG_CHECKED(attribute, 4); - CONVERT_SMI_ARG_CHECKED(access_control, 5); RUNTIME_ASSERT(getter->IsUndefined() || getter->IsFunctionTemplateInfo()); RUNTIME_ASSERT(setter->IsUndefined() || setter->IsFunctionTemplateInfo()); RUNTIME_ASSERT(PropertyDetails::AttributesField::is_valid( @@ -2190,8 +2181,7 @@ RUNTIME_FUNCTION(Runtime_SetAccessorProperty) { name, InstantiateAccessorComponent(isolate, getter), InstantiateAccessorComponent(isolate, setter), - static_cast(attribute), - static_cast(access_control)); + static_cast(attribute)); return isolate->heap()->undefined_value(); } diff --git a/src/runtime.h b/src/runtime.h index 6fad657816..494033bcec 100644 --- a/src/runtime.h +++ b/src/runtime.h @@ -206,7 +206,7 @@ namespace internal { F(GetTemplateField, 2, 1) \ F(DisableAccessChecks, 1, 1) \ F(EnableAccessChecks, 1, 1) \ - F(SetAccessorProperty, 6, 1) \ + F(SetAccessorProperty, 5, 1) \ \ /* Dates */ \ F(DateCurrentTime, 0, 1) \ diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index e01b6bab8e..56979c63e1 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -9020,19 +9020,13 @@ static bool IndexedAccessBlocker(Local global, } -static int g_echo_value_1 = -1; -static int g_echo_value_2 = -1; +static int g_echo_value = -1; static void EchoGetter( Local name, const v8::PropertyCallbackInfo& info) { - info.GetReturnValue().Set(v8_num(g_echo_value_1)); -} - - -static void EchoGetter(const v8::FunctionCallbackInfo& info) { - info.GetReturnValue().Set(v8_num(g_echo_value_2)); + info.GetReturnValue().Set(v8_num(g_echo_value)); } @@ -9040,14 +9034,7 @@ static void EchoSetter(Local name, Local value, const v8::PropertyCallbackInfo&) { if (value->IsNumber()) - g_echo_value_1 = value->Int32Value(); -} - - -static void EchoSetter(const v8::FunctionCallbackInfo& info) { - v8::Handle value = info[0]; - if (value->IsNumber()) - g_echo_value_2 = value->Int32Value(); + g_echo_value = value->Int32Value(); } @@ -9088,13 +9075,6 @@ TEST(AccessControl) { v8::AccessControl(v8::ALL_CAN_READ | v8::ALL_CAN_WRITE)); - global_template->SetAccessorProperty( - v8_str("accessible_js_prop"), - v8::FunctionTemplate::New(isolate, EchoGetter), - v8::FunctionTemplate::New(isolate, EchoSetter), - v8::None, - v8::AccessControl(v8::ALL_CAN_READ | v8::ALL_CAN_WRITE)); - // Add an accessor that is not accessible by cross-domain JS code. global_template->SetAccessor(v8_str("blocked_prop"), UnreachableGetter, UnreachableSetter, @@ -9223,45 +9203,26 @@ TEST(AccessControl) { value = CompileRun("other.accessible_prop = 3"); CHECK(value->IsNumber()); CHECK_EQ(3, value->Int32Value()); - CHECK_EQ(3, g_echo_value_1); - - // Access accessible js property - value = CompileRun("other.accessible_js_prop = 3"); - CHECK(value->IsNumber()); - CHECK_EQ(3, value->Int32Value()); - CHECK_EQ(3, g_echo_value_2); + CHECK_EQ(3, g_echo_value); value = CompileRun("other.accessible_prop"); CHECK(value->IsNumber()); CHECK_EQ(3, value->Int32Value()); - value = CompileRun("other.accessible_js_prop"); - CHECK(value->IsNumber()); - CHECK_EQ(3, value->Int32Value()); - value = CompileRun( "Object.getOwnPropertyDescriptor(other, 'accessible_prop').value"); CHECK(value->IsNumber()); CHECK_EQ(3, value->Int32Value()); - value = CompileRun( - "Object.getOwnPropertyDescriptor(other, 'accessible_js_prop').get()"); - CHECK(value->IsNumber()); - CHECK_EQ(3, value->Int32Value()); - value = CompileRun("propertyIsEnumerable.call(other, 'accessible_prop')"); CHECK(value->IsTrue()); - value = CompileRun("propertyIsEnumerable.call(other, 'accessible_js_prop')"); - CHECK(value->IsTrue()); - // Enumeration doesn't enumerate accessors from inaccessible objects in // the prototype chain even if the accessors are in themselves accessible. value = CompileRun("(function(){var obj = {'__proto__':other};" "for (var p in obj)" " if (p == 'accessible_prop' ||" - " p == 'accessible_js_prop' ||" " p == 'blocked_js_prop' ||" " p == 'blocked_js_prop') {" " return false;" @@ -9336,7 +9297,7 @@ TEST(AccessControlES5) { // Make sure that we can set the accessible accessors value using normal // assignment. CompileRun("other.accessible_prop = 42"); - CHECK_EQ(42, g_echo_value_1); + CHECK_EQ(42, g_echo_value); v8::Handle value; CompileRun("Object.defineProperty(other, 'accessible_prop', {value: -1})"); diff --git a/test/mjsunit/runtime-gen/setaccessorproperty.js b/test/mjsunit/runtime-gen/setaccessorproperty.js index 1f805f7263..bcf2a348e5 100644 --- a/test/mjsunit/runtime-gen/setaccessorproperty.js +++ b/test/mjsunit/runtime-gen/setaccessorproperty.js @@ -6,5 +6,4 @@ var _name = "name"; var arg2 = undefined; var arg3 = undefined; var _attribute = 1; -var _access_control = 1; -%SetAccessorProperty(_object, _name, arg2, arg3, _attribute, _access_control); +%SetAccessorProperty(_object, _name, arg2, arg3, _attribute); diff --git a/tools/generate-runtime-tests.py b/tools/generate-runtime-tests.py index 04c90431c9..a604cd3555 100755 --- a/tools/generate-runtime-tests.py +++ b/tools/generate-runtime-tests.py @@ -158,8 +158,7 @@ CUSTOM_KNOWN_GOOD_INPUT = { "NumberToRadixString": [None, "2", None], "ParseJson": ["\"{}\"", 1], "RegExpExecMultiple": [None, None, "['a']", "['a']", None], - "SetAccessorProperty": [None, None, "undefined", "undefined", None, None, - None], + "SetAccessorProperty": [None, None, "undefined", "undefined", None, None], "SetIteratorInitialize": [None, None, "2", None], "SetDebugEventListener": ["undefined", None, None], "SetFunctionBreakPoint": [None, 200, None, None],