Skip to content

Conversation

shvaikalesh
Copy link
Member

@shvaikalesh shvaikalesh commented Feb 26, 2024

1c5838b

[JSC] ProxyObject's "apply" and "construct" traps use incorrect global object
https://bugs.webkit.org/show_bug.cgi?id=270015
<rdar://problem/123530886>

Reviewed by NOBODY (OOPS!).

Per spec, calling a ProxyObject or bound function doesn't push a new execution context onto the stack,
unlike calling an ordinary [1] or built-in [2] functions does.

With this in mind, whenever the spec references TypeError or any other intrinsic [3], the one associated
with the running execution context [4] should be used, which includes ProxyObject's [[Call]] and
[[Construct]] methods. They don't override the resolution of intrinsics.

To match the spec and other runtimes, this change replaces the global object, used by ProxyObject methods,
with the one obtained by traversing the call stack. This patch leverages existing Function.prototype.caller
implementation [5], which already implements resolution of current execution context, after tweaking it
to return correct global object even in case of DFG call frame inlining.

Call stack traversal is so fast that it doesn't regress even added microbenchmarks:

                                     ToT                      patch

proxy-apply                    81.6943+-0.1479     ?     82.2021+-0.4902        ?
proxy-apply-miss-handler       62.8556+-0.0727     ?     62.8685+-0.0845        ?

<geometric>                    71.6580+-0.0774     ?     71.8841+-0.2086        ? might be 1.0032x slower

[1]: https://tc39.es/ecma262/#sec-prepareforordinarycall (step 12)
[2]: https://tc39.es/ecma262/#sec-builtincallorconstruct (step 9)
[3]: https://tc39.es/ecma262/#sec-well-known-intrinsic-objects
[4]: https://tc39.es/ecma262/#current-realm
[5]: https://github.com/claudepache/es-legacy-function-reflection/blob/master/spec.md#get-functionprototypecaller (step 3)

* JSTests/microbenchmarks/proxy-apply-miss-handler.js: Added.
* JSTests/microbenchmarks/proxy-apply.js: Added.
* JSTests/stress/proxy-call-cross-realm.js: Added.
* JSTests/test262/expectations.yaml: Mark 24 tests as passing.
* Source/JavaScriptCore/interpreter/StackVisitor.cpp:
(JSC::CallerExecutionContextFunctor::operator() const):
(JSC::CallerExecutionContextFunctor::resultGlobalObject const):
* Source/JavaScriptCore/interpreter/StackVisitor.h:
(JSC::CallerExecutionContextFunctor::CallerExecutionContextFunctor):
(JSC::CallerExecutionContextFunctor::resultCallee const):
* Source/JavaScriptCore/runtime/FunctionPrototype.cpp:
(JSC::JSC_DEFINE_CUSTOM_GETTER):
(JSC::RetrieveCallerFunctionFunctor::RetrieveCallerFunctionFunctor): Deleted.
(JSC::RetrieveCallerFunctionFunctor::result const): Deleted.
(JSC::RetrieveCallerFunctionFunctor::operator() const): Deleted.
(JSC::retrieveCallerFunction): Deleted.
* Source/JavaScriptCore/runtime/ProxyObject.cpp:
(JSC::retrieveCallerGlobalObjectWithFallback):
(JSC::JSC_DEFINE_HOST_FUNCTION):

1c5838b

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ❌ 🧪 mac-wk2 ❌ 🧪 gtk-wk2
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 tv ❌ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim ✅ 🛠 jsc-armv7
✅ 🛠 watch ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch-sim

…l object

https://bugs.webkit.org/show_bug.cgi?id=270015
<rdar://problem/123530886>

Reviewed by NOBODY (OOPS!).

Per spec, calling a ProxyObject or bound function doesn't push a new execution context onto the stack,
unlike calling an ordinary [1] or built-in [2] functions does.

With this in mind, whenever the spec references TypeError or any other intrinsic [3], the one associated
with the running execution context [4] should be used, which includes ProxyObject's [[Call]] and
[[Construct]] methods. They don't override the resolution of intrinsics.

To match the spec and other runtimes, this change replaces the global object, used by ProxyObject methods,
with the one obtained by traversing the call stack. This patch leverages existing Function.prototype.caller
implementation [5], which already implements resolution of current execution context, after tweaking it
to return correct global object even in case of DFG call frame inlining.

Call stack traversal is so fast that it doesn't regress even added microbenchmarks:

                                     ToT                      patch

proxy-apply                    81.6943+-0.1479     ?     82.2021+-0.4902        ?
proxy-apply-miss-handler       62.8556+-0.0727     ?     62.8685+-0.0845        ?

<geometric>                    71.6580+-0.0774     ?     71.8841+-0.2086        ? might be 1.0032x slower

[1]: https://tc39.es/ecma262/#sec-prepareforordinarycall (step 12)
[2]: https://tc39.es/ecma262/#sec-builtincallorconstruct (step 9)
[3]: https://tc39.es/ecma262/#sec-well-known-intrinsic-objects
[4]: https://tc39.es/ecma262/#current-realm
[5]: https://github.com/claudepache/es-legacy-function-reflection/blob/master/spec.md#get-functionprototypecaller (step 3)

* JSTests/microbenchmarks/proxy-apply-miss-handler.js: Added.
* JSTests/microbenchmarks/proxy-apply.js: Added.
* JSTests/stress/proxy-call-cross-realm.js: Added.
* JSTests/test262/expectations.yaml: Mark 24 tests as passing.
* Source/JavaScriptCore/interpreter/StackVisitor.cpp:
(JSC::CallerExecutionContextFunctor::operator() const):
(JSC::CallerExecutionContextFunctor::resultGlobalObject const):
* Source/JavaScriptCore/interpreter/StackVisitor.h:
(JSC::CallerExecutionContextFunctor::CallerExecutionContextFunctor):
(JSC::CallerExecutionContextFunctor::resultCallee const):
* Source/JavaScriptCore/runtime/FunctionPrototype.cpp:
(JSC::JSC_DEFINE_CUSTOM_GETTER):
(JSC::RetrieveCallerFunctionFunctor::RetrieveCallerFunctionFunctor): Deleted.
(JSC::RetrieveCallerFunctionFunctor::result const): Deleted.
(JSC::RetrieveCallerFunctionFunctor::operator() const): Deleted.
(JSC::retrieveCallerFunction): Deleted.
* Source/JavaScriptCore/runtime/ProxyObject.cpp:
(JSC::retrieveCallerGlobalObjectWithFallback):
(JSC::JSC_DEFINE_HOST_FUNCTION):
@shvaikalesh shvaikalesh self-assigned this Feb 26, 2024
@shvaikalesh shvaikalesh added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Feb 26, 2024
}

if (callee) {
if (callee->inherits<JSBoundFunction>() || callee->inherits<JSRemoteFunction>() || callee->type() == ProxyObjectType)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also skip wasm and native frames?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should: if a ProxyObject gets somehow called directly from WASM / IC (I am very lacking knowledge here), without an intermediate call frame, we would end up in

JSGlobalObject* CallFrame::lexicalGlobalObjectFromNativeCallee(VM& vm) const
{
    auto* nativeCallee = callee().asNativeCallee();
    switch (nativeCallee->category()) {
    case NativeCallee::Category::Wasm: {
#if ENABLE(WEBASSEMBLY)
        return wasmInstance()->globalObject();
#else
        return nullptr;
#endif
    }
    case NativeCallee::Category::InlineCache: {
        return callerFrame()->lexicalGlobalObject(vm);
    }
    }
    return nullptr;
}

which seems sane, given ProxyObject is definitely doesn't set up it's own execution context (which we use only for fallback).

Copy link
Contributor

@justinmichaud justinmichaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me

@shvaikalesh shvaikalesh requested a review from a team as a code owner February 26, 2024 18:54
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. merging-blocked Applied to prevent a change from being merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants