Reviewers: dcarney, Michael Starzinger,
Message:
Please take a look when you next have a moment (or include someone even
better
placed to have a look at this one.)
Note:
- Spent much time trying, but I somewhat failed in reducing the Worker's
use of
object templates on its global scope object as a testcase. Hence & instead,
the
testcase "only" triggers the problematic stub code path, but doesn't mirror
the
exact steps of the upstream crashing TC.
- One could imagine having two versions of StoreCallbackProperty(), one
where
holder != receiver and one not. I considered it not worth doing, at least
now/initially.
Description:
Fix stub-invoked setter callback handling.
When invoking a setter callback for a property using
JSObject::SetPropertyWithCallback(),the callback arguments includes
a correct pair of receiver and holder objects.
Such a pair of _possibly different_ arguments (receiver, holder) must
also be supplied when invoking the same setter callback from JITed
code, when the setter is invoked through the StoreCallbackProperty
stub.
An example where this matters are the accessor properties kept on the
global scope of Worker (i.e., properties kept on the global object
itself, and not on its prototype.) Conflating the receiver with the
holder leads to general confusion when attempting to fetch out the
wrapper object.
LOG=N
R=
BUG=239669
Please review this at https://codereview.chromium.org/139263008/
SVN Base: git://github.com/v8/v8.git@bleeding_edge
Affected files (+65, -19 lines):
M src/arm/stub-cache-arm.cc
M src/ia32/stub-cache-ia32.cc
M src/mips/stub-cache-mips.cc
M src/stub-cache.cc
M src/x64/stub-cache-x64.cc
M test/cctest/test-api.cc
Index: src/arm/stub-cache-arm.cc
diff --git a/src/arm/stub-cache-arm.cc b/src/arm/stub-cache-arm.cc
index
a20702c22c19e81a620eed6912b33ef197071460..0fddbedca8dca7dbecacbc976dee6ebde5db5100
100644
--- a/src/arm/stub-cache-arm.cc
+++ b/src/arm/stub-cache-arm.cc
@@ -2518,13 +2518,14 @@ Handle<Code>
StoreStubCompiler::CompileStoreCallback(
Handle<JSObject> holder,
Handle<Name> name,
Handle<ExecutableAccessorInfo> callback) {
- HandlerFrontend(IC::CurrentTypeOf(object, isolate()),
- receiver(), holder, name);
+ Register holder_reg = HandlerFrontend(
+ IC::CurrentTypeOf(object, isolate()), receiver(), holder, name);
// Stub never generated for non-global objects that require access
checks.
ASSERT(holder->IsJSGlobalProxy() || !holder->IsAccessCheckNeeded());
__ push(receiver()); // receiver
+ __ push(holder_reg);
__ mov(ip, Operand(callback)); // callback info
__ push(ip);
__ mov(ip, Operand(name));
@@ -2533,7 +2534,7 @@ Handle<Code> StoreStubCompiler::CompileStoreCallback(
// Do tail-call to the runtime system.
ExternalReference store_callback_property =
ExternalReference(IC_Utility(IC::kStoreCallbackProperty), isolate());
- __ TailCallExternalReference(store_callback_property, 4, 1);
+ __ TailCallExternalReference(store_callback_property, 5, 1);
// Return the generated code.
return GetCode(kind(), Code::FAST, name);
Index: src/ia32/stub-cache-ia32.cc
diff --git a/src/ia32/stub-cache-ia32.cc b/src/ia32/stub-cache-ia32.cc
index
d974940d7cb822a5e3ff27644f964002b9cd44a8..800add864d459799c7dc8cf7c0bfc0b1d442c74d
100644
--- a/src/ia32/stub-cache-ia32.cc
+++ b/src/ia32/stub-cache-ia32.cc
@@ -2638,11 +2638,12 @@ Handle<Code>
StoreStubCompiler::CompileStoreCallback(
Handle<JSObject> holder,
Handle<Name> name,
Handle<ExecutableAccessorInfo> callback) {
- HandlerFrontend(IC::CurrentTypeOf(object, isolate()),
- receiver(), holder, name);
+ Register holder_reg = HandlerFrontend(
+ IC::CurrentTypeOf(object, isolate()), receiver(), holder, name);
__ pop(scratch1()); // remove the return address
__ push(receiver());
+ __ push(holder_reg);
__ Push(callback);
__ Push(name);
__ push(value());
@@ -2651,7 +2652,7 @@ Handle<Code> StoreStubCompiler::CompileStoreCallback(
// Do tail-call to the runtime system.
ExternalReference store_callback_property =
ExternalReference(IC_Utility(IC::kStoreCallbackProperty), isolate());
- __ TailCallExternalReference(store_callback_property, 4, 1);
+ __ TailCallExternalReference(store_callback_property, 5, 1);
// Return the generated code.
return GetCode(kind(), Code::FAST, name);
Index: src/mips/stub-cache-mips.cc
diff --git a/src/mips/stub-cache-mips.cc b/src/mips/stub-cache-mips.cc
index
b7948b14d63f0b666f9d8c032bd8043a0d87c350..3fb322b50348674a86c17119345596744c5b74d2
100644
--- a/src/mips/stub-cache-mips.cc
+++ b/src/mips/stub-cache-mips.cc
@@ -2514,14 +2514,15 @@ Handle<Code>
StoreStubCompiler::CompileStoreCallback(
Handle<JSObject> holder,
Handle<Name> name,
Handle<ExecutableAccessorInfo> callback) {
- HandlerFrontend(IC::CurrentTypeOf(object, isolate()),
- receiver(), holder, name);
+ Register holder_reg = HandlerFrontend(
+ IC::CurrentTypeOf(object, isolate()), receiver(), holder, name);
// Stub never generated for non-global objects that require access
// checks.
ASSERT(holder->IsJSGlobalProxy() || !holder->IsAccessCheckNeeded());
__ push(receiver()); // Receiver.
+ __ push(holder_reg);
__ li(at, Operand(callback)); // Callback info.
__ push(at);
__ li(at, Operand(name));
@@ -2530,7 +2531,7 @@ Handle<Code> StoreStubCompiler::CompileStoreCallback(
// Do tail-call to the runtime system.
ExternalReference store_callback_property =
ExternalReference(IC_Utility(IC::kStoreCallbackProperty), isolate());
- __ TailCallExternalReference(store_callback_property, 4, 1);
+ __ TailCallExternalReference(store_callback_property, 5, 1);
// Return the generated code.
return GetCode(kind(), Code::FAST, name);
Index: src/stub-cache.cc
diff --git a/src/stub-cache.cc b/src/stub-cache.cc
index
38f1960594e400960903ab4588d6c27e971c5d56..11fe01ce1a6b343594857b714c8e51c078c27f21
100644
--- a/src/stub-cache.cc
+++ b/src/stub-cache.cc
@@ -810,24 +810,25 @@ void StubCache::CollectMatchingMaps(SmallMapList*
types,
RUNTIME_FUNCTION(MaybeObject*, StoreCallbackProperty) {
- JSObject* recv = JSObject::cast(args[0]);
- ExecutableAccessorInfo* callback = ExecutableAccessorInfo::cast(args[1]);
+ JSObject* receiver = JSObject::cast(args[0]);
+ JSObject* holder = JSObject::cast(args[1]);
+ ExecutableAccessorInfo* callback = ExecutableAccessorInfo::cast(args[2]);
Address setter_address = v8::ToCData<Address>(callback->setter());
v8::AccessorSetterCallback fun =
FUNCTION_CAST<v8::AccessorSetterCallback>(setter_address);
ASSERT(fun != NULL);
- ASSERT(callback->IsCompatibleReceiver(recv));
- Handle<Name> name = args.at<Name>(2);
- Handle<Object> value = args.at<Object>(3);
+ ASSERT(callback->IsCompatibleReceiver(receiver));
+ Handle<Name> name = args.at<Name>(3);
+ Handle<Object> value = args.at<Object>(4);
HandleScope scope(isolate);
// TODO(rossberg): Support symbols in the API.
if (name->IsSymbol()) return *value;
Handle<String> str = Handle<String>::cast(name);
- LOG(isolate, ApiNamedPropertyAccess("store", recv, *name));
+ LOG(isolate, ApiNamedPropertyAccess("store", receiver, *name));
PropertyCallbackArguments
- custom_args(isolate, callback->data(), recv, recv);
+ custom_args(isolate, callback->data(), receiver, holder);
custom_args.Call(fun, v8::Utils::ToLocal(str),
v8::Utils::ToLocal(value));
RETURN_IF_SCHEDULED_EXCEPTION(isolate);
return *value;
Index: src/x64/stub-cache-x64.cc
diff --git a/src/x64/stub-cache-x64.cc b/src/x64/stub-cache-x64.cc
index
b0bdf70a4e650de6f27e7687e461b7570e09a74f..f7dc2cb3165a829d6380669a47bad84c67ea4f74
100644
--- a/src/x64/stub-cache-x64.cc
+++ b/src/x64/stub-cache-x64.cc
@@ -2542,11 +2542,12 @@ Handle<Code>
StoreStubCompiler::CompileStoreCallback(
Handle<JSObject> holder,
Handle<Name> name,
Handle<ExecutableAccessorInfo> callback) {
- HandlerFrontend(IC::CurrentTypeOf(object, isolate()),
- receiver(), holder, name);
+ Register holder_reg = HandlerFrontend(
+ IC::CurrentTypeOf(object, isolate()), receiver(), holder, name);
__ PopReturnAddressTo(scratch1());
__ push(receiver());
+ __ push(holder_reg);
__ Push(callback); // callback info
__ Push(name);
__ push(value());
@@ -2555,7 +2556,7 @@ Handle<Code> StoreStubCompiler::CompileStoreCallback(
// Do tail-call to the runtime system.
ExternalReference store_callback_property =
ExternalReference(IC_Utility(IC::kStoreCallbackProperty), isolate());
- __ TailCallExternalReference(store_callback_property, 4, 1);
+ __ TailCallExternalReference(store_callback_property, 5, 1);
// Return the generated code.
return GetCode(kind(), Code::FAST, name);
Index: test/cctest/test-api.cc
diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index
66ffc9a8b3679afe7be2ae8ac4167aeb2e489f89..d8f69ba198c8ebb6d7c466fa8be433edb4ff0d72
100644
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -21627,3 +21627,44 @@ TEST(EscapeableHandleScope) {
}
}
}
+
+
+static void SetterWhichExpectsThisAndHolderToDiffer(
+ Local<String>, Local<Value>, const v8::PropertyCallbackInfo<void>&
info) {
+ CHECK(info.Holder() != info.This());
+}
+
+
+TEST(Regress239669) {
+ // Regress618 triggers the StoreCallbackProperty() code path
+ // we're after; this is a reduced version of it.
+ const char* source = "function C1() {"
+ " this.x = 23;"
+ "};"
+ "C1.prototype = P;";
+
+ LocalContext context;
+ v8::Isolate* isolate = context->GetIsolate();
+ v8::HandleScope scope(isolate);
+
+ // Use a simple object as prototype.
+ v8::Local<v8::Object> prototype = v8::Object::New(isolate);
+ prototype->Set(v8_str("y"), v8_num(42));
+ context->Global()->Set(v8_str("P"), prototype);
+
+ CompileRun(source);
+ v8::Local<v8::Script> script = v8::Script::Compile(v8_str("new C1();"));
+ for (int i = 0; i < 2; i++) {
+ v8::Handle<v8::Object>::Cast(script->Run());
+ }
+
+ Local<ObjectTemplate> templ = ObjectTemplate::New(isolate);
+ templ->SetAccessor(v8_str("x"), 0,
SetterWhichExpectsThisAndHolderToDiffer);
+ context->Global()->Set(v8_str("P"), templ->NewInstance());
+
+ CompileRun(source);
+ script = v8::Script::Compile(v8_str("new C1();"));
+ for (int i = 0; i < 2; i++) {
+ v8::Handle<v8::Object>::Cast(script->Run());
+ }
+}
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
