From 1c2aa60520653a7a94ff0e5709438040783011d7 Mon Sep 17 00:00:00 2001 From: Yang Guo Date: Tue, 4 Sep 2018 10:53:13 +0200 Subject: [PATCH] [debug-evaluate] extend accessors by runtime receiver checks Also extend the API to reflect this new feature. R=jgruber@chromium.org, szuend@google.com, ulan@chromium.org Bug: v8:8125 Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: Ic7a7604a8c663ba04b324eb8902ff325a25654e7 Reviewed-on: https://chromium-review.googlesource.com/1202087 Reviewed-by: Ulan Degenbaev Reviewed-by: Jakob Gruber Commit-Queue: Yang Guo Cr-Commit-Position: refs/heads/master@{#55604} --- include/v8.h | 35 +++++--- src/accessors.cc | 3 +- src/accessors.h | 50 +++++------ src/api-arguments-inl.h | 85 ++++++++++++------- src/api-arguments.h | 3 +- src/api.cc | 60 +++++++------ src/debug/debug-evaluate.cc | 32 +------ src/debug/debug-evaluate.h | 1 - src/debug/debug.cc | 59 +++++++++++-- src/debug/debug.h | 6 +- src/external-reference-table.cc | 4 +- src/heap/factory-inl.h | 2 +- src/heap/factory.h | 2 +- src/heap/heap-inl.h | 2 +- src/heap/heap.h | 5 +- src/heap/setup-heap-internal.cc | 13 +-- src/objects/api-callbacks-inl.h | 18 +++- src/objects/api-callbacks.h | 23 +++-- src/profiler/heap-snapshot-generator.cc | 3 +- test/cctest/test-api-accessors.cc | 66 +++++++++++++- test/cctest/test-roots.cc | 2 +- ...g-evaluate-no-side-effect-runtime-check.js | 3 +- .../debug-evaluate-no-side-effect.js | 22 ++++- 23 files changed, 337 insertions(+), 162 deletions(-) diff --git a/include/v8.h b/include/v8.h index fb34f5cd64..706b747f81 100644 --- a/include/v8.h +++ b/include/v8.h @@ -3279,10 +3279,17 @@ enum PropertyFilter { * Options for marking whether callbacks may trigger JS-observable side effects. * Side-effect-free callbacks are whitelisted during debug evaluation with * throwOnSideEffect. It applies when calling a Function, FunctionTemplate, - * or an Accessor's getter callback. For Interceptors, please see + * or an Accessor callback. For Interceptors, please see * PropertyHandlerFlags's kHasNoSideEffect. + * Callbacks that only cause side effects to the receiver are whitelisted if + * invoked on receiver objects that are created within the same debug-evaluate + * call, as these objects are temporary and the side effect does not escape. */ -enum class SideEffectType { kHasSideEffect, kHasNoSideEffect }; +enum class SideEffectType { + kHasSideEffect, + kHasNoSideEffect, + kHasSideEffectToReceiver +}; /** * Keys/Properties filter enums: @@ -3423,7 +3430,8 @@ class V8_EXPORT Object : public Value { AccessorNameGetterCallback getter, AccessorNameSetterCallback setter = 0, MaybeLocal data = MaybeLocal(), AccessControl settings = DEFAULT, PropertyAttribute attribute = None, - SideEffectType getter_side_effect_type = SideEffectType::kHasSideEffect); + SideEffectType getter_side_effect_type = SideEffectType::kHasSideEffect, + SideEffectType setter_side_effect_type = SideEffectType::kHasSideEffect); void SetAccessorProperty(Local name, Local getter, Local setter = Local(), @@ -3439,7 +3447,8 @@ class V8_EXPORT Object : public Value { AccessorNameGetterCallback getter, AccessorNameSetterCallback setter = nullptr, Local data = Local(), PropertyAttribute attributes = None, - SideEffectType getter_side_effect_type = SideEffectType::kHasSideEffect); + SideEffectType getter_side_effect_type = SideEffectType::kHasSideEffect, + SideEffectType setter_side_effect_type = SideEffectType::kHasSideEffect); /** * Attempts to create a property with the given name which behaves like a data @@ -3453,7 +3462,8 @@ class V8_EXPORT Object : public Value { Local context, Local name, AccessorNameGetterCallback getter, Local data = Local(), PropertyAttribute attributes = None, - SideEffectType getter_side_effect_type = SideEffectType::kHasSideEffect); + SideEffectType getter_side_effect_type = SideEffectType::kHasSideEffect, + SideEffectType setter_side_effect_type = SideEffectType::kHasSideEffect); /** * Functionality for private properties. @@ -5388,7 +5398,8 @@ class V8_EXPORT Template : public Data { Local data = Local(), PropertyAttribute attribute = None, Local signature = Local(), AccessControl settings = DEFAULT, - SideEffectType getter_side_effect_type = SideEffectType::kHasSideEffect); + SideEffectType getter_side_effect_type = SideEffectType::kHasSideEffect, + SideEffectType setter_side_effect_type = SideEffectType::kHasSideEffect); void SetNativeDataProperty( Local name, AccessorNameGetterCallback getter, AccessorNameSetterCallback setter = 0, @@ -5396,7 +5407,8 @@ class V8_EXPORT Template : public Data { Local data = Local(), PropertyAttribute attribute = None, Local signature = Local(), AccessControl settings = DEFAULT, - SideEffectType getter_side_effect_type = SideEffectType::kHasSideEffect); + SideEffectType getter_side_effect_type = SideEffectType::kHasSideEffect, + SideEffectType setter_side_effect_type = SideEffectType::kHasSideEffect); /** * Like SetNativeDataProperty, but V8 will replace the native data property @@ -5405,7 +5417,8 @@ class V8_EXPORT Template : public Data { void SetLazyDataProperty( Local name, AccessorNameGetterCallback getter, Local data = Local(), PropertyAttribute attribute = None, - SideEffectType getter_side_effect_type = SideEffectType::kHasSideEffect); + SideEffectType getter_side_effect_type = SideEffectType::kHasSideEffect, + SideEffectType setter_side_effect_type = SideEffectType::kHasSideEffect); /** * During template instantiation, sets the value with the intrinsic property @@ -6120,13 +6133,15 @@ class V8_EXPORT ObjectTemplate : public Template { AccessorSetterCallback setter = 0, Local data = Local(), AccessControl settings = DEFAULT, PropertyAttribute attribute = None, Local signature = Local(), - SideEffectType getter_side_effect_type = SideEffectType::kHasSideEffect); + SideEffectType getter_side_effect_type = SideEffectType::kHasSideEffect, + SideEffectType setter_side_effect_type = SideEffectType::kHasSideEffect); void SetAccessor( Local name, AccessorNameGetterCallback getter, AccessorNameSetterCallback setter = 0, Local data = Local(), AccessControl settings = DEFAULT, PropertyAttribute attribute = None, Local signature = Local(), - SideEffectType getter_side_effect_type = SideEffectType::kHasSideEffect); + SideEffectType getter_side_effect_type = SideEffectType::kHasSideEffect, + SideEffectType setter_side_effect_type = SideEffectType::kHasSideEffect); /** * Sets a named property handler on the object template. diff --git a/src/accessors.cc b/src/accessors.cc index da935f3652..7b8314ccc3 100644 --- a/src/accessors.cc +++ b/src/accessors.cc @@ -31,7 +31,8 @@ Handle Accessors::MakeAccessor( info->set_is_special_data_property(true); info->set_is_sloppy(false); info->set_replace_on_access(false); - info->set_has_no_side_effect(false); + info->set_getter_side_effect_type(SideEffectType::kHasSideEffect); + info->set_setter_side_effect_type(SideEffectType::kHasSideEffect); name = factory->InternalizeName(name); info->set_name(*name); Handle get = v8::FromCData(isolate, getter); diff --git a/src/accessors.h b/src/accessors.h index 69fdbbb74e..a8ae7da689 100644 --- a/src/accessors.h +++ b/src/accessors.h @@ -22,27 +22,27 @@ class JavaScriptFrame; // The list of accessor descriptors. This is a second-order macro // taking a macro to be applied to all accessor descriptor names. -#define ACCESSOR_INFO_LIST(V) \ - V(arguments_iterator, ArgumentsIterator) \ - V(array_length, ArrayLength) \ - V(bound_function_length, BoundFunctionLength) \ - V(bound_function_name, BoundFunctionName) \ - V(error_stack, ErrorStack) \ - V(function_arguments, FunctionArguments) \ - V(function_caller, FunctionCaller) \ - V(function_name, FunctionName) \ - V(function_length, FunctionLength) \ - V(function_prototype, FunctionPrototype) \ - V(string_length, StringLength) - -#define SIDE_EFFECT_FREE_ACCESSOR_INFO_LIST(V) \ - V(ArrayLength) \ - V(BoundFunctionLength) \ - V(BoundFunctionName) \ - V(FunctionName) \ - V(FunctionLength) \ - V(FunctionPrototype) \ - V(StringLength) +// V(accessor_name, AccessorName, GetterSideEffectType, SetterSideEffectType) +#define ACCESSOR_INFO_LIST(V) \ + V(arguments_iterator, ArgumentsIterator, kHasNoSideEffect, \ + kHasSideEffectToReceiver) \ + V(array_length, ArrayLength, kHasNoSideEffect, kHasSideEffectToReceiver) \ + V(bound_function_length, BoundFunctionLength, kHasNoSideEffect, \ + kHasSideEffectToReceiver) \ + V(bound_function_name, BoundFunctionName, kHasNoSideEffect, \ + kHasSideEffectToReceiver) \ + V(error_stack, ErrorStack, kHasSideEffectToReceiver, \ + kHasSideEffectToReceiver) \ + V(function_arguments, FunctionArguments, kHasNoSideEffect, \ + kHasSideEffectToReceiver) \ + V(function_caller, FunctionCaller, kHasNoSideEffect, \ + kHasSideEffectToReceiver) \ + V(function_name, FunctionName, kHasNoSideEffect, kHasSideEffectToReceiver) \ + V(function_length, FunctionLength, kHasNoSideEffect, \ + kHasSideEffectToReceiver) \ + V(function_prototype, FunctionPrototype, kHasNoSideEffect, \ + kHasSideEffectToReceiver) \ + V(string_length, StringLength, kHasNoSideEffect, kHasSideEffectToReceiver) #define ACCESSOR_SETTER_LIST(V) \ V(ArrayLengthSetter) \ @@ -55,9 +55,9 @@ class JavaScriptFrame; class Accessors : public AllStatic { public: -#define ACCESSOR_GETTER_DECLARATION(accessor_name, AccessorName) \ - static void AccessorName##Getter( \ - v8::Local name, \ +#define ACCESSOR_GETTER_DECLARATION(accessor_name, AccessorName, ...) \ + static void AccessorName##Getter( \ + v8::Local name, \ const v8::PropertyCallbackInfo& info); ACCESSOR_INFO_LIST(ACCESSOR_GETTER_DECLARATION) #undef ACCESSOR_GETTER_DECLARATION @@ -118,7 +118,7 @@ class Accessors : public AllStatic { AccessorNameBooleanSetterCallback setter); private: -#define ACCESSOR_INFO_DECLARATION(accessor_name, AccessorName) \ +#define ACCESSOR_INFO_DECLARATION(accessor_name, AccessorName, ...) \ static Handle Make##AccessorName##Info(Isolate* isolate); ACCESSOR_INFO_LIST(ACCESSOR_INFO_DECLARATION) #undef ACCESSOR_INFO_DECLARATION diff --git a/src/api-arguments-inl.h b/src/api-arguments-inl.h index 89f606ed41..1e5d6b2aaa 100644 --- a/src/api-arguments-inl.h +++ b/src/api-arguments-inl.h @@ -8,6 +8,7 @@ #include "src/api-arguments.h" #include "src/api-inl.h" +#include "src/debug/debug.h" #include "src/objects/api-callbacks.h" #include "src/tracing/trace-event.h" #include "src/vm-state-inl.h" @@ -34,6 +35,10 @@ inline JSObject* PropertyCallbackArguments::holder() { return JSObject::cast(this->begin()[T::kHolderIndex]); } +inline Object* PropertyCallbackArguments::receiver() { + return Object::cast(this->begin()[T::kThisIndex]); +} + inline JSObject* FunctionCallbackArguments::holder() { return JSObject::cast(this->begin()[T::kHolderIndex]); } @@ -47,14 +52,24 @@ inline JSObject* FunctionCallbackArguments::holder() { DCHECK(!name->IsPrivate()); \ DCHECK_IMPLIES(name->IsSymbol(), interceptor->can_intercept_symbols()); -#define PREPARE_CALLBACK_INFO(ISOLATE, F, RETURN_VALUE, API_RETURN_TYPE, \ - CALLBACK_INFO) \ - if (ISOLATE->debug_execution_mode() == DebugInfo::kSideEffects && \ - !ISOLATE->debug()->PerformSideEffectCheckForCallback(CALLBACK_INFO)) { \ - return RETURN_VALUE(); \ - } \ - VMState state(ISOLATE); \ - ExternalCallbackScope call_scope(ISOLATE, FUNCTION_ADDR(F)); \ +#define PREPARE_CALLBACK_INFO(ISOLATE, F, RETURN_VALUE, API_RETURN_TYPE, \ + CALLBACK_INFO, RECEIVER, ACCESSOR_KIND) \ + if (ISOLATE->debug_execution_mode() == DebugInfo::kSideEffects && \ + !ISOLATE->debug()->PerformSideEffectCheckForCallback( \ + CALLBACK_INFO, RECEIVER, Debug::k##ACCESSOR_KIND)) { \ + return RETURN_VALUE(); \ + } \ + VMState state(ISOLATE); \ + ExternalCallbackScope call_scope(ISOLATE, FUNCTION_ADDR(F)); \ + PropertyCallbackInfo callback_info(begin()); + +#define PREPARE_CALLBACK_INFO_FAIL_SIDE_EFFECT_CHECK(ISOLATE, F, RETURN_VALUE, \ + API_RETURN_TYPE) \ + if (ISOLATE->debug_execution_mode() == DebugInfo::kSideEffects) { \ + return RETURN_VALUE(); \ + } \ + VMState state(ISOLATE); \ + ExternalCallbackScope call_scope(ISOLATE, FUNCTION_ADDR(F)); \ PropertyCallbackInfo callback_info(begin()); #define CREATE_NAMED_CALLBACK(FUNCTION, TYPE, RETURN_TYPE, API_RETURN_TYPE, \ @@ -65,11 +80,13 @@ inline JSObject* FunctionCallbackArguments::holder() { Isolate* isolate = this->isolate(); \ RuntimeCallTimerScope timer( \ isolate, RuntimeCallCounterId::kNamed##FUNCTION##Callback); \ + Handle receiver_check_unsupported; \ GenericNamedProperty##FUNCTION##Callback f = \ ToCData( \ interceptor->TYPE()); \ PREPARE_CALLBACK_INFO(isolate, f, Handle, API_RETURN_TYPE, \ - INFO_FOR_SIDE_EFFECT); \ + INFO_FOR_SIDE_EFFECT, receiver_check_unsupported, \ + NotAccessor); \ LOG(isolate, \ ApiNamedPropertyAccess("interceptor-named-" #TYPE, holder(), *name)); \ f(v8::Utils::ToLocal(name), callback_info); \ @@ -87,10 +104,12 @@ FOR_EACH_CALLBACK(CREATE_NAMED_CALLBACK) Isolate* isolate = this->isolate(); \ RuntimeCallTimerScope timer( \ isolate, RuntimeCallCounterId::kIndexed##FUNCTION##Callback); \ + Handle receiver_check_unsupported; \ IndexedProperty##FUNCTION##Callback f = \ ToCData(interceptor->TYPE()); \ PREPARE_CALLBACK_INFO(isolate, f, Handle, API_RETURN_TYPE, \ - INFO_FOR_SIDE_EFFECT); \ + INFO_FOR_SIDE_EFFECT, receiver_check_unsupported, \ + NotAccessor); \ LOG(isolate, ApiIndexedPropertyAccess("interceptor-indexed-" #TYPE, \ holder(), index)); \ f(index, callback_info); \ @@ -108,9 +127,11 @@ Handle FunctionCallbackArguments::Call(CallHandlerInfo* handler) { RuntimeCallTimerScope timer(isolate, RuntimeCallCounterId::kFunctionCallback); v8::FunctionCallback f = v8::ToCData(handler->callback()); + Handle receiver_check_unsupported; if (isolate->debug_execution_mode() == DebugInfo::kSideEffects && !isolate->debug()->PerformSideEffectCheckForCallback( - handle(handler, isolate))) { + handle(handler, isolate), receiver_check_unsupported, + Debug::kNotAccessor)) { return Handle(); } VMState state(isolate); @@ -167,10 +188,11 @@ Handle PropertyCallbackArguments::CallNamedDescriptor( Handle PropertyCallbackArguments::BasicCallNamedGetterCallback( GenericNamedPropertyGetterCallback f, Handle name, - Handle info) { + Handle info, Handle receiver) { DCHECK(!name->IsPrivate()); Isolate* isolate = this->isolate(); - PREPARE_CALLBACK_INFO(isolate, f, Handle, v8::Value, info); + PREPARE_CALLBACK_INFO(isolate, f, Handle, v8::Value, info, receiver, + Getter); f(v8::Utils::ToLocal(name), callback_info); return GetReturnValue(isolate); } @@ -184,9 +206,8 @@ Handle PropertyCallbackArguments::CallNamedSetter( Isolate* isolate = this->isolate(); RuntimeCallTimerScope timer(isolate, RuntimeCallCounterId::kNamedSetterCallback); - Handle side_effect_check_not_supported; - PREPARE_CALLBACK_INFO(isolate, f, Handle, v8::Value, - side_effect_check_not_supported); + PREPARE_CALLBACK_INFO_FAIL_SIDE_EFFECT_CHECK(isolate, f, Handle, + v8::Value); LOG(isolate, ApiNamedPropertyAccess("interceptor-named-set", holder(), *name)); f(v8::Utils::ToLocal(name), v8::Utils::ToLocal(value), callback_info); @@ -202,9 +223,8 @@ Handle PropertyCallbackArguments::CallNamedDefiner( RuntimeCallCounterId::kNamedDefinerCallback); GenericNamedPropertyDefinerCallback f = ToCData(interceptor->definer()); - Handle side_effect_check_not_supported; - PREPARE_CALLBACK_INFO(isolate, f, Handle, v8::Value, - side_effect_check_not_supported); + PREPARE_CALLBACK_INFO_FAIL_SIDE_EFFECT_CHECK(isolate, f, Handle, + v8::Value); LOG(isolate, ApiNamedPropertyAccess("interceptor-named-define", holder(), *name)); f(v8::Utils::ToLocal(name), desc, callback_info); @@ -219,9 +239,8 @@ Handle PropertyCallbackArguments::CallIndexedSetter( RuntimeCallCounterId::kIndexedSetterCallback); IndexedPropertySetterCallback f = ToCData(interceptor->setter()); - Handle side_effect_check_not_supported; - PREPARE_CALLBACK_INFO(isolate, f, Handle, v8::Value, - side_effect_check_not_supported); + PREPARE_CALLBACK_INFO_FAIL_SIDE_EFFECT_CHECK(isolate, f, Handle, + v8::Value); LOG(isolate, ApiIndexedPropertyAccess("interceptor-indexed-set", holder(), index)); f(index, v8::Utils::ToLocal(value), callback_info); @@ -237,9 +256,8 @@ Handle PropertyCallbackArguments::CallIndexedDefiner( RuntimeCallCounterId::kIndexedDefinerCallback); IndexedPropertyDefinerCallback f = ToCData(interceptor->definer()); - Handle side_effect_check_not_supported; - PREPARE_CALLBACK_INFO(isolate, f, Handle, v8::Value, - side_effect_check_not_supported); + PREPARE_CALLBACK_INFO_FAIL_SIDE_EFFECT_CHECK(isolate, f, Handle, + v8::Value); LOG(isolate, ApiIndexedPropertyAccess("interceptor-indexed-define", holder(), index)); f(index, desc, callback_info); @@ -275,7 +293,9 @@ Handle PropertyCallbackArguments::CallIndexedDescriptor( Handle PropertyCallbackArguments::BasicCallIndexedGetterCallback( IndexedPropertyGetterCallback f, uint32_t index, Handle info) { Isolate* isolate = this->isolate(); - PREPARE_CALLBACK_INFO(isolate, f, Handle, v8::Value, info); + Handle receiver_check_unsupported; + PREPARE_CALLBACK_INFO(isolate, f, Handle, v8::Value, info, + receiver_check_unsupported, Getter); f(index, callback_info); return GetReturnValue(isolate); } @@ -287,7 +307,9 @@ Handle PropertyCallbackArguments::CallPropertyEnumerator( v8::ToCData(interceptor->enumerator()); // TODO(cbruni): assert same type for indexed and named callback. Isolate* isolate = this->isolate(); - PREPARE_CALLBACK_INFO(isolate, f, Handle, v8::Array, interceptor); + Handle receiver_check_unsupported; + PREPARE_CALLBACK_INFO(isolate, f, Handle, v8::Array, interceptor, + receiver_check_unsupported, NotAccessor); f(callback_info); return GetReturnValue(isolate); } @@ -303,7 +325,8 @@ Handle PropertyCallbackArguments::CallAccessorGetter( LOG(isolate, ApiNamedPropertyAccess("accessor-getter", holder(), *name)); AccessorNameGetterCallback f = ToCData(info->getter()); - return BasicCallNamedGetterCallback(f, name, info); + return BasicCallNamedGetterCallback(f, name, info, + handle(receiver(), isolate)); } Handle PropertyCallbackArguments::CallAccessorSetter( @@ -314,15 +337,15 @@ Handle PropertyCallbackArguments::CallAccessorSetter( RuntimeCallCounterId::kAccessorSetterCallback); AccessorNameSetterCallback f = ToCData(accessor_info->setter()); - Handle side_effect_check_not_supported; - PREPARE_CALLBACK_INFO(isolate, f, Handle, void, - side_effect_check_not_supported); + PREPARE_CALLBACK_INFO(isolate, f, Handle, void, accessor_info, + handle(receiver(), isolate), Setter); LOG(isolate, ApiNamedPropertyAccess("accessor-setter", holder(), *name)); f(v8::Utils::ToLocal(name), v8::Utils::ToLocal(value), callback_info); return GetReturnValue(isolate); } #undef PREPARE_CALLBACK_INFO +#undef PREPARE_CALLBACK_INFO_FAIL_SIDE_EFFECT_CHECK } // namespace internal } // namespace v8 diff --git a/src/api-arguments.h b/src/api-arguments.h index 0a0a7362c7..7e19d209ab 100644 --- a/src/api-arguments.h +++ b/src/api-arguments.h @@ -133,9 +133,10 @@ class PropertyCallbackArguments IndexedPropertyGetterCallback f, uint32_t index, Handle info); inline Handle BasicCallNamedGetterCallback( GenericNamedPropertyGetterCallback f, Handle name, - Handle info); + Handle info, Handle receiver = Handle()); inline JSObject* holder(); + inline Object* receiver(); // Don't copy PropertyCallbackArguments, because they would both have the // same prev_ pointer. diff --git a/src/api.cc b/src/api.cc index e573bddf19..2f047edecc 100644 --- a/src/api.cc +++ b/src/api.cc @@ -1676,7 +1676,8 @@ static void TemplateSetAccessor( Template* template_obj, v8::Local name, Getter getter, Setter setter, Data data, AccessControl settings, PropertyAttribute attribute, v8::Local signature, bool is_special_data_property, - bool replace_on_access, SideEffectType getter_side_effect_type) { + bool replace_on_access, SideEffectType getter_side_effect_type, + SideEffectType setter_side_effect_type) { auto info = Utils::OpenHandle(template_obj); auto isolate = info->GetIsolate(); ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate); @@ -1686,8 +1687,8 @@ static void TemplateSetAccessor( is_special_data_property, replace_on_access); accessor_info->set_initial_property_attributes( static_cast(attribute)); - accessor_info->set_has_no_side_effect(getter_side_effect_type == - SideEffectType::kHasNoSideEffect); + accessor_info->set_getter_side_effect_type(getter_side_effect_type); + accessor_info->set_setter_side_effect_type(setter_side_effect_type); i::ApiNatives::AddNativeDataProperty(isolate, info, accessor_info); } @@ -1695,29 +1696,34 @@ void Template::SetNativeDataProperty( v8::Local name, AccessorGetterCallback getter, AccessorSetterCallback setter, v8::Local data, PropertyAttribute attribute, v8::Local signature, - AccessControl settings, SideEffectType getter_side_effect_type) { + AccessControl settings, SideEffectType getter_side_effect_type, + SideEffectType setter_side_effect_type) { TemplateSetAccessor(this, name, getter, setter, data, settings, attribute, - signature, true, false, getter_side_effect_type); + signature, true, false, getter_side_effect_type, + setter_side_effect_type); } void Template::SetNativeDataProperty( v8::Local name, AccessorNameGetterCallback getter, AccessorNameSetterCallback setter, v8::Local data, PropertyAttribute attribute, v8::Local signature, - AccessControl settings, SideEffectType getter_side_effect_type) { + AccessControl settings, SideEffectType getter_side_effect_type, + SideEffectType setter_side_effect_type) { TemplateSetAccessor(this, name, getter, setter, data, settings, attribute, - signature, true, false, getter_side_effect_type); + signature, true, false, getter_side_effect_type, + setter_side_effect_type); } void Template::SetLazyDataProperty(v8::Local name, AccessorNameGetterCallback getter, v8::Local data, PropertyAttribute attribute, - SideEffectType getter_side_effect_type) { + SideEffectType getter_side_effect_type, + SideEffectType setter_side_effect_type) { TemplateSetAccessor(this, name, getter, static_cast(nullptr), data, DEFAULT, attribute, Local(), true, - true, getter_side_effect_type); + true, getter_side_effect_type, setter_side_effect_type); } void Template::SetIntrinsicDataProperty(Local name, Intrinsic intrinsic, @@ -1737,10 +1743,11 @@ void ObjectTemplate::SetAccessor(v8::Local name, v8::Local data, AccessControl settings, PropertyAttribute attribute, v8::Local signature, - SideEffectType getter_side_effect_type) { + SideEffectType getter_side_effect_type, + SideEffectType setter_side_effect_type) { TemplateSetAccessor(this, name, getter, setter, data, settings, attribute, signature, i::FLAG_disable_old_api_accessors, false, - getter_side_effect_type); + getter_side_effect_type, setter_side_effect_type); } void ObjectTemplate::SetAccessor(v8::Local name, @@ -1749,10 +1756,11 @@ void ObjectTemplate::SetAccessor(v8::Local name, v8::Local data, AccessControl settings, PropertyAttribute attribute, v8::Local signature, - SideEffectType getter_side_effect_type) { + SideEffectType getter_side_effect_type, + SideEffectType setter_side_effect_type) { TemplateSetAccessor(this, name, getter, setter, data, settings, attribute, signature, i::FLAG_disable_old_api_accessors, false, - getter_side_effect_type); + getter_side_effect_type, setter_side_effect_type); } template ObjectSetAccessor( Local context, Object* self, Local name, Getter getter, Setter setter, Data data, AccessControl settings, PropertyAttribute attributes, bool is_special_data_property, - bool replace_on_access, - SideEffectType getter_side_effect_type = SideEffectType::kHasSideEffect) { + bool replace_on_access, SideEffectType getter_side_effect_type, + SideEffectType setter_side_effect_type) { auto isolate = reinterpret_cast(context->GetIsolate()); ENTER_V8_NO_SCRIPT(isolate, context, Object, SetAccessor, Nothing(), i::HandleScope); @@ -4581,8 +4589,8 @@ static Maybe ObjectSetAccessor( i::Handle info = MakeAccessorInfo(isolate, name, getter, setter, data, settings, signature, is_special_data_property, replace_on_access); - info->set_has_no_side_effect(getter_side_effect_type == - SideEffectType::kHasNoSideEffect); + info->set_getter_side_effect_type(getter_side_effect_type); + info->set_setter_side_effect_type(setter_side_effect_type); if (info.is_null()) return Nothing(); bool fast = obj->HasFastProperties(); i::Handle result; @@ -4605,11 +4613,12 @@ Maybe Object::SetAccessor(Local context, Local name, AccessorNameSetterCallback setter, MaybeLocal data, AccessControl settings, PropertyAttribute attribute, - SideEffectType getter_side_effect_type) { + SideEffectType getter_side_effect_type, + SideEffectType setter_side_effect_type) { return ObjectSetAccessor(context, this, name, getter, setter, data.FromMaybe(Local()), settings, attribute, i::FLAG_disable_old_api_accessors, false, - getter_side_effect_type); + getter_side_effect_type, setter_side_effect_type); } @@ -4636,19 +4645,22 @@ Maybe Object::SetNativeDataProperty( v8::Local context, v8::Local name, AccessorNameGetterCallback getter, AccessorNameSetterCallback setter, v8::Local data, PropertyAttribute attributes, - SideEffectType getter_side_effect_type) { + SideEffectType getter_side_effect_type, + SideEffectType setter_side_effect_type) { return ObjectSetAccessor(context, this, name, getter, setter, data, DEFAULT, - attributes, true, false, getter_side_effect_type); + attributes, true, false, getter_side_effect_type, + setter_side_effect_type); } Maybe Object::SetLazyDataProperty( v8::Local context, v8::Local name, AccessorNameGetterCallback getter, v8::Local data, - PropertyAttribute attributes, SideEffectType getter_side_effect_type) { + PropertyAttribute attributes, SideEffectType getter_side_effect_type, + SideEffectType setter_side_effect_type) { return ObjectSetAccessor(context, this, name, getter, static_cast(nullptr), data, DEFAULT, attributes, true, true, - getter_side_effect_type); + getter_side_effect_type, setter_side_effect_type); } Maybe v8::Object::HasOwnProperty(Local context, @@ -9684,7 +9696,7 @@ int debug::GetNativeAccessorDescriptor(v8::Local context, } auto isolate = reinterpret_cast(context->GetIsolate()); int result = 0; -#define IS_BUILTIN_ACESSOR(name, _) \ +#define IS_BUILTIN_ACESSOR(name, ...) \ if (*structure == *isolate->factory()->name##_accessor()) \ result |= static_cast(debug::NativeAccessorType::IsBuiltin); ACCESSOR_INFO_LIST(IS_BUILTIN_ACESSOR) diff --git a/src/debug/debug-evaluate.cc b/src/debug/debug-evaluate.cc index c2353a8e0c..3cf7576678 100644 --- a/src/debug/debug-evaluate.cc +++ b/src/debug/debug-evaluate.cc @@ -16,7 +16,6 @@ #include "src/interpreter/bytecode-array-iterator.h" #include "src/interpreter/bytecodes.h" #include "src/isolate-inl.h" -#include "src/objects/api-callbacks.h" #include "src/snapshot/snapshot.h" namespace v8 { @@ -796,7 +795,10 @@ DebugInfo::SideEffectState BuiltinGetSideEffectState(Builtins::Name id) { case Builtins::kMakeURIError: // RegExp builtins. case Builtins::kRegExpConstructor: + // Internal. + case Builtins::kStrictPoisonPillThrower: return DebugInfo::kHasNoSideEffect; + // Set builtins. case Builtins::kSetIteratorPrototypeNext: case Builtins::kSetPrototypeAdd: @@ -950,34 +952,6 @@ DebugInfo::SideEffectState DebugEvaluate::FunctionGetSideEffectState( return DebugInfo::kHasSideEffects; } -// static -bool DebugEvaluate::CallbackHasNoSideEffect(Object* callback_info) { - DisallowHeapAllocation no_gc; - if (callback_info->IsAccessorInfo()) { - // List of whitelisted internal accessors can be found in accessors.h. - AccessorInfo* info = AccessorInfo::cast(callback_info); - if (info->has_no_side_effect()) return true; - if (FLAG_trace_side_effect_free_debug_evaluate) { - PrintF("[debug-evaluate] API Callback '"); - info->name()->ShortPrint(); - PrintF("' may cause side effect.\n"); - } - } else if (callback_info->IsInterceptorInfo()) { - InterceptorInfo* info = InterceptorInfo::cast(callback_info); - if (info->has_no_side_effect()) return true; - if (FLAG_trace_side_effect_free_debug_evaluate) { - PrintF("[debug-evaluate] API Interceptor may cause side effect.\n"); - } - } else if (callback_info->IsCallHandlerInfo()) { - CallHandlerInfo* info = CallHandlerInfo::cast(callback_info); - if (info->IsSideEffectFreeCallHandlerInfo()) return true; - if (FLAG_trace_side_effect_free_debug_evaluate) { - PrintF("[debug-evaluate] API CallHandlerInfo may cause side effect.\n"); - } - } - return false; -} - // static void DebugEvaluate::ApplySideEffectChecks( Handle bytecode_array) { diff --git a/src/debug/debug-evaluate.h b/src/debug/debug-evaluate.h index 420c6c208b..470a4900a7 100644 --- a/src/debug/debug-evaluate.h +++ b/src/debug/debug-evaluate.h @@ -41,7 +41,6 @@ class DebugEvaluate : public AllStatic { static DebugInfo::SideEffectState FunctionGetSideEffectState( Isolate* isolate, Handle info); - static bool CallbackHasNoSideEffect(Object* callback_info); static void ApplySideEffectChecks(Handle bytecode_array); private: diff --git a/src/debug/debug.cc b/src/debug/debug.cc index 2de33ac66e..89fc077fc8 100644 --- a/src/debug/debug.cc +++ b/src/debug/debug.cc @@ -29,6 +29,7 @@ #include "src/isolate-inl.h" #include "src/log.h" #include "src/messages.h" +#include "src/objects/api-callbacks-inl.h" #include "src/objects/debug-objects-inl.h" #include "src/objects/js-generator-inl.h" #include "src/objects/js-promise-inl.h" @@ -2180,16 +2181,55 @@ Handle Debug::return_value_handle() { return handle(thread_local_.return_value_, isolate_); } -bool Debug::PerformSideEffectCheckForCallback(Handle callback_info) { +bool Debug::PerformSideEffectCheckForCallback( + Handle callback_info, Handle receiver, + Debug::AccessorKind accessor_kind) { + DCHECK_EQ(!receiver.is_null(), callback_info->IsAccessorInfo()); DCHECK_EQ(isolate_->debug_execution_mode(), DebugInfo::kSideEffects); if (!callback_info.is_null() && callback_info->IsCallHandlerInfo() && i::CallHandlerInfo::cast(*callback_info)->NextCallHasNoSideEffect()) { return true; } // TODO(7515): always pass a valid callback info object. - if (!callback_info.is_null() && - DebugEvaluate::CallbackHasNoSideEffect(*callback_info)) { - return true; + if (!callback_info.is_null()) { + if (callback_info->IsAccessorInfo()) { + // List of whitelisted internal accessors can be found in accessors.h. + AccessorInfo* info = AccessorInfo::cast(*callback_info); + DCHECK_NE(kNotAccessor, accessor_kind); + switch (accessor_kind == kSetter ? info->setter_side_effect_type() + : info->getter_side_effect_type()) { + case SideEffectType::kHasNoSideEffect: + // We do not support setter accessors with no side effects, since + // calling set accessors go through a store bytecode. Store bytecodes + // are considered to cause side effects (to non-temporary objects). + DCHECK_NE(kSetter, accessor_kind); + return true; + case SideEffectType::kHasSideEffectToReceiver: + DCHECK(!receiver.is_null()); + if (PerformSideEffectCheckForObject(receiver)) return true; + isolate_->OptionalRescheduleException(false); + return false; + case SideEffectType::kHasSideEffect: + break; + } + if (FLAG_trace_side_effect_free_debug_evaluate) { + PrintF("[debug-evaluate] API Callback '"); + info->name()->ShortPrint(); + PrintF("' may cause side effect.\n"); + } + } else if (callback_info->IsInterceptorInfo()) { + InterceptorInfo* info = InterceptorInfo::cast(*callback_info); + if (info->has_no_side_effect()) return true; + if (FLAG_trace_side_effect_free_debug_evaluate) { + PrintF("[debug-evaluate] API Interceptor may cause side effect.\n"); + } + } else if (callback_info->IsCallHandlerInfo()) { + CallHandlerInfo* info = CallHandlerInfo::cast(*callback_info); + if (info->IsSideEffectFreeCallHandlerInfo()) return true; + if (FLAG_trace_side_effect_free_debug_evaluate) { + PrintF("[debug-evaluate] API CallHandlerInfo may cause side effect.\n"); + } + } } side_effect_check_failed_ = true; // Throw an uncatchable termination exception. @@ -2226,11 +2266,14 @@ bool Debug::PerformSideEffectCheckAtBytecode(InterpretedFrame* frame) { bool Debug::PerformSideEffectCheckForObject(Handle object) { DCHECK_EQ(isolate_->debug_execution_mode(), DebugInfo::kSideEffects); - if (object->IsHeapObject()) { - if (temporary_objects_->HasObject(Handle::cast(object))) { - return true; - } + // We expect no side-effects for primitives. + if (object->IsNumber()) return true; + if (object->IsName()) return true; + + if (temporary_objects_->HasObject(Handle::cast(object))) { + return true; } + if (FLAG_trace_side_effect_free_debug_evaluate) { PrintF("[debug-evaluate] failed runtime side effect check.\n"); } diff --git a/src/debug/debug.h b/src/debug/debug.h index a6ad7bd4da..8d13504005 100644 --- a/src/debug/debug.h +++ b/src/debug/debug.h @@ -326,7 +326,11 @@ class Debug { bool PerformSideEffectCheck(Handle function, Handle receiver); - bool PerformSideEffectCheckForCallback(Handle callback_info); + + enum AccessorKind { kNotAccessor, kGetter, kSetter }; + bool PerformSideEffectCheckForCallback(Handle callback_info, + Handle receiver, + AccessorKind accessor_kind); bool PerformSideEffectCheckAtBytecode(InterpretedFrame* frame); bool PerformSideEffectCheckForObject(Handle object); diff --git a/src/external-reference-table.cc b/src/external-reference-table.cc index 4d555e1829..afd3354d89 100644 --- a/src/external-reference-table.cc +++ b/src/external-reference-table.cc @@ -159,8 +159,8 @@ void ExternalReferenceTable::AddAccessors(int* index) { }; static const AccessorRefTable getters[] = { -#define ACCESSOR_INFO_DECLARATION(accessor_name, AccessorName) \ - {FUNCTION_ADDR(&Accessors::AccessorName##Getter), \ +#define ACCESSOR_INFO_DECLARATION(accessor_name, AccessorName, ...) \ + {FUNCTION_ADDR(&Accessors::AccessorName##Getter), \ "Accessors::" #AccessorName "Getter"}, /* NOLINT(whitespace/indent) */ ACCESSOR_INFO_LIST(ACCESSOR_INFO_DECLARATION) #undef ACCESSOR_INFO_DECLARATION diff --git a/src/heap/factory-inl.h b/src/heap/factory-inl.h index 614c6ec174..df59109bbb 100644 --- a/src/heap/factory-inl.h +++ b/src/heap/factory-inl.h @@ -73,7 +73,7 @@ PUBLIC_SYMBOL_LIST(SYMBOL_ACCESSOR) WELL_KNOWN_SYMBOL_LIST(SYMBOL_ACCESSOR) #undef SYMBOL_ACCESSOR -#define ACCESSOR_INFO_ACCESSOR(accessor_name, AccessorName) \ +#define ACCESSOR_INFO_ACCESSOR(accessor_name, AccessorName, ...) \ Handle Factory::accessor_name##_accessor() { \ return Handle(bit_cast( \ &isolate() \ diff --git a/src/heap/factory.h b/src/heap/factory.h index 8d53a479e6..b6c5cf8b1c 100644 --- a/src/heap/factory.h +++ b/src/heap/factory.h @@ -861,7 +861,7 @@ class V8_EXPORT_PRIVATE Factory { WELL_KNOWN_SYMBOL_LIST(SYMBOL_ACCESSOR) #undef SYMBOL_ACCESSOR -#define ACCESSOR_INFO_ACCESSOR(accessor_name, AccessorName) \ +#define ACCESSOR_INFO_ACCESSOR(accessor_name, ...) \ inline Handle accessor_name##_accessor(); ACCESSOR_INFO_LIST(ACCESSOR_INFO_ACCESSOR) #undef ACCESSOR_INFO_ACCESSOR diff --git a/src/heap/heap-inl.h b/src/heap/heap-inl.h index 62f07ea322..ac1740b22a 100644 --- a/src/heap/heap-inl.h +++ b/src/heap/heap-inl.h @@ -64,7 +64,7 @@ MUTABLE_ROOT_LIST(ROOT_ACCESSOR) DATA_HANDLER_LIST(DATA_HANDLER_MAP_ACCESSOR) #undef DATA_HANDLER_MAP_ACCESSOR -#define ACCESSOR_INFO_ACCESSOR(accessor_name, AccessorName) \ +#define ACCESSOR_INFO_ACCESSOR(accessor_name, AccessorName, ...) \ AccessorInfo* Heap::accessor_name##_accessor() { \ return AccessorInfo::cast(roots_[k##AccessorName##AccessorRootIndex]); \ } diff --git a/src/heap/heap.h b/src/heap/heap.h index 2e750d56fa..c67d64cd36 100644 --- a/src/heap/heap.h +++ b/src/heap/heap.h @@ -325,7 +325,8 @@ class Heap { WELL_KNOWN_SYMBOL_LIST(DECL) #undef DECL -#define DECL(accessor_name, AccessorName) k##AccessorName##AccessorRootIndex, +#define DECL(accessor_name, AccessorName, ...) \ + k##AccessorName##AccessorRootIndex, ACCESSOR_INFO_LIST(DECL) #undef DECL @@ -821,7 +822,7 @@ class Heap { DATA_HANDLER_LIST(DATA_HANDLER_MAP_ACCESSOR) #undef DATA_HANDLER_MAP_ACCESSOR -#define ACCESSOR_INFO_ACCESSOR(accessor_name, AccessorName) \ +#define ACCESSOR_INFO_ACCESSOR(accessor_name, ...) \ inline AccessorInfo* accessor_name##_accessor(); ACCESSOR_INFO_LIST(ACCESSOR_INFO_ACCESSOR) #undef ACCESSOR_INFO_ACCESSOR diff --git a/src/heap/setup-heap-internal.cc b/src/heap/setup-heap-internal.cc index 29476fc5f0..b2bdd65551 100644 --- a/src/heap/setup-heap-internal.cc +++ b/src/heap/setup-heap-internal.cc @@ -901,16 +901,19 @@ void Heap::CreateInternalAccessorInfoObjects() { HandleScope scope(isolate); Handle acessor_info; -#define INIT_ACCESSOR_INFO(accessor_name, AccessorName) \ +#define INIT_ACCESSOR_INFO(accessor_name, AccessorName, ...) \ acessor_info = Accessors::Make##AccessorName##Info(isolate); \ roots_[k##AccessorName##AccessorRootIndex] = *acessor_info; ACCESSOR_INFO_LIST(INIT_ACCESSOR_INFO) #undef INIT_ACCESSOR_INFO -#define INIT_SIDE_EFFECT_FLAG(AccessorName) \ - AccessorInfo::cast(roots_[k##AccessorName##AccessorRootIndex]) \ - ->set_has_no_side_effect(true); - SIDE_EFFECT_FREE_ACCESSOR_INFO_LIST(INIT_SIDE_EFFECT_FLAG) +#define INIT_SIDE_EFFECT_FLAG(accessor_name, AccessorName, GetterType, \ + SetterType) \ + AccessorInfo::cast(roots_[k##AccessorName##AccessorRootIndex]) \ + ->set_getter_side_effect_type(SideEffectType::GetterType); \ + AccessorInfo::cast(roots_[k##AccessorName##AccessorRootIndex]) \ + ->set_setter_side_effect_type(SideEffectType::SetterType); + ACCESSOR_INFO_LIST(INIT_SIDE_EFFECT_FLAG) #undef INIT_SIDE_EFFECT_FLAG } diff --git a/src/objects/api-callbacks-inl.h b/src/objects/api-callbacks-inl.h index 4f7680d8ed..b8ca8bef20 100644 --- a/src/objects/api-callbacks-inl.h +++ b/src/objects/api-callbacks-inl.h @@ -59,8 +59,22 @@ BIT_FIELD_ACCESSORS(AccessorInfo, flags, is_special_data_property, BIT_FIELD_ACCESSORS(AccessorInfo, flags, replace_on_access, AccessorInfo::ReplaceOnAccessBit) BIT_FIELD_ACCESSORS(AccessorInfo, flags, is_sloppy, AccessorInfo::IsSloppyBit) -BIT_FIELD_ACCESSORS(AccessorInfo, flags, has_no_side_effect, - AccessorInfo::HasNoSideEffectBit) +BIT_FIELD_ACCESSORS(AccessorInfo, flags, getter_side_effect_type, + AccessorInfo::GetterSideEffectTypeBits) + +SideEffectType AccessorInfo::setter_side_effect_type() const { + return SetterSideEffectTypeBits::decode(flags()); +} + +void AccessorInfo::set_setter_side_effect_type(SideEffectType value) { + // We do not support describing setters as having no side effect, since + // calling set accessors must go through a store bytecode. Store bytecodes + // support checking receivers for temporary objects, but still expect + // the receiver to be written to. + CHECK_NE(value, SideEffectType::kHasNoSideEffect); + set_flags(SetterSideEffectTypeBits::update(flags(), value)); +} + BIT_FIELD_ACCESSORS(AccessorInfo, flags, initial_property_attributes, AccessorInfo::InitialAttributesBits) diff --git a/src/objects/api-callbacks.h b/src/objects/api-callbacks.h index 6f3629a2c3..f7522da8a7 100644 --- a/src/objects/api-callbacks.h +++ b/src/objects/api-callbacks.h @@ -48,7 +48,12 @@ class AccessorInfo : public Struct { DECL_BOOLEAN_ACCESSORS(is_special_data_property) DECL_BOOLEAN_ACCESSORS(replace_on_access) DECL_BOOLEAN_ACCESSORS(is_sloppy) - DECL_BOOLEAN_ACCESSORS(has_no_side_effect) + + inline SideEffectType getter_side_effect_type() const; + inline void set_getter_side_effect_type(SideEffectType type); + + inline SideEffectType setter_side_effect_type() const; + inline void set_setter_side_effect_type(SideEffectType type); // The property attributes used when an API object template is instantiated // for the first time. Changing of this value afterwards does not affect @@ -89,13 +94,15 @@ class AccessorInfo : public Struct { inline bool HasExpectedReceiverType(); // Bit positions in |flags|. -#define ACCESSOR_INFO_FLAGS_BIT_FIELDS(V, _) \ - V(AllCanReadBit, bool, 1, _) \ - V(AllCanWriteBit, bool, 1, _) \ - V(IsSpecialDataPropertyBit, bool, 1, _) \ - V(IsSloppyBit, bool, 1, _) \ - V(ReplaceOnAccessBit, bool, 1, _) \ - V(HasNoSideEffectBit, bool, 1, _) \ +#define ACCESSOR_INFO_FLAGS_BIT_FIELDS(V, _) \ + V(AllCanReadBit, bool, 1, _) \ + V(AllCanWriteBit, bool, 1, _) \ + V(IsSpecialDataPropertyBit, bool, 1, _) \ + V(IsSloppyBit, bool, 1, _) \ + V(ReplaceOnAccessBit, bool, 1, _) \ + V(GetterSideEffectTypeBits, SideEffectType, 2, _) \ + /* We could save a bit from setter side-effect type, if necessary */ \ + V(SetterSideEffectTypeBits, SideEffectType, 2, _) \ V(InitialAttributesBits, PropertyAttributes, 3, _) DEFINE_BIT_FIELDS(ACCESSOR_INFO_FLAGS_BIT_FIELDS) diff --git a/src/profiler/heap-snapshot-generator.cc b/src/profiler/heap-snapshot-generator.cc index 27a953dd81..bfc62885cc 100644 --- a/src/profiler/heap-snapshot-generator.cc +++ b/src/profiler/heap-snapshot-generator.cc @@ -1935,8 +1935,7 @@ const char* V8HeapExplorer::GetStrongGcSubrootName(Object* object) { PUBLIC_SYMBOL_LIST(SYMBOL_NAME) WELL_KNOWN_SYMBOL_LIST(SYMBOL_NAME) #undef SYMBOL_NAME -#define ACCESSOR_NAME(accessor_name, AccessorName) \ - NAME_ENTRY(accessor_name##_accessor) +#define ACCESSOR_NAME(accessor_name, ...) NAME_ENTRY(accessor_name##_accessor) ACCESSOR_INFO_LIST(ACCESSOR_NAME) #undef ACCESSOR_NAME #undef NAME_ENTRY diff --git a/test/cctest/test-api-accessors.cc b/test/cctest/test-api-accessors.cc index 5bda0432ea..b64726037a 100644 --- a/test/cctest/test-api-accessors.cc +++ b/test/cctest/test-api-accessors.cc @@ -240,8 +240,12 @@ static void Getter(v8::Local name, static void StringGetter(v8::Local name, const v8::PropertyCallbackInfo& info) {} -static void Setter(v8::Local name, v8::Local value, - const v8::PropertyCallbackInfo& info) {} +static int set_accessor_call_count = 0; + +static void Setter(v8::Local name, v8::Local value, + const v8::PropertyCallbackInfo& info) { + set_accessor_call_count++; +} } // namespace // Re-declaration of non-configurable accessors should throw. @@ -297,6 +301,64 @@ TEST(AccessorSetHasNoSideEffect) { .ToLocalChecked() ->Int32Value(env.local()) .FromJust()); + CHECK_EQ(0, set_accessor_call_count); +} + +// Set accessors can be whitelisted as side-effect-free via SetAccessor. +TEST(SetAccessorSetSideEffectReceiverCheck1) { + LocalContext env; + v8::Isolate* isolate = env->GetIsolate(); + v8::HandleScope scope(isolate); + + v8::Local templ = v8::ObjectTemplate::New(isolate); + v8::Local obj = templ->NewInstance(env.local()).ToLocalChecked(); + CHECK(env->Global()->Set(env.local(), v8_str("obj"), obj).FromJust()); + obj->SetAccessor(env.local(), v8_str("foo"), Getter, Setter, + v8::MaybeLocal(), v8::AccessControl::DEFAULT, + v8::PropertyAttribute::None, + v8::SideEffectType::kHasNoSideEffect, + v8::SideEffectType::kHasSideEffectToReceiver) + .ToChecked(); + CHECK(v8::debug::EvaluateGlobal(isolate, v8_str("obj.foo"), true) + .ToLocalChecked() + ->Equals(env.local(), v8_str("return value")) + .FromJust()); + v8::TryCatch try_catch(isolate); + CHECK(v8::debug::EvaluateGlobal(isolate, v8_str("obj.foo = 1"), true) + .IsEmpty()); + CHECK(try_catch.HasCaught()); + CHECK_EQ(0, set_accessor_call_count); +} + +static void ConstructCallback(const v8::FunctionCallbackInfo& info) { +} + +TEST(SetAccessorSetSideEffectReceiverCheck2) { + LocalContext env; + v8::Isolate* isolate = env->GetIsolate(); + v8::HandleScope scope(isolate); + + v8::Local templ = v8::FunctionTemplate::New( + isolate, ConstructCallback, v8::Local(), + v8::Local(), 0, v8::ConstructorBehavior::kAllow, + v8::SideEffectType::kHasNoSideEffect); + templ->InstanceTemplate()->SetAccessor( + v8_str("bar"), Getter, Setter, v8::Local(), + v8::AccessControl::DEFAULT, v8::PropertyAttribute::None, + v8::Local(), + v8::SideEffectType::kHasSideEffectToReceiver, + v8::SideEffectType::kHasSideEffectToReceiver); + CHECK(env->Global() + ->Set(env.local(), v8_str("f"), + templ->GetFunction(env.local()).ToLocalChecked()) + .FromJust()); + CHECK(v8::debug::EvaluateGlobal(isolate, v8_str("new f().bar"), true) + .ToLocalChecked() + ->Equals(env.local(), v8_str("return value")) + .FromJust()); + v8::debug::EvaluateGlobal(isolate, v8_str("new f().bar = 1"), true) + .ToLocalChecked(); + CHECK_EQ(1, set_accessor_call_count); } // Accessors can be whitelisted as side-effect-free via SetNativeDataProperty. diff --git a/test/cctest/test-roots.cc b/test/cctest/test-roots.cc index f99b9df399..18cc7d2f52 100644 --- a/test/cctest/test-roots.cc +++ b/test/cctest/test-roots.cc @@ -124,7 +124,7 @@ TEST(TestAccessorInfosNotReadOnly) { Factory* factory = CcTest::i_isolate()->factory(); Heap* heap = CcTest::i_isolate()->heap(); -#define TEST_ROOT(name, AccessorName) CHECK_NOT_IN_RO_SPACE(name##_accessor) +#define TEST_ROOT(name, ...) CHECK_NOT_IN_RO_SPACE(name##_accessor) ACCESSOR_INFO_LIST(TEST_ROOT) #undef TEST_ROOT } diff --git a/test/debugger/debug/side-effect/debug-evaluate-no-side-effect-runtime-check.js b/test/debugger/debug/side-effect/debug-evaluate-no-side-effect-runtime-check.js index 7450cb2206..7a0f373be7 100644 --- a/test/debugger/debug/side-effect/debug-evaluate-no-side-effect-runtime-check.js +++ b/test/debugger/debug/side-effect/debug-evaluate-no-side-effect-runtime-check.js @@ -50,8 +50,7 @@ success([1], `return_array_use_spread([1])`); // CallAccessorSetter var array = [1,2,3]; fail(`array.length = 2`); -// TODO(7515): this one should be side effect free -fail(`[1,2,3].length = 2`); +success(2, `[1,2,3].length = 2`); // StaDataPropertyInLiteral function return_literal_with_data_property(a) { diff --git a/test/debugger/debug/side-effect/debug-evaluate-no-side-effect.js b/test/debugger/debug/side-effect/debug-evaluate-no-side-effect.js index fa14b4b862..5504cef16d 100644 --- a/test/debugger/debug/side-effect/debug-evaluate-no-side-effect.js +++ b/test/debugger/debug/side-effect/debug-evaluate-no-side-effect.js @@ -9,6 +9,7 @@ let a = 1; var object = { property : 2, get getter() { return 3; } }; +var string0 = new String("string"); var string1 = { toString() { return "x"; } }; var string2 = { toString() { print("x"); return "x"; } }; var array = [4, 5]; @@ -19,6 +20,9 @@ function set_a() { a = 2; } function get_a() { return a; } var bound = get_a.bind(0); +function return_arg0() { return return_arg0.arguments[0]; } +function return_caller_name() { return return_caller_name.caller.name; } + var global_eval = eval; function listener(event, exec_state, event_data, data) { @@ -32,6 +36,7 @@ function listener(event, exec_state, event_data, data) { assertThrows(() => exec_state.frame(0).evaluate(source, true), EvalError); } + // Simple test. success(3, "1 + 2"); // Dymanic load. @@ -62,8 +67,9 @@ function listener(event, exec_state, event_data, data) { success("set_a", "set_a.name"); success(0, "bound.length"); success("bound get_a", "bound.name"); + success(1, "return_arg0(1)"); + success("f", "(function f() { return return_caller_name() })()"); // Non-evaluated call. - success("abc", "['abc'].join('foo')"); // Constructed literals. success([1], "[1]"); success({x: 1}, "({x: 1})"); @@ -82,13 +88,25 @@ function listener(event, exec_state, event_data, data) { fail("try { set_a() } catch (e) {}"); // Test that call to set accessor fails. fail("array.length = 4"); - fail("'x'.length = 1"); fail("set_a.name = 'set_b'"); fail("set_a.length = 1"); fail("bound.name = 'bound'"); fail("bound.length = 1"); + fail("set_a.prototype = null"); // Test that call to non-whitelisted get accessor fails. fail("error.stack"); + // Call to set accessors with receiver check. + success(1, "[].length = 1"); + success(1, "'x'.length = 1"); + fail("string0.length = 1"); + success(1, "(new String('abc')).length = 1"); + success("g", "(function(){}).name = 'g'"); + success(1, "(function(){}).length = 1"); + success("g", "get_a.bind(0).name = 'g'"); + success(1, "get_a.bind(0).length = 1"); + success(null, "(function(){}).prototype = null"); + success(true, "(new Error()).stack.length > 1"); + success("a", "(new Error()).stack = 'a'"); // Eval is not allowed. fail("eval('Math.sin(1)')"); fail("eval('exception = 1')");