From 16ae2551aedb81f3f1122fedea7ca22e490e4b49 Mon Sep 17 00:00:00 2001 From: "ulan@chromium.org" Date: Mon, 26 Sep 2011 14:54:57 +0000 Subject: [PATCH] Search prototypes for accessor setters if interceptor returns empty value. Extract the part of SetPropertyForResult that searches the prototype chain for accessor setters into a separate function SetPropertyInPrototypes. Call this function in SetPropertyPostInterceptor. This should fix both optimized and unoptimized cases because the cache stub for storing with interceptor calls the runtime system. BUG=v8:1636 TEST=cctest/test-api.cc/EmptyInterceptorDoesNotShadowAccessors Review URL: http://codereview.chromium.org/7991007 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@9431 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/objects.cc | 98 +++++++++++++++++------- src/objects.h | 12 +++ test/cctest/test-api.cc | 196 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 278 insertions(+), 28 deletions(-) diff --git a/src/objects.cc b/src/objects.cc index 3d31407..46d5264 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -1664,6 +1664,14 @@ MaybeObject* JSObject::SetPropertyPostInterceptor( // found. Use set property to handle all these cases. return SetProperty(&result, name, value, attributes, strict_mode); } + bool found = false; + MaybeObject* result_object; + result_object = SetPropertyWithCallbackSetterInPrototypes(name, + value, + attributes, + &found, + strict_mode); + if (found) return result_object; // Add a new real property. return AddProperty(name, value, attributes, strict_mode); } @@ -1991,6 +1999,60 @@ MaybeObject* JSObject::SetElementWithCallbackSetterInPrototypes( return heap->the_hole_value(); } +MaybeObject* JSObject::SetPropertyWithCallbackSetterInPrototypes( + String* name, + Object* value, + PropertyAttributes attributes, + bool* found, + StrictModeFlag strict_mode) { + LookupResult result; + LookupCallbackSetterInPrototypes(name, &result); + Heap* heap = GetHeap(); + if (result.IsFound()) { + *found = true; + if (result.type() == CALLBACKS) { + return SetPropertyWithCallback(result.GetCallbackObject(), + name, + value, + result.holder(), + strict_mode); + } else if (result.type() == HANDLER) { + // We could not find a local property so let's check whether there is an + // accessor that wants to handle the property. + LookupResult accessor_result; + LookupCallbackSetterInPrototypes(name, &accessor_result); + if (accessor_result.IsFound()) { + if (accessor_result.type() == CALLBACKS) { + return SetPropertyWithCallback(accessor_result.GetCallbackObject(), + name, + value, + accessor_result.holder(), + strict_mode); + } else if (accessor_result.type() == HANDLER) { + // There is a proxy in the prototype chain. Invoke its + // getOwnPropertyDescriptor trap. + bool found = false; + // SetPropertyWithHandlerIfDefiningSetter can cause GC, + // make sure to use the handlified references after calling + // the function. + Handle self(this); + Handle hname(name); + Handle hvalue(value); + MaybeObject* result = + accessor_result.proxy()->SetPropertyWithHandlerIfDefiningSetter( + name, value, attributes, strict_mode, &found); + if (found) return result; + // The proxy does not define the property as an accessor. + // Consequently, it has no effect on setting the receiver. + return self->AddProperty(*hname, *hvalue, attributes, strict_mode); + } + } + } + } + *found = false; + return heap->the_hole_value(); +} + void JSObject::LookupInDescriptor(String* name, LookupResult* result) { DescriptorArray* descriptors = map()->instance_descriptors(); @@ -2623,34 +2685,14 @@ MaybeObject* JSObject::SetPropertyForResult(LookupResult* result, } if (!result->IsProperty() && !IsJSContextExtensionObject()) { - // We could not find a local property so let's check whether there is an - // accessor that wants to handle the property. - LookupResult accessor_result; - LookupCallbackSetterInPrototypes(name, &accessor_result); - if (accessor_result.IsFound()) { - if (accessor_result.type() == CALLBACKS) { - return SetPropertyWithCallback(accessor_result.GetCallbackObject(), - name, - value, - accessor_result.holder(), - strict_mode); - } else if (accessor_result.type() == HANDLER) { - // There is a proxy in the prototype chain. Invoke its - // getOwnPropertyDescriptor trap. - bool found = false; - Handle self(this); - Handle hname(name); - Handle hvalue(value); - MaybeObject* result = - accessor_result.proxy()->SetPropertyWithHandlerIfDefiningSetter( - name, value, attributes, strict_mode, &found); - if (found) return result; - // The proxy does not define the property as an accessor. - // Consequently, it has no effect on setting the receiver. - // Make sure to use the handlified references at this point! - return self->AddProperty(*hname, *hvalue, attributes, strict_mode); - } - } + bool found = false; + MaybeObject* result_object; + result_object = SetPropertyWithCallbackSetterInPrototypes(name, + value, + attributes, + &found, + strict_mode); + if (found) return result_object; } // At this point, no GC should have happened, as this would invalidate diff --git a/src/objects.h b/src/objects.h index 019bd46..ba8eb87 100644 --- a/src/objects.h +++ b/src/objects.h @@ -1987,6 +1987,18 @@ class JSObject: public JSReceiver { StrictModeFlag strict_mode, bool check_prototype); + // Searches the prototype chain for a callback setter and sets the property + // with the setter if it finds one. The '*found' flag indicates whether + // a setter was found or not. + // This function can cause GC and can return a failure result with + // '*found==true'. + MUST_USE_RESULT MaybeObject* SetPropertyWithCallbackSetterInPrototypes( + String* name, + Object* value, + PropertyAttributes attributes, + bool* found, + StrictModeFlag strict_mode); + MUST_USE_RESULT MaybeObject* DeletePropertyPostInterceptor(String* name, DeleteMode mode); MUST_USE_RESULT MaybeObject* DeletePropertyWithInterceptor(String* name); diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index 23ceee2..17fd226 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -81,6 +81,11 @@ static void ExpectString(const char* code, const char* expected) { CHECK_EQ(expected, *ascii); } +static void ExpectInt32(const char* code, int expected) { + Local result = CompileRun(code); + CHECK(result->IsInt32()); + CHECK_EQ(expected, result->Int32Value()); +} static void ExpectBoolean(const char* code, bool expected) { Local result = CompileRun(code); @@ -1297,6 +1302,197 @@ static v8::Handle EchoNamedProperty(Local name, return name; } +// Helper functions for Interceptor/Accessor interaction tests + +Handle SimpleAccessorGetter(Local name, + const AccessorInfo& info) { + Handle self = info.This(); + return self->Get(String::Concat(v8_str("accessor_"), name)); +} + +void SimpleAccessorSetter(Local name, Local value, + const AccessorInfo& info) { + Handle self = info.This(); + self->Set(String::Concat(v8_str("accessor_"), name), value); +} + +Handle EmptyInterceptorGetter(Local name, + const AccessorInfo& info) { + return Handle(); +} + +Handle EmptyInterceptorSetter(Local name, + Local value, + const AccessorInfo& info) { + return Handle(); +} + +Handle InterceptorGetter(Local name, + const AccessorInfo& info) { + // Intercept names that start with 'interceptor_'. + String::AsciiValue ascii(name); + char* name_str = *ascii; + char prefix[] = "interceptor_"; + int i; + for (i = 0; name_str[i] && prefix[i]; ++i) { + if (name_str[i] != prefix[i]) return Handle(); + } + Handle self = info.This(); + return self->GetHiddenValue(v8_str(name_str + i)); +} + +Handle InterceptorSetter(Local name, + Local value, + const AccessorInfo& info) { + // Intercept accesses that set certain integer values. + if (value->IsInt32() && value->Int32Value() < 10000) { + Handle self = info.This(); + self->SetHiddenValue(name, value); + return value; + } + return Handle(); +} + +void AddAccessor(Handle templ, + Handle name, + v8::AccessorGetter getter, + v8::AccessorSetter setter) { + templ->PrototypeTemplate()->SetAccessor(name, getter, setter); +} + +void AddInterceptor(Handle templ, + v8::NamedPropertyGetter getter, + v8::NamedPropertySetter setter) { + templ->InstanceTemplate()->SetNamedPropertyHandler(getter, setter); +} + +THREADED_TEST(EmptyInterceptorDoesNotShadowAccessors) { + v8::HandleScope scope; + Handle parent = FunctionTemplate::New(); + Handle child = FunctionTemplate::New(); + child->Inherit(parent); + AddAccessor(parent, v8_str("age"), + SimpleAccessorGetter, SimpleAccessorSetter); + AddInterceptor(child, EmptyInterceptorGetter, EmptyInterceptorSetter); + LocalContext env; + env->Global()->Set(v8_str("Child"), child->GetFunction()); + CompileRun("var child = new Child;" + "child.age = 10;"); + ExpectBoolean("child.hasOwnProperty('age')", false); + ExpectInt32("child.age", 10); + ExpectInt32("child.accessor_age", 10); +} + +THREADED_TEST(EmptyInterceptorDoesNotShadowJSAccessors) { + v8::HandleScope scope; + Handle parent = FunctionTemplate::New(); + Handle child = FunctionTemplate::New(); + child->Inherit(parent); + AddInterceptor(child, EmptyInterceptorGetter, EmptyInterceptorSetter); + LocalContext env; + env->Global()->Set(v8_str("Child"), child->GetFunction()); + CompileRun("var child = new Child;" + "var parent = child.__proto__;" + "Object.defineProperty(parent, 'age', " + " {get: function(){ return this.accessor_age; }, " + " set: function(v){ this.accessor_age = v; }, " + " enumerable: true, configurable: true});" + "child.age = 10;"); + ExpectBoolean("child.hasOwnProperty('age')", false); + ExpectInt32("child.age", 10); + ExpectInt32("child.accessor_age", 10); +} + +THREADED_TEST(EmptyInterceptorDoesNotAffectJSProperties) { + v8::HandleScope scope; + Handle parent = FunctionTemplate::New(); + Handle child = FunctionTemplate::New(); + child->Inherit(parent); + AddInterceptor(child, EmptyInterceptorGetter, EmptyInterceptorSetter); + LocalContext env; + env->Global()->Set(v8_str("Child"), child->GetFunction()); + CompileRun("var child = new Child;" + "var parent = child.__proto__;" + "parent.name = 'Alice';"); + ExpectBoolean("child.hasOwnProperty('name')", false); + ExpectString("child.name", "Alice"); + CompileRun("child.name = 'Bob';"); + ExpectString("child.name", "Bob"); + ExpectBoolean("child.hasOwnProperty('name')", true); + ExpectString("parent.name", "Alice"); +} + +THREADED_TEST(SwitchFromInterceptorToAccessor) { + v8::HandleScope scope; + Handle parent = FunctionTemplate::New(); + Handle child = FunctionTemplate::New(); + child->Inherit(parent); + AddAccessor(parent, v8_str("age"), + SimpleAccessorGetter, SimpleAccessorSetter); + AddInterceptor(child, InterceptorGetter, InterceptorSetter); + LocalContext env; + env->Global()->Set(v8_str("Child"), child->GetFunction()); + CompileRun("var child = new Child;" + "function setAge(i){ child.age = i; };" + "for(var i = 0; i <= 10000; i++) setAge(i);"); + // All i < 10000 go to the interceptor. + ExpectInt32("child.interceptor_age", 9999); + // The last i goes to the accessor. + ExpectInt32("child.accessor_age", 10000); +} + +THREADED_TEST(SwitchFromAccessorToInterceptor) { + v8::HandleScope scope; + Handle parent = FunctionTemplate::New(); + Handle child = FunctionTemplate::New(); + child->Inherit(parent); + AddAccessor(parent, v8_str("age"), + SimpleAccessorGetter, SimpleAccessorSetter); + AddInterceptor(child, InterceptorGetter, InterceptorSetter); + LocalContext env; + env->Global()->Set(v8_str("Child"), child->GetFunction()); + CompileRun("var child = new Child;" + "function setAge(i){ child.age = i; };" + "for(var i = 20000; i >= 9999; i--) setAge(i);"); + // All i >= 10000 go to the accessor. + ExpectInt32("child.accessor_age", 10000); + // The last i goes to the interceptor. + ExpectInt32("child.interceptor_age", 9999); +} + +THREADED_TEST(SwitchFromInterceptorToProperty) { + v8::HandleScope scope; + Handle parent = FunctionTemplate::New(); + Handle child = FunctionTemplate::New(); + child->Inherit(parent); + AddInterceptor(child, InterceptorGetter, InterceptorSetter); + LocalContext env; + env->Global()->Set(v8_str("Child"), child->GetFunction()); + CompileRun("var child = new Child;" + "function setAge(i){ child.age = i; };" + "for(var i = 0; i <= 10000; i++) setAge(i);"); + // All i < 10000 go to the interceptor. + ExpectInt32("child.interceptor_age", 9999); + // The last i goes to child's own property. + ExpectInt32("child.age", 10000); +} + +THREADED_TEST(SwitchFromPropertyToInterceptor) { + v8::HandleScope scope; + Handle parent = FunctionTemplate::New(); + Handle child = FunctionTemplate::New(); + child->Inherit(parent); + AddInterceptor(child, InterceptorGetter, InterceptorSetter); + LocalContext env; + env->Global()->Set(v8_str("Child"), child->GetFunction()); + CompileRun("var child = new Child;" + "function setAge(i){ child.age = i; };" + "for(var i = 20000; i >= 9999; i--) setAge(i);"); + // All i >= 10000 go to child's own property. + ExpectInt32("child.age", 10000); + // The last i goes to the interceptor. + ExpectInt32("child.interceptor_age", 9999); +} THREADED_TEST(NamedPropertyHandlerGetter) { echo_named_call_count = 0; -- 2.7.4