LibJS+LibWeb: Change JobCallback to be GC-allocated

Fixes leak caused by mutual dependency when JS::Handle<JobCallback> is
owned by GC-allocated PromiseReaction.

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;
 

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<JobCallback> 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<Heap>)
 }
 
 // 9.13 CleanupFinalizationRegistry ( finalizationRegistry ), https://tc39.es/ecma262/#sec-cleanup-finalization-registry
-ThrowCompletionOr<void> FinalizationRegistry::cleanup(Optional<JobCallback> callback)
+ThrowCompletionOr<void> FinalizationRegistry::cleanup(JS::GCPtr<JobCallback> callback)
 {
     auto& vm = this->vm();
 
@@ -70,7 +70,7 @@ ThrowCompletionOr<void> FinalizationRegistry::cleanup(Optional<JobCallback> 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<void> FinalizationRegistry::cleanup(Optional<JobCallback> 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);

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<void> cleanup(Optional<JobCallback> = {});
+    ThrowCompletionOr<void> cleanup(GCPtr<JobCallback> = {});
 
     virtual void remove_dead_cells(Badge<Heap>) 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<JobCallback>, Object& prototype);
 
     virtual void visit_edges(Visitor& visitor) override;
 
     NonnullGCPtr<Realm> m_realm;
-    JobCallback m_cleanup_callback;
+    NonnullGCPtr<JobCallback> m_cleanup_callback;
 
     struct FinalizationRecord {
         GCPtr<Cell> target;

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<JobCallback> {} : vm.host_make_job_callback(callback.as_function())));
+    TRY(finalization_registry->cleanup(callback.is_undefined() ? GCPtr<JobCallback> {} : vm.host_make_job_callback(callback.as_function())));
 
     // 5. Return undefined.
     return js_undefined();

Userland/Libraries/LibJS/Runtime/JobCallback.cpp

@@ -5,26 +5,36 @@
  */
 
 #include <LibJS/Runtime/AbstractOperations.h>
-#include <LibJS/Runtime/FunctionObject.h>
 #include <LibJS/Runtime/JobCallback.h>
 
 namespace JS {
 
+JS_DEFINE_ALLOCATOR(JobCallback);
+
+JS::NonnullGCPtr<JobCallback> JobCallback::create(JS::VM& vm, FunctionObject& callback, OwnPtr<CustomData> custom_data)
+{
+    return vm.heap().allocate_without_realm<JobCallback>(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<JobCallback> 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<Value> call_job_callback(VM& vm, JobCallback& job_callback, Value this_value, ReadonlySpan<Value> arguments_list)
+ThrowCompletionOr<Value> call_job_callback(VM& vm, JS::NonnullGCPtr<JobCallback> job_callback, Value this_value, ReadonlySpan<Value> 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);
 }
 
 }

Userland/Libraries/LibJS/Runtime/JobCallback.h

@@ -9,20 +9,40 @@
 #include <AK/OwnPtr.h>
 #include <LibJS/Heap/Handle.h>
 #include <LibJS/Runtime/Completion.h>
+#include <LibJS/Runtime/FunctionObject.h>
+#include <LibJS/Runtime/VM.h>
 
 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<FunctionObject> callback;
-    OwnPtr<CustomData> custom_data { nullptr };
+    [[nodiscard]] static JS::NonnullGCPtr<JobCallback> create(JS::VM& vm, FunctionObject& callback, OwnPtr<CustomData> custom_data);
+
+    JobCallback(FunctionObject& callback, OwnPtr<CustomData> 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<FunctionObject> m_callback;
+    OwnPtr<CustomData> m_custom_data { nullptr };
 };
 
-JobCallback make_job_callback(FunctionObject& callback);
-ThrowCompletionOr<Value> call_job_callback(VM&, JobCallback&, Value this_value, ReadonlySpan<Value> arguments_list);
+JS::NonnullGCPtr<JobCallback> make_job_callback(FunctionObject& callback);
+ThrowCompletionOr<Value> call_job_callback(VM&, JS::NonnullGCPtr<JobCallback>, Value this_value, ReadonlySpan<Value> arguments_list);
 
 }

Userland/Libraries/LibJS/Runtime/Promise.cpp

@@ -300,7 +300,7 @@ Value Promise::perform_then(Value on_fulfilled, Value on_rejected, GCPtr<Promise
 
     // 3. If IsCallable(onFulfilled) is false, then
     //     a. Let onFulfilledJobCallback be empty.
-    Optional<JobCallback> on_fulfilled_job_callback;
+    GCPtr<JobCallback> 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<Promise
 
     // 5. If IsCallable(onRejected) is false, then
     //     a. Let onRejectedJobCallback be empty.
-    Optional<JobCallback> on_rejected_job_callback;
+    GCPtr<JobCallback> on_rejected_job_callback;
 
     // 6. Else,
     if (on_rejected.is_function()) {

Userland/Libraries/LibJS/Runtime/PromiseJobs.cpp

@@ -26,12 +26,12 @@ static ThrowCompletionOr<Value> 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<Value> 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<Value> { &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<Value> { &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<Value> 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<JobCallback> 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);
     };
 

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<JobCallback> then);
 
 }

Userland/Libraries/LibJS/Runtime/PromiseReaction.cpp

@@ -12,12 +12,12 @@ namespace JS {
 
 JS_DEFINE_ALLOCATOR(PromiseReaction);
 
-NonnullGCPtr<PromiseReaction> PromiseReaction::create(VM& vm, Type type, GCPtr<PromiseCapability> capability, Optional<JobCallback> handler)
+NonnullGCPtr<PromiseReaction> PromiseReaction::create(VM& vm, Type type, GCPtr<PromiseCapability> capability, GCPtr<JobCallback> handler)
 {
     return vm.heap().allocate_without_realm<PromiseReaction>(type, capability, move(handler));
 }
 
-PromiseReaction::PromiseReaction(Type type, GCPtr<PromiseCapability> capability, Optional<JobCallback> handler)
+PromiseReaction::PromiseReaction(Type type, GCPtr<PromiseCapability> capability, GCPtr<JobCallback> 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);
 }
 
 }

Userland/Libraries/LibJS/Runtime/PromiseReaction.h

@@ -24,24 +24,24 @@ public:
         Reject,
     };
 
-    static NonnullGCPtr<PromiseReaction> create(VM& vm, Type type, GCPtr<PromiseCapability> capability, Optional<JobCallback> handler);
+    static NonnullGCPtr<PromiseReaction> create(VM& vm, Type type, GCPtr<PromiseCapability> capability, JS::GCPtr<JobCallback> handler);
 
     virtual ~PromiseReaction() = default;
 
     Type type() const { return m_type; }
     GCPtr<PromiseCapability> capability() const { return m_capability; }
 
-    Optional<JobCallback>& handler() { return m_handler; }
-    Optional<JobCallback> const& handler() const { return m_handler; }
+    JS::GCPtr<JobCallback> handler() { return m_handler; }
+    JS::GCPtr<JobCallback const> handler() const { return m_handler; }
 
 private:
-    PromiseReaction(Type type, GCPtr<PromiseCapability> capability, Optional<JobCallback> handler);
+    PromiseReaction(Type type, GCPtr<PromiseCapability> capability, JS::GCPtr<JobCallback> handler);
 
     virtual void visit_edges(Visitor&) override;
 
     Type m_type;
     GCPtr<PromiseCapability> m_capability;
-    Optional<JobCallback> m_handler;
+    JS::GCPtr<JobCallback> m_handler;
 };
 
 }

Userland/Libraries/LibJS/Runtime/VM.h

@@ -255,7 +255,7 @@ public:
     Function<ThrowCompletionOr<Value>(JobCallback&, Value, ReadonlySpan<Value>)> host_call_job_callback;
     Function<void(FinalizationRegistry&)> host_enqueue_finalization_registry_cleanup_job;
     Function<void(Function<ThrowCompletionOr<Value>()>, Realm*)> host_enqueue_promise_job;
-    Function<JobCallback(FunctionObject&)> host_make_job_callback;
+    Function<JS::NonnullGCPtr<JobCallback>(FunctionObject&)> host_make_job_callback;
     Function<ThrowCompletionOr<void>(Realm&)> host_ensure_can_compile_strings;
     Function<ThrowCompletionOr<void>(Object&)> host_ensure_can_add_private_element;
     Function<ThrowCompletionOr<HandledByHost>(ArrayBuffer&, size_t)> host_resize_array_buffer;

Userland/Libraries/LibWeb/Bindings/MainThreadVM.cpp

@@ -191,7 +191,7 @@ ErrorOr<void> 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<JS::Value> arguments_list) {
-        auto& callback_host_defined = verify_cast<WebEngineCustomJobCallbackData>(*callback.custom_data);
+        auto& callback_host_defined = verify_cast<WebEngineCustomJobCallbackData>(*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<void> 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<void> 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<void> 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<JS::JobCallback> {
         // 1. Let incumbent settings be the incumbent settings object.
         auto& incumbent_settings = HTML::incumbent_settings_object();
 
@@ -351,7 +351,7 @@ ErrorOr<void> 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