From a3b4c2a30f8e6852523de2ecc40c40459e4c237f Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Tue, 12 Mar 2024 03:49:13 +0100 Subject: [PATCH] LibJS+LibWeb: Change JobCallback to be GC-allocated Fixes leak caused by mutual dependency when JS::Handle is owned by GC-allocated PromiseReaction. --- Userland/Libraries/LibJS/Forward.h | 2 +- .../LibJS/Runtime/FinalizationRegistry.cpp | 11 +++---- .../LibJS/Runtime/FinalizationRegistry.h | 10 +++---- .../Runtime/FinalizationRegistryPrototype.cpp | 2 +- .../Libraries/LibJS/Runtime/JobCallback.cpp | 22 ++++++++++---- .../Libraries/LibJS/Runtime/JobCallback.h | 30 +++++++++++++++---- Userland/Libraries/LibJS/Runtime/Promise.cpp | 4 +-- .../Libraries/LibJS/Runtime/PromiseJobs.cpp | 20 ++++++------- .../Libraries/LibJS/Runtime/PromiseJobs.h | 2 +- .../LibJS/Runtime/PromiseReaction.cpp | 5 ++-- .../Libraries/LibJS/Runtime/PromiseReaction.h | 10 +++---- Userland/Libraries/LibJS/Runtime/VM.h | 2 +- .../LibWeb/Bindings/MainThreadVM.cpp | 10 +++---- 13 files changed, 81 insertions(+), 49 deletions(-) diff --git a/Userland/Libraries/LibJS/Forward.h b/Userland/Libraries/LibJS/Forward.h index df6d0ec9b04..3c6e51c2282 100644 --- a/Userland/Libraries/LibJS/Forward.h +++ b/Userland/Libraries/LibJS/Forward.h @@ -222,7 +222,7 @@ class WeakContainer; class WrappedFunction; enum class DeclarationKind; struct AlreadyResolved; -struct JobCallback; +class JobCallback; struct ModuleRequest; struct ModuleWithSpecifier; diff --git a/Userland/Libraries/LibJS/Runtime/FinalizationRegistry.cpp b/Userland/Libraries/LibJS/Runtime/FinalizationRegistry.cpp index fb172330172..e3b2b02624a 100644 --- a/Userland/Libraries/LibJS/Runtime/FinalizationRegistry.cpp +++ b/Userland/Libraries/LibJS/Runtime/FinalizationRegistry.cpp @@ -11,11 +11,11 @@ namespace JS { JS_DEFINE_ALLOCATOR(FinalizationRegistry); -FinalizationRegistry::FinalizationRegistry(Realm& realm, JobCallback cleanup_callback, Object& prototype) +FinalizationRegistry::FinalizationRegistry(Realm& realm, NonnullGCPtr cleanup_callback, Object& prototype) : Object(ConstructWithPrototypeTag::Tag, prototype) , WeakContainer(heap()) , m_realm(realm) - , m_cleanup_callback(move(cleanup_callback)) + , m_cleanup_callback(cleanup_callback) { } @@ -62,7 +62,7 @@ void FinalizationRegistry::remove_dead_cells(Badge) } // 9.13 CleanupFinalizationRegistry ( finalizationRegistry ), https://tc39.es/ecma262/#sec-cleanup-finalization-registry -ThrowCompletionOr FinalizationRegistry::cleanup(Optional callback) +ThrowCompletionOr FinalizationRegistry::cleanup(JS::GCPtr callback) { auto& vm = this->vm(); @@ -70,7 +70,7 @@ ThrowCompletionOr FinalizationRegistry::cleanup(Optional call // Note: Ensured by type. // 2. Let callback be finalizationRegistry.[[CleanupCallback]]. - auto& cleanup_callback = callback.has_value() ? callback.value() : m_cleanup_callback; + auto cleanup_callback = callback ? callback : m_cleanup_callback; // 3. While finalizationRegistry.[[Cells]] contains a Record cell such that cell.[[WeakRefTarget]] is empty, an implementation may perform the following steps: for (auto it = m_records.begin(); it != m_records.end(); ++it) { @@ -84,7 +84,7 @@ ThrowCompletionOr FinalizationRegistry::cleanup(Optional call it.remove(m_records); // c. Perform ? HostCallJobCallback(callback, undefined, « cell.[[HeldValue]] »). - TRY(vm.host_call_job_callback(cleanup_callback, js_undefined(), move(arguments))); + TRY(vm.host_call_job_callback(*cleanup_callback, js_undefined(), move(arguments))); } // 4. Return unused. @@ -95,6 +95,7 @@ void FinalizationRegistry::visit_edges(Cell::Visitor& visitor) { Base::visit_edges(visitor); visitor.visit(m_realm); + visitor.visit(m_cleanup_callback); for (auto& record : m_records) { visitor.visit(record.held_value); visitor.visit(record.unregister_token); diff --git a/Userland/Libraries/LibJS/Runtime/FinalizationRegistry.h b/Userland/Libraries/LibJS/Runtime/FinalizationRegistry.h index fa027027392..e4f548ead18 100644 --- a/Userland/Libraries/LibJS/Runtime/FinalizationRegistry.h +++ b/Userland/Libraries/LibJS/Runtime/FinalizationRegistry.h @@ -28,23 +28,23 @@ public: void add_finalization_record(Cell& target, Value held_value, Cell* unregister_token); bool remove_by_token(Cell& unregister_token); - ThrowCompletionOr cleanup(Optional = {}); + ThrowCompletionOr cleanup(GCPtr = {}); virtual void remove_dead_cells(Badge) override; Realm& realm() { return *m_realm; } Realm const& realm() const { return *m_realm; } - JobCallback& cleanup_callback() { return m_cleanup_callback; } - JobCallback const& cleanup_callback() const { return m_cleanup_callback; } + JobCallback& cleanup_callback() { return *m_cleanup_callback; } + JobCallback const& cleanup_callback() const { return *m_cleanup_callback; } private: - FinalizationRegistry(Realm&, JobCallback, Object& prototype); + FinalizationRegistry(Realm&, NonnullGCPtr, Object& prototype); virtual void visit_edges(Visitor& visitor) override; NonnullGCPtr m_realm; - JobCallback m_cleanup_callback; + NonnullGCPtr m_cleanup_callback; struct FinalizationRecord { GCPtr target; diff --git a/Userland/Libraries/LibJS/Runtime/FinalizationRegistryPrototype.cpp b/Userland/Libraries/LibJS/Runtime/FinalizationRegistryPrototype.cpp index 792a5856edf..2ae5a6092e2 100644 --- a/Userland/Libraries/LibJS/Runtime/FinalizationRegistryPrototype.cpp +++ b/Userland/Libraries/LibJS/Runtime/FinalizationRegistryPrototype.cpp @@ -47,7 +47,7 @@ JS_DEFINE_NATIVE_FUNCTION(FinalizationRegistryPrototype::cleanup_some) // IMPLEMENTATION DEFINED: The specification for this function hasn't been updated to accommodate for JobCallback records. // This just follows how the constructor immediately converts the callback to a JobCallback using HostMakeJobCallback. // 4. Perform ? CleanupFinalizationRegistry(finalizationRegistry, callback). - TRY(finalization_registry->cleanup(callback.is_undefined() ? Optional {} : vm.host_make_job_callback(callback.as_function()))); + TRY(finalization_registry->cleanup(callback.is_undefined() ? GCPtr {} : vm.host_make_job_callback(callback.as_function()))); // 5. Return undefined. return js_undefined(); diff --git a/Userland/Libraries/LibJS/Runtime/JobCallback.cpp b/Userland/Libraries/LibJS/Runtime/JobCallback.cpp index e0f06fd9d7c..8119f811f56 100644 --- a/Userland/Libraries/LibJS/Runtime/JobCallback.cpp +++ b/Userland/Libraries/LibJS/Runtime/JobCallback.cpp @@ -5,26 +5,36 @@ */ #include -#include #include namespace JS { +JS_DEFINE_ALLOCATOR(JobCallback); + +JS::NonnullGCPtr JobCallback::create(JS::VM& vm, FunctionObject& callback, OwnPtr custom_data) +{ + return vm.heap().allocate_without_realm(callback, move(custom_data)); +} + +void JobCallback::visit_edges(Visitor& visitor) +{ + visitor.visit(m_callback); +} + // 9.5.2 HostMakeJobCallback ( callback ), https://tc39.es/ecma262/#sec-hostmakejobcallback -JobCallback make_job_callback(FunctionObject& callback) +JS::NonnullGCPtr make_job_callback(FunctionObject& callback) { // 1. Return the JobCallback Record { [[Callback]]: callback, [[HostDefined]]: empty }. - return { make_handle(&callback) }; + return JobCallback::create(callback.vm(), callback, {}); } // 9.5.3 HostCallJobCallback ( jobCallback, V, argumentsList ), https://tc39.es/ecma262/#sec-hostcalljobcallback -ThrowCompletionOr call_job_callback(VM& vm, JobCallback& job_callback, Value this_value, ReadonlySpan arguments_list) +ThrowCompletionOr call_job_callback(VM& vm, JS::NonnullGCPtr job_callback, Value this_value, ReadonlySpan arguments_list) { // 1. Assert: IsCallable(jobCallback.[[Callback]]) is true. - VERIFY(!job_callback.callback.is_null()); // 2. Return ? Call(jobCallback.[[Callback]], V, argumentsList). - return call(vm, job_callback.callback.cell(), this_value, arguments_list); + return call(vm, job_callback->callback(), this_value, arguments_list); } } diff --git a/Userland/Libraries/LibJS/Runtime/JobCallback.h b/Userland/Libraries/LibJS/Runtime/JobCallback.h index 4ff01c0d0d3..2c0da5a739f 100644 --- a/Userland/Libraries/LibJS/Runtime/JobCallback.h +++ b/Userland/Libraries/LibJS/Runtime/JobCallback.h @@ -9,20 +9,40 @@ #include #include #include +#include +#include namespace JS { // 9.5.1 JobCallback Records, https://tc39.es/ecma262/#sec-jobcallback-records -struct JobCallback { +class JobCallback : public JS::Cell { + JS_CELL(JobCallback, JS::Cell); + JS_DECLARE_ALLOCATOR(JobCallback); + +public: struct CustomData { virtual ~CustomData() = default; }; - Handle callback; - OwnPtr custom_data { nullptr }; + [[nodiscard]] static JS::NonnullGCPtr create(JS::VM& vm, FunctionObject& callback, OwnPtr custom_data); + + JobCallback(FunctionObject& callback, OwnPtr custom_data) + : m_callback(callback) + , m_custom_data(move(custom_data)) + { + } + + void visit_edges(Visitor& visitor) override; + + FunctionObject& callback() { return m_callback; } + CustomData* custom_data() { return m_custom_data; } + +private: + JS::NonnullGCPtr m_callback; + OwnPtr m_custom_data { nullptr }; }; -JobCallback make_job_callback(FunctionObject& callback); -ThrowCompletionOr call_job_callback(VM&, JobCallback&, Value this_value, ReadonlySpan arguments_list); +JS::NonnullGCPtr make_job_callback(FunctionObject& callback); +ThrowCompletionOr call_job_callback(VM&, JS::NonnullGCPtr, Value this_value, ReadonlySpan arguments_list); } diff --git a/Userland/Libraries/LibJS/Runtime/Promise.cpp b/Userland/Libraries/LibJS/Runtime/Promise.cpp index 78d03f9361a..a339b844027 100644 --- a/Userland/Libraries/LibJS/Runtime/Promise.cpp +++ b/Userland/Libraries/LibJS/Runtime/Promise.cpp @@ -300,7 +300,7 @@ Value Promise::perform_then(Value on_fulfilled, Value on_rejected, GCPtr on_fulfilled_job_callback; + GCPtr on_fulfilled_job_callback; // 4. Else, if (on_fulfilled.is_function()) { @@ -311,7 +311,7 @@ Value Promise::perform_then(Value on_fulfilled, Value on_rejected, GCPtr on_rejected_job_callback; + GCPtr on_rejected_job_callback; // 6. Else, if (on_rejected.is_function()) { diff --git a/Userland/Libraries/LibJS/Runtime/PromiseJobs.cpp b/Userland/Libraries/LibJS/Runtime/PromiseJobs.cpp index 59b3a0f9fb8..48991d50099 100644 --- a/Userland/Libraries/LibJS/Runtime/PromiseJobs.cpp +++ b/Userland/Libraries/LibJS/Runtime/PromiseJobs.cpp @@ -26,12 +26,12 @@ static ThrowCompletionOr run_reaction_job(VM& vm, PromiseReaction& reacti auto type = reaction.type(); // c. Let handler be reaction.[[Handler]]. - auto& handler = reaction.handler(); + auto handler = reaction.handler(); Completion handler_result; // d. If handler is empty, then - if (!handler.has_value()) { + if (!handler) { dbgln_if(PROMISE_DEBUG, "run_reaction_job: Handler is empty"); // i. If type is Fulfill, let handlerResult be NormalCompletion(argument). @@ -51,8 +51,8 @@ static ThrowCompletionOr run_reaction_job(VM& vm, PromiseReaction& reacti } // e. Else, let handlerResult be Completion(HostCallJobCallback(handler, undefined, « argument »)). else { - dbgln_if(PROMISE_DEBUG, "run_reaction_job: Calling handler callback {} @ {} with argument {}", handler.value().callback.cell()->class_name(), handler.value().callback.cell(), argument); - handler_result = vm.host_call_job_callback(handler.value(), js_undefined(), ReadonlySpan { &argument, 1 }); + dbgln_if(PROMISE_DEBUG, "run_reaction_job: Calling handler callback {} @ {} with argument {}", handler->callback().class_name(), &handler->callback(), argument); + handler_result = vm.host_call_job_callback(*handler, js_undefined(), ReadonlySpan { &argument, 1 }); } // f. If promiseCapability is undefined, then @@ -98,10 +98,10 @@ PromiseJob create_promise_reaction_job(VM& vm, PromiseReaction& reaction, Value Realm* handler_realm { nullptr }; // 3. If reaction.[[Handler]] is not empty, then - auto& handler = reaction.handler(); - if (handler.has_value()) { + auto handler = reaction.handler(); + if (handler) { // a. Let getHandlerRealmResult be Completion(GetFunctionRealm(reaction.[[Handler]].[[Callback]])). - auto get_handler_realm_result = get_function_realm(vm, *handler->callback.cell()); + auto get_handler_realm_result = get_function_realm(vm, handler->callback()); // b. If getHandlerRealmResult is a normal completion, set handlerRealm to getHandlerRealmResult.[[Value]]. if (!get_handler_realm_result.is_throw_completion()) { @@ -144,10 +144,10 @@ static ThrowCompletionOr run_resolve_thenable_job(VM& vm, Promise& promis } // 27.2.2.2 NewPromiseResolveThenableJob ( promiseToResolve, thenable, then ), https://tc39.es/ecma262/#sec-newpromiseresolvethenablejob -PromiseJob create_promise_resolve_thenable_job(VM& vm, Promise& promise_to_resolve, Value thenable, JobCallback then) +PromiseJob create_promise_resolve_thenable_job(VM& vm, Promise& promise_to_resolve, Value thenable, JS::NonnullGCPtr then) { // 2. Let getThenRealmResult be Completion(GetFunctionRealm(then.[[Callback]])). - auto get_then_realm_result = get_function_realm(vm, *then.callback.cell()); + auto get_then_realm_result = get_function_realm(vm, then->callback()); Realm* then_realm; @@ -164,7 +164,7 @@ 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 = move(then)]() mutable { + auto job = [&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); }; diff --git a/Userland/Libraries/LibJS/Runtime/PromiseJobs.h b/Userland/Libraries/LibJS/Runtime/PromiseJobs.h index ca22c8a1ccf..09f14873a7a 100644 --- a/Userland/Libraries/LibJS/Runtime/PromiseJobs.h +++ b/Userland/Libraries/LibJS/Runtime/PromiseJobs.h @@ -21,6 +21,6 @@ struct PromiseJob { // NOTE: These return a PromiseJob to prevent awkward casting at call sites. PromiseJob create_promise_reaction_job(VM&, PromiseReaction&, Value argument); -PromiseJob create_promise_resolve_thenable_job(VM&, Promise&, Value thenable, JobCallback then); +PromiseJob create_promise_resolve_thenable_job(VM&, Promise&, Value thenable, JS::NonnullGCPtr then); } diff --git a/Userland/Libraries/LibJS/Runtime/PromiseReaction.cpp b/Userland/Libraries/LibJS/Runtime/PromiseReaction.cpp index 96c14876f91..b2893dd0b67 100644 --- a/Userland/Libraries/LibJS/Runtime/PromiseReaction.cpp +++ b/Userland/Libraries/LibJS/Runtime/PromiseReaction.cpp @@ -12,12 +12,12 @@ namespace JS { JS_DEFINE_ALLOCATOR(PromiseReaction); -NonnullGCPtr PromiseReaction::create(VM& vm, Type type, GCPtr capability, Optional handler) +NonnullGCPtr PromiseReaction::create(VM& vm, Type type, GCPtr capability, GCPtr handler) { return vm.heap().allocate_without_realm(type, capability, move(handler)); } -PromiseReaction::PromiseReaction(Type type, GCPtr capability, Optional handler) +PromiseReaction::PromiseReaction(Type type, GCPtr capability, GCPtr handler) : m_type(type) , m_capability(capability) , m_handler(move(handler)) @@ -28,6 +28,7 @@ void PromiseReaction::visit_edges(Cell::Visitor& visitor) { Base::visit_edges(visitor); visitor.visit(m_capability); + visitor.visit(m_handler); } } diff --git a/Userland/Libraries/LibJS/Runtime/PromiseReaction.h b/Userland/Libraries/LibJS/Runtime/PromiseReaction.h index 04000a798fc..4398f4a115b 100644 --- a/Userland/Libraries/LibJS/Runtime/PromiseReaction.h +++ b/Userland/Libraries/LibJS/Runtime/PromiseReaction.h @@ -24,24 +24,24 @@ public: Reject, }; - static NonnullGCPtr create(VM& vm, Type type, GCPtr capability, Optional handler); + static NonnullGCPtr create(VM& vm, Type type, GCPtr capability, JS::GCPtr handler); virtual ~PromiseReaction() = default; Type type() const { return m_type; } GCPtr capability() const { return m_capability; } - Optional& handler() { return m_handler; } - Optional const& handler() const { return m_handler; } + JS::GCPtr handler() { return m_handler; } + JS::GCPtr handler() const { return m_handler; } private: - PromiseReaction(Type type, GCPtr capability, Optional handler); + PromiseReaction(Type type, GCPtr capability, JS::GCPtr handler); virtual void visit_edges(Visitor&) override; Type m_type; GCPtr m_capability; - Optional m_handler; + JS::GCPtr m_handler; }; } diff --git a/Userland/Libraries/LibJS/Runtime/VM.h b/Userland/Libraries/LibJS/Runtime/VM.h index 0437e239d2f..fce45802c77 100644 --- a/Userland/Libraries/LibJS/Runtime/VM.h +++ b/Userland/Libraries/LibJS/Runtime/VM.h @@ -255,7 +255,7 @@ public: Function(JobCallback&, Value, ReadonlySpan)> host_call_job_callback; Function host_enqueue_finalization_registry_cleanup_job; Function()>, Realm*)> host_enqueue_promise_job; - Function host_make_job_callback; + Function(FunctionObject&)> host_make_job_callback; Function(Realm&)> host_ensure_can_compile_strings; Function(Object&)> host_ensure_can_add_private_element; Function(ArrayBuffer&, size_t)> host_resize_array_buffer; diff --git a/Userland/Libraries/LibWeb/Bindings/MainThreadVM.cpp b/Userland/Libraries/LibWeb/Bindings/MainThreadVM.cpp index 3392566ede9..72bf89bba8e 100644 --- a/Userland/Libraries/LibWeb/Bindings/MainThreadVM.cpp +++ b/Userland/Libraries/LibWeb/Bindings/MainThreadVM.cpp @@ -191,7 +191,7 @@ ErrorOr initialize_main_thread_vm() // 8.1.5.4.1 HostCallJobCallback(callback, V, argumentsList), https://html.spec.whatwg.org/multipage/webappapis.html#hostcalljobcallback s_main_thread_vm->host_call_job_callback = [](JS::JobCallback& callback, JS::Value this_value, ReadonlySpan arguments_list) { - auto& callback_host_defined = verify_cast(*callback.custom_data); + auto& callback_host_defined = verify_cast(*callback.custom_data()); // 1. Let incumbent settings be callback.[[HostDefined]].[[IncumbentSettings]]. (NOTE: Not necessary) // 2. Let script execution context be callback.[[HostDefined]].[[ActiveScriptContext]]. (NOTE: Not necessary) @@ -204,7 +204,7 @@ ErrorOr initialize_main_thread_vm() s_main_thread_vm->push_execution_context(*callback_host_defined.active_script_context); // 5. Let result be Call(callback.[[Callback]], V, argumentsList). - auto result = JS::call(*s_main_thread_vm, *callback.callback.cell(), this_value, arguments_list); + auto result = JS::call(*s_main_thread_vm, callback.callback(), this_value, arguments_list); // 6. If script execution context is not null, then pop script execution context from the JavaScript execution context stack. if (callback_host_defined.active_script_context) { @@ -227,7 +227,7 @@ ErrorOr initialize_main_thread_vm() // 2. Queue a global task on the JavaScript engine task source given global to perform the following steps: HTML::queue_global_task(HTML::Task::Source::JavaScriptEngine, global, [&finalization_registry] { // 1. Let entry be finalizationRegistry.[[CleanupCallback]].[[Callback]].[[Realm]]'s environment settings object. - auto& entry = host_defined_environment_settings_object(*finalization_registry.cleanup_callback().callback.cell()->realm()); + auto& entry = host_defined_environment_settings_object(*finalization_registry.cleanup_callback().callback().realm()); // 2. Check if we can run script with entry. If this returns "do not run", then return. if (entry.can_run_script() == HTML::RunScriptDecision::DoNotRun) @@ -319,7 +319,7 @@ ErrorOr initialize_main_thread_vm() }; // 8.1.5.4.4 HostMakeJobCallback(callable), https://html.spec.whatwg.org/multipage/webappapis.html#hostmakejobcallback - s_main_thread_vm->host_make_job_callback = [](JS::FunctionObject& callable) -> JS::JobCallback { + s_main_thread_vm->host_make_job_callback = [](JS::FunctionObject& callable) -> JS::NonnullGCPtr { // 1. Let incumbent settings be the incumbent settings object. auto& incumbent_settings = HTML::incumbent_settings_object(); @@ -351,7 +351,7 @@ ErrorOr initialize_main_thread_vm() // 5. Return the JobCallback Record { [[Callback]]: callable, [[HostDefined]]: { [[IncumbentSettings]]: incumbent settings, [[ActiveScriptContext]]: script execution context } }. auto host_defined = adopt_own(*new WebEngineCustomJobCallbackData(incumbent_settings, move(script_execution_context))); - return { JS::make_handle(&callable), move(host_defined) }; + return JS::JobCallback::create(*s_main_thread_vm, callable, move(host_defined)); }; // 8.1.5.5.1 HostGetImportMetaProperties(moduleRecord), https://html.spec.whatwg.org/multipage/webappapis.html#hostgetimportmetaproperties