From 41cc8e75f2087d61bb56930136e268e32ddb1bf0 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Mon, 25 Mar 2024 14:18:22 +0100 Subject: [PATCH] LibJS: Make PromiseJob store callback as a HeapFunction This is a speculative fix for a flake seen on CI where a JobCallback captured by a PromiseJob callback was GC'd prematurely. --- Userland/Libraries/LibJS/Forward.h | 3 +++ Userland/Libraries/LibJS/Runtime/PromiseJobs.cpp | 12 ++++++------ Userland/Libraries/LibJS/Runtime/PromiseJobs.h | 2 +- Userland/Libraries/LibJS/Runtime/VM.cpp | 13 ++++++++----- Userland/Libraries/LibJS/Runtime/VM.h | 6 +++--- Userland/Libraries/LibWeb/Bindings/MainThreadVM.cpp | 4 ++-- 6 files changed, 23 insertions(+), 17 deletions(-) diff --git a/Userland/Libraries/LibJS/Forward.h b/Userland/Libraries/LibJS/Forward.h index 3c6e51c2282..06e425b3c52 100644 --- a/Userland/Libraries/LibJS/Forward.h +++ b/Userland/Libraries/LibJS/Forward.h @@ -296,6 +296,9 @@ struct TimeZoneMethods; struct PartialDurationRecord; }; +template +class HeapFunction; + template requires(!IsLvalueReference) class ThrowCompletionOr; diff --git a/Userland/Libraries/LibJS/Runtime/PromiseJobs.cpp b/Userland/Libraries/LibJS/Runtime/PromiseJobs.cpp index 48991d50099..eaed8ca4833 100644 --- a/Userland/Libraries/LibJS/Runtime/PromiseJobs.cpp +++ b/Userland/Libraries/LibJS/Runtime/PromiseJobs.cpp @@ -90,9 +90,9 @@ PromiseJob create_promise_reaction_job(VM& vm, PromiseReaction& reaction, Value { // 1. Let job be a new Job Abstract Closure with no parameters that captures reaction and argument and performs the following steps when called: // See run_reaction_job for "the following steps". - auto job = [&vm, reaction = make_handle(&reaction), argument = make_handle(argument)] { + auto job = create_heap_function(vm.heap(), [&vm, reaction = make_handle(&reaction), argument = make_handle(argument)] { return run_reaction_job(vm, *reaction.cell(), argument.value()); - }; + }); // 2. Let handlerRealm be null. Realm* handler_realm { nullptr }; @@ -115,7 +115,7 @@ PromiseJob create_promise_reaction_job(VM& vm, PromiseReaction& reaction, Value } // 4. Return the Record { [[Job]]: job, [[Realm]]: handlerRealm }. - return { move(job), handler_realm }; + return { job, handler_realm }; } // 27.2.2.2 NewPromiseResolveThenableJob ( promiseToResolve, thenable, then ), https://tc39.es/ecma262/#sec-newpromiseresolvethenablejob @@ -164,12 +164,12 @@ PromiseJob create_promise_resolve_thenable_job(VM& vm, Promise& promise_to_resol // 1. Let job be a new Job Abstract Closure with no parameters that captures promiseToResolve, thenable, and then and performs the following steps when called: // See run_resolve_thenable_job() for "the following steps". - auto job = [&vm, promise_to_resolve = make_handle(promise_to_resolve), thenable = make_handle(thenable), then]() mutable { + auto job = create_heap_function(vm.heap(), [&vm, promise_to_resolve = make_handle(promise_to_resolve), thenable = make_handle(thenable), then]() mutable { return run_resolve_thenable_job(vm, *promise_to_resolve.cell(), thenable.value(), then); - }; + }); // 6. Return the Record { [[Job]]: job, [[Realm]]: thenRealm }. - return { move(job), then_realm }; + return { job, then_realm }; } } diff --git a/Userland/Libraries/LibJS/Runtime/PromiseJobs.h b/Userland/Libraries/LibJS/Runtime/PromiseJobs.h index 09f14873a7a..95689d7a0cc 100644 --- a/Userland/Libraries/LibJS/Runtime/PromiseJobs.h +++ b/Userland/Libraries/LibJS/Runtime/PromiseJobs.h @@ -15,7 +15,7 @@ namespace JS { struct PromiseJob { - Function()> job; + NonnullGCPtr()>> job; GCPtr realm; }; diff --git a/Userland/Libraries/LibJS/Runtime/VM.cpp b/Userland/Libraries/LibJS/Runtime/VM.cpp index 237d6b6823d..819bd57c139 100644 --- a/Userland/Libraries/LibJS/Runtime/VM.cpp +++ b/Userland/Libraries/LibJS/Runtime/VM.cpp @@ -86,8 +86,8 @@ VM::VM(OwnPtr custom_data, ErrorMessages error_messages) enqueue_finalization_registry_cleanup_job(finalization_registry); }; - host_enqueue_promise_job = [this](Function()> job, Realm* realm) { - enqueue_promise_job(move(job), realm); + host_enqueue_promise_job = [this](NonnullGCPtr()>> job, Realm* realm) { + enqueue_promise_job(job, realm); }; host_make_job_callback = [](FunctionObject& function_object) { @@ -214,6 +214,9 @@ void VM::gather_roots(HashMap& roots) gather_roots_from_execution_context_stack(m_execution_context_stack); for (auto& saved_stack : m_saved_execution_context_stacks) gather_roots_from_execution_context_stack(saved_stack); + + for (auto& job : m_promise_jobs) + roots.set(job, HeapRoot { .type = HeapRoot::Type::VM }); } ThrowCompletionOr VM::named_evaluation_if_anonymous_function(ASTNode const& expression, DeprecatedFlyString const& name) @@ -636,19 +639,19 @@ void VM::run_queued_promise_jobs() auto job = m_promise_jobs.take_first(); dbgln_if(PROMISE_DEBUG, "Calling promise job function"); - [[maybe_unused]] auto result = job(); + [[maybe_unused]] auto result = job->function()(); } } // 9.5.4 HostEnqueuePromiseJob ( job, realm ), https://tc39.es/ecma262/#sec-hostenqueuepromisejob -void VM::enqueue_promise_job(Function()> job, Realm*) +void VM::enqueue_promise_job(NonnullGCPtr()>> job, Realm*) { // An implementation of HostEnqueuePromiseJob must conform to the requirements in 9.5 as well as the following: // - FIXME: If realm is not null, each time job is invoked the implementation must perform implementation-defined steps such that execution is prepared to evaluate ECMAScript code at the time of job's invocation. // - FIXME: Let scriptOrModule be GetActiveScriptOrModule() at the time HostEnqueuePromiseJob is invoked. If realm is not null, each time job is invoked the implementation must perform implementation-defined steps // such that scriptOrModule is the active script or module at the time of job's invocation. // - Jobs must run in the same order as the HostEnqueuePromiseJob invocations that scheduled them. - m_promise_jobs.append(move(job)); + m_promise_jobs.append(job); } void VM::run_queued_finalization_registry_cleanup_jobs() diff --git a/Userland/Libraries/LibJS/Runtime/VM.h b/Userland/Libraries/LibJS/Runtime/VM.h index fce45802c77..908883767ee 100644 --- a/Userland/Libraries/LibJS/Runtime/VM.h +++ b/Userland/Libraries/LibJS/Runtime/VM.h @@ -210,7 +210,7 @@ public: CommonPropertyNames names; void run_queued_promise_jobs(); - void enqueue_promise_job(Function()> job, Realm*); + void enqueue_promise_job(NonnullGCPtr()>> job, Realm*); void run_queued_finalization_registry_cleanup_jobs(); void enqueue_finalization_registry_cleanup_job(FinalizationRegistry&); @@ -254,7 +254,7 @@ public: Function host_promise_rejection_tracker; Function(JobCallback&, Value, ReadonlySpan)> host_call_job_callback; Function host_enqueue_finalization_registry_cleanup_job; - Function()>, Realm*)> host_enqueue_promise_job; + Function()>>, Realm*)> host_enqueue_promise_job; Function(FunctionObject&)> host_make_job_callback; Function(Realm&)> host_ensure_can_compile_strings; Function(Object&)> host_ensure_can_add_private_element; @@ -300,7 +300,7 @@ private: // GlobalSymbolRegistry, https://tc39.es/ecma262/#table-globalsymbolregistry-record-fields HashMap> m_global_symbol_registry; - Vector()>> m_promise_jobs; + Vector()>>> m_promise_jobs; Vector> m_finalization_registry_cleanup_jobs; diff --git a/Userland/Libraries/LibWeb/Bindings/MainThreadVM.cpp b/Userland/Libraries/LibWeb/Bindings/MainThreadVM.cpp index 72bf89bba8e..ca3ccc326dc 100644 --- a/Userland/Libraries/LibWeb/Bindings/MainThreadVM.cpp +++ b/Userland/Libraries/LibWeb/Bindings/MainThreadVM.cpp @@ -249,7 +249,7 @@ ErrorOr initialize_main_thread_vm() }; // 8.1.5.4.3 HostEnqueuePromiseJob(job, realm), https://html.spec.whatwg.org/multipage/webappapis.html#hostenqueuepromisejob - s_main_thread_vm->host_enqueue_promise_job = [](Function()> job, JS::Realm* realm) { + s_main_thread_vm->host_enqueue_promise_job = [](JS::NonnullGCPtr()>> job, JS::Realm* realm) { // 1. If realm is not null, then let job settings be the settings object for realm. Otherwise, let job settings be null. HTML::EnvironmentSettingsObject* job_settings { nullptr }; if (realm) @@ -296,7 +296,7 @@ ErrorOr initialize_main_thread_vm() } // 3. Let result be job(). - [[maybe_unused]] auto result = job(); + auto result = job->function()(); // 4. If job settings is not null, then clean up after running script with job settings. if (job_settings) {