diff --git a/valdi/src/valdi/runtime/JavaScript/JavaScriptRuntime.cpp b/valdi/src/valdi/runtime/JavaScript/JavaScriptRuntime.cpp index 5f0b468ce..720071682 100644 --- a/valdi/src/valdi/runtime/JavaScript/JavaScriptRuntime.cpp +++ b/valdi/src/valdi/runtime/JavaScript/JavaScriptRuntime.cpp @@ -176,7 +176,7 @@ class JavaScriptRuntimeTraceProxyCallable : public JSFunction { const JSValue& callback) : _traceName(traceName), _referenceInfo(ReferenceInfoBuilder().withObject(nameFromJSFunction(jsContext, callback)).build()), - _callback(callback) {} + _callback(JSValueRef::makeRetained(jsContext, callback)) {} ~JavaScriptRuntimeTraceProxyCallable() override = default; @@ -186,13 +186,13 @@ class JavaScriptRuntimeTraceProxyCallable : public JSFunction { JSValueRef operator()(JSFunctionNativeCallContext& callContext) noexcept override { VALDI_TRACE(_traceName); - return callContext.getContext().callObjectAsFunction(_callback, callContext); + return callContext.getContext().callObjectAsFunction(_callback.get(), callContext); } private: StringBox _traceName; ReferenceInfo _referenceInfo; - JSValue _callback; + JSValueRef _callback; }; class JSRuntimeNativeObjectsManagerImpl : public snap::valdi_core::JSRuntimeNativeObjectsManager { diff --git a/valdi/test/integration/Runtime_tests.cpp b/valdi/test/integration/Runtime_tests.cpp index dafad6825..36e81b4b0 100644 --- a/valdi/test/integration/Runtime_tests.cpp +++ b/valdi/test/integration/Runtime_tests.cpp @@ -272,6 +272,40 @@ TEST_P(RuntimeFixture, exposesDefaultApiVersion) { ASSERT_EQ(0, evalResult.value().toInt()); } +TEST_P(RuntimeFixture, traceProxyRetainsCallbackAcrossGC) { + // Regression: JavaScriptRuntimeTraceProxyCallable used to hold its wrapped callback as a bare, + // non-owning JSValue. Under Hermes the callback's pooled value could be collected and its slot + // recycled while the proxy still referenced it, so invoking the proxy hit a dangling handle and + // threw "Value is not a function". The proxy must retain the callback for its own lifetime. + auto javaScriptRuntime = wrapper.runtime->getJavaScriptRuntime(); + + // makeTraceProxy is only bound when tracing is compiled in (kTracingEnabled); skip otherwise. + auto probe = javaScriptRuntime->evaluateScript( + makeShared(std::string("return typeof runtime.makeTraceProxy === 'function' ? 1 : 0;")) + ->toBytesView(), + STRING_LITERAL("eval.js")); + ASSERT_TRUE(probe) << probe.description(); + if (probe.value().toInt() != 1) { + GTEST_SKIP() << "tracing disabled in this build (runtime.makeTraceProxy not bound)"; + } + + // The closure has no JS reference other than the trace proxy itself. + std::string setupBody = "globalThis.__traceProxy = runtime.makeTraceProxy('regressionTag', () => 42); return 0;"; + auto setupResult = + javaScriptRuntime->evaluateScript(makeShared(setupBody)->toBytesView(), STRING_LITERAL("eval.js")); + ASSERT_TRUE(setupResult) << setupResult.description(); + + // Force GC: pre-fix the unrooted closure is collected and its slot recycled. + javaScriptRuntime->dispatchSynchronouslyOnJsThread([](auto& jsEntry) { jsEntry.jsContext.garbageCollect(); }); + + // Invoke through the proxy: it must still resolve to the original callback. + std::string callBody = "return __traceProxy();"; + auto callResult = + javaScriptRuntime->evaluateScript(makeShared(callBody)->toBytesView(), STRING_LITERAL("eval.js")); + ASSERT_TRUE(callResult) << callResult.description(); + ASSERT_EQ(42, callResult.value().toInt()); +} + TEST_P(RuntimeFixture, canLoadSimpleViewTree) { auto tree = wrapper.createViewNodeTreeAndContext("test", "BasicViewTree");