From 251ae221566c38d3fc71fd1a19b26685f875e194 Mon Sep 17 00:00:00 2001 From: "verwaest@chromium.org" Date: Mon, 7 Jul 2014 11:00:44 +0000 Subject: [PATCH] Treat ExecutableAccessorInfo as regular data properties. BUG= R=dcarney@chromium.org, mvstanton@chromium.org Review URL: https://codereview.chromium.org/368783006 git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@22236 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/accessors.cc | 19 +------ src/ic.cc | 4 +- src/objects.cc | 19 +++++++ test/cctest/test-api.cc | 86 +++++++++++++++++------------- test/mjsunit/es7/object-observe.js | 4 +- 5 files changed, 75 insertions(+), 57 deletions(-) diff --git a/src/accessors.cc b/src/accessors.cc index 6ade56aaee..eb7e372851 100644 --- a/src/accessors.cc +++ b/src/accessors.cc @@ -191,16 +191,7 @@ void Accessors::ArrayLengthSetter( Handle object = Handle::cast( Utils::OpenHandle(*info.This())); Handle value = Utils::OpenHandle(*val); - // This means one of the object's prototypes is a JSArray and the - // object does not have a 'length' property. Calling SetProperty - // causes an infinite loop. - if (!object->IsJSArray()) { - MaybeHandle maybe_result = - JSObject::SetOwnPropertyIgnoreAttributes( - object, isolate->factory()->length_string(), value, NONE); - maybe_result.Check(); - return; - } + ASSERT(object->IsJSArray()); value = FlattenNumber(isolate, value); @@ -871,13 +862,7 @@ static Handle SetFunctionPrototype(Isolate* isolate, function = Handle(function_raw, isolate); } - if (!function->should_have_prototype()) { - // Since we hit this accessor, object will have no prototype property. - MaybeHandle maybe_result = - JSObject::SetOwnPropertyIgnoreAttributes( - receiver, isolate->factory()->prototype_string(), value, NONE); - return maybe_result.ToHandleChecked(); - } + ASSERT(function->should_have_prototype()); Handle old_value; bool is_observed = *function == *receiver && function->map()->is_observed(); diff --git a/src/ic.cc b/src/ic.cc index 60fcdc8a88..6a92719cc2 100644 --- a/src/ic.cc +++ b/src/ic.cc @@ -1206,7 +1206,9 @@ static bool LookupForWrite(Handle receiver, if (lookup->IsReadOnly() || !lookup->IsCacheable()) return false; if (lookup->holder() == *receiver) return lookup->CanHoldValue(value); - if (lookup->IsPropertyCallbacks()) return true; + if (lookup->IsPropertyCallbacks() && + !lookup->GetCallbackObject()->IsExecutableAccessorInfo()) + return true; // JSGlobalProxy either stores on the global object in the prototype, or // goes into the runtime if access checks are needed, so this is always // safe. diff --git a/src/objects.cc b/src/objects.cc index c84176b644..ab34acf880 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -3096,6 +3096,25 @@ MaybeHandle JSObject::SetPropertyViaPrototypes( *done = true; if (!result.IsReadOnly()) { Handle callback_object(result.GetCallbackObject(), isolate); + // Only store via executable access info setters if the holder is the + // receiver or on its hidden prototype chain. + if (callback_object->IsExecutableAccessorInfo()) { + Handle current = object; + while (*current != result.holder()) { + // There is a callbacks holder, so we are guaranteed that all + // objects in between are JSObjects. + Handle prototype( + JSObject::cast(current->GetPrototype())); + if (!current->IsJSGlobalProxy() && + !prototype->map()->is_hidden_prototype()) { + *done = false; + break; + } + current = prototype; + } + // Break out of the switch after breaking out of the loop above. + if (*current != result.holder()) break; + } return SetPropertyWithCallback(object, name, value, handle(result.holder()), callback_object, strict_mode); diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index a3ed5c578f..1af7d7badf 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -1924,7 +1924,7 @@ void AddAccessor(Handle templ, Handle name, v8::AccessorGetterCallback getter, v8::AccessorSetterCallback setter) { - templ->PrototypeTemplate()->SetAccessor(name, getter, setter); + templ->InstanceTemplate()->SetAccessor(name, getter, setter); } void AddInterceptor(Handle templ, @@ -1937,6 +1937,7 @@ void AddInterceptor(Handle templ, THREADED_TEST(EmptyInterceptorDoesNotShadowAccessors) { v8::HandleScope scope(CcTest::isolate()); Handle parent = FunctionTemplate::New(CcTest::isolate()); + parent->SetHiddenPrototype(true); Handle child = FunctionTemplate::New(CcTest::isolate()); child->Inherit(parent); AddAccessor(parent, v8_str("age"), @@ -1946,7 +1947,7 @@ THREADED_TEST(EmptyInterceptorDoesNotShadowAccessors) { env->Global()->Set(v8_str("Child"), child->GetFunction()); CompileRun("var child = new Child;" "child.age = 10;"); - ExpectBoolean("child.hasOwnProperty('age')", false); + ExpectBoolean("child.hasOwnProperty('age')", true); ExpectInt32("child.age", 10); ExpectInt32("child.accessor_age", 10); } @@ -9985,10 +9986,13 @@ THREADED_TEST(ShadowObject) { LocalContext context(NULL, global_template); Local t = v8::FunctionTemplate::New(isolate); - t->InstanceTemplate()->SetNamedPropertyHandler(ShadowNamedGet); - t->InstanceTemplate()->SetIndexedPropertyHandler(ShadowIndexedGet); - Local proto = t->PrototypeTemplate(); + t->SetHiddenPrototype(true); + Local pt = v8::FunctionTemplate::New(isolate); + t->Inherit(pt); + Local proto = pt->PrototypeTemplate(); Local instance = t->InstanceTemplate(); + instance->SetNamedPropertyHandler(ShadowNamedGet); + instance->SetIndexedPropertyHandler(ShadowIndexedGet); proto->Set(v8_str("f"), v8::FunctionTemplate::New(isolate, @@ -18535,12 +18539,12 @@ void FooSetInterceptor(Local name, TEST(SetterOnConstructorPrototype) { v8::Isolate* isolate = CcTest::isolate(); v8::HandleScope scope(isolate); - Local templ = ObjectTemplate::New(isolate); - templ->SetAccessor(v8_str("x"), - GetterWhichReturns42, - SetterWhichSetsYOnThisTo23); + Local templ = FunctionTemplate::New(isolate); + templ->SetHiddenPrototype(true); + templ->InstanceTemplate()->SetAccessor(v8_str("x"), GetterWhichReturns42, + SetterWhichSetsYOnThisTo23); LocalContext context; - context->Global()->Set(v8_str("P"), templ->NewInstance()); + context->Global()->Set(v8_str("P"), templ->InstanceTemplate()->NewInstance()); CompileRun("function C1() {" " this.x = 23;" "};" @@ -18562,8 +18566,7 @@ TEST(SetterOnConstructorPrototype) { script = v8_compile("new C2();"); for (int i = 0; i < 10; i++) { v8::Handle c2 = v8::Handle::Cast(script->Run()); - CHECK_EQ(42, c2->Get(v8_str("x"))->Int32Value()); - CHECK_EQ(23, c2->Get(v8_str("y"))->Int32Value()); + CHECK_EQ(23, c2->Get(v8_str("x"))->Int32Value()); } } @@ -18649,11 +18652,11 @@ TEST(Regress618) { } // Use an API object with accessors as prototype. - Local templ = ObjectTemplate::New(isolate); - templ->SetAccessor(v8_str("x"), - GetterWhichReturns42, - SetterWhichSetsYOnThisTo23); - context->Global()->Set(v8_str("P"), templ->NewInstance()); + Local templ = FunctionTemplate::New(isolate); + templ->SetHiddenPrototype(true); + templ->InstanceTemplate()->SetAccessor(v8_str("x"), GetterWhichReturns42, + SetterWhichSetsYOnThisTo23); + context->Global()->Set(v8_str("P"), templ->InstanceTemplate()->NewInstance()); // This compile will get the code from the compilation cache. CompileRun(source); @@ -21067,16 +21070,16 @@ static void InstanceCheckedSetter(Local name, } -static void CheckInstanceCheckedResult(int getters, - int setters, - bool expects_callbacks, +static void CheckInstanceCheckedResult(int getters, int setters, + bool expects_callbacks, bool is_setter, TryCatch* try_catch) { if (expects_callbacks) { CHECK(!try_catch->HasCaught()); CHECK_EQ(getters, instance_checked_getter_count); CHECK_EQ(setters, instance_checked_setter_count); } else { - CHECK(try_catch->HasCaught()); + CHECK((is_setter && !try_catch->HasCaught()) || + (!is_setter && try_catch->HasCaught())); CHECK_EQ(0, instance_checked_getter_count); CHECK_EQ(0, instance_checked_setter_count); } @@ -21091,33 +21094,38 @@ static void CheckInstanceCheckedAccessors(bool expects_callbacks) { // Test path through generic runtime code. CompileRun("obj.foo"); - CheckInstanceCheckedResult(1, 0, expects_callbacks, &try_catch); + CheckInstanceCheckedResult(1, 0, expects_callbacks, false, &try_catch); CompileRun("obj.foo = 23"); - CheckInstanceCheckedResult(1, 1, expects_callbacks, &try_catch); + CheckInstanceCheckedResult(1, 1, expects_callbacks, true, &try_catch); + if (!expects_callbacks) CompileRun("delete obj.foo"); // Test path through generated LoadIC and StoredIC. CompileRun("function test_get(o) { o.foo; }" "test_get(obj);"); - CheckInstanceCheckedResult(2, 1, expects_callbacks, &try_catch); + CheckInstanceCheckedResult(2, 1, expects_callbacks, false, &try_catch); CompileRun("test_get(obj);"); - CheckInstanceCheckedResult(3, 1, expects_callbacks, &try_catch); + CheckInstanceCheckedResult(3, 1, expects_callbacks, false, &try_catch); CompileRun("test_get(obj);"); - CheckInstanceCheckedResult(4, 1, expects_callbacks, &try_catch); + CheckInstanceCheckedResult(4, 1, expects_callbacks, false, &try_catch); CompileRun("function test_set(o) { o.foo = 23; }" "test_set(obj);"); - CheckInstanceCheckedResult(4, 2, expects_callbacks, &try_catch); + CheckInstanceCheckedResult(4, 2, expects_callbacks, true, &try_catch); + if (!expects_callbacks) CompileRun("delete obj.foo"); CompileRun("test_set(obj);"); - CheckInstanceCheckedResult(4, 3, expects_callbacks, &try_catch); + CheckInstanceCheckedResult(4, 3, expects_callbacks, true, &try_catch); + if (!expects_callbacks) CompileRun("delete obj.foo"); CompileRun("test_set(obj);"); - CheckInstanceCheckedResult(4, 4, expects_callbacks, &try_catch); + CheckInstanceCheckedResult(4, 4, expects_callbacks, true, &try_catch); + if (!expects_callbacks) CompileRun("delete obj.foo"); // Test path through optimized code. CompileRun("%OptimizeFunctionOnNextCall(test_get);" "test_get(obj);"); - CheckInstanceCheckedResult(5, 4, expects_callbacks, &try_catch); + CheckInstanceCheckedResult(5, 4, expects_callbacks, false, &try_catch); CompileRun("%OptimizeFunctionOnNextCall(test_set);" "test_set(obj);"); - CheckInstanceCheckedResult(5, 5, expects_callbacks, &try_catch); + if (!expects_callbacks) CompileRun("delete obj.foo"); + CheckInstanceCheckedResult(5, 5, expects_callbacks, true, &try_catch); // Cleanup so that closures start out fresh in next check. CompileRun("%DeoptimizeFunction(test_get);" @@ -21189,14 +21197,16 @@ THREADED_TEST(InstanceCheckOnPrototypeAccessor) { LocalContext context; v8::HandleScope scope(context->GetIsolate()); + Local proto_templ = + FunctionTemplate::New(context->GetIsolate()); + proto_templ->SetHiddenPrototype(true); + Local proto = proto_templ->InstanceTemplate(); + proto->SetAccessor( + v8_str("foo"), InstanceCheckedGetter, InstanceCheckedSetter, + Handle(), v8::DEFAULT, v8::None, + v8::AccessorSignature::New(context->GetIsolate(), proto_templ)); Local templ = FunctionTemplate::New(context->GetIsolate()); - Local proto = templ->PrototypeTemplate(); - proto->SetAccessor(v8_str("foo"), - InstanceCheckedGetter, InstanceCheckedSetter, - Handle(), - v8::DEFAULT, - v8::None, - v8::AccessorSignature::New(context->GetIsolate(), templ)); + templ->Inherit(proto_templ); context->Global()->Set(v8_str("f"), templ->GetFunction()); printf("Testing positive ...\n"); diff --git a/test/mjsunit/es7/object-observe.js b/test/mjsunit/es7/object-observe.js index 7bb579f0c1..27aa73d02b 100644 --- a/test/mjsunit/es7/object-observe.js +++ b/test/mjsunit/es7/object-observe.js @@ -1685,7 +1685,9 @@ var obj = { __proto__: fun }; Object.observe(obj, observer.callback); obj.prototype = 7; Object.deliverChangeRecords(observer.callback); -observer.assertNotCalled(); +observer.assertCallbackRecords([ + { object: obj, name: 'prototype', type: 'add'}, +]); // Check that changes in observation status are detected in all IC states and