LibJS+LibWeb: Make CyclicModule & GraphLoadingState GC-allocated

This allows them to participate in the ownership graph and fixes a
lifetime issue in module loading found by ASAN.

Co-Authored-By: networkException <networkexception@serenityos.org>

Userland/Libraries/LibJS/CyclicModule.cpp

@@ -35,8 +35,17 @@ void CyclicModule::visit_edges(Cell::Visitor& visitor)
         visitor.visit(loaded_module.module);
 }
 
+void GraphLoadingState::visit_edges(Cell::Visitor& visitor)
+{
+    Base::visit_edges(visitor);
+    visitor.visit(promise_capability);
+    visitor.visit(host_defined);
+    for (auto* module : visited)
+        visitor.visit(*module);
+}
+
 // 16.2.1.5.1 LoadRequestedModules ( [ hostDefined ] ), https://tc39.es/ecma262/#sec-LoadRequestedModules
-PromiseCapability& CyclicModule::load_requested_modules(JS::Realm& realm, Optional<GraphLoadingState::HostDefined> host_defined)
+PromiseCapability& CyclicModule::load_requested_modules(JS::Realm& realm, GCPtr<GraphLoadingState::HostDefined> host_defined)
 {
     // 1. If hostDefined is not present, let hostDefined be EMPTY.
     // NOTE: The empty state is handled by hostDefined being an optional without value.
@@ -45,7 +54,7 @@ PromiseCapability& CyclicModule::load_requested_modules(JS::Realm& realm, Option
     auto promise_capability = MUST(new_promise_capability(realm.vm(), realm.intrinsics().promise_constructor()));
 
     // 3. Let state be the GraphLoadingState Record { [[IsLoading]]: true, [[PendingModulesCount]]: 1, [[Visited]]: « », [[PromiseCapability]]: pc, [[HostDefined]]: hostDefined }.
-    auto state = GraphLoadingState { .promise_capability = promise_capability, .is_loading = true, .pending_module_count = 1, .visited = {}, .host_defined = move(host_defined) };
+    auto state = heap().allocate_without_realm<GraphLoadingState>(promise_capability, true, 1, HashTable<CyclicModule*> {}, move(host_defined));
 
     // 4. Perform InnerModuleLoading(state, module).
     inner_module_loading(state);

Userland/Libraries/LibJS/CyclicModule.h

@@ -25,18 +25,33 @@ enum class ModuleStatus {
 class CyclicModule;
 
 // https://tc39.es/ecma262/#graphloadingstate-record
-struct GraphLoadingState {
+struct GraphLoadingState : public Cell {
-    struct HostDefined {
+    JS_CELL(GraphLoadingState, Cell);
-        virtual ~HostDefined() = default;
 
-        virtual void visit_edges(Cell::Visitor&) { }
+public:
+    struct HostDefined : Cell {
+        JS_CELL(HostDefined, Cell);
+
+    public:
+        virtual ~HostDefined() = default;
     };
 
     GCPtr<PromiseCapability> promise_capability; // [[PromiseCapability]]
     bool is_loading { false };                   // [[IsLoading]]
     size_t pending_module_count { 0 };           // [[PendingModulesCount]]
     HashTable<CyclicModule*> visited;            // [[Visited]]
-    Optional<HostDefined> host_defined;          // [[HostDefined]]
+    GCPtr<HostDefined> host_defined;             // [[HostDefined]]
+
+private:
+    GraphLoadingState(GCPtr<PromiseCapability> promise_capability, bool is_loading, size_t pending_module_count, HashTable<CyclicModule*> visited, GCPtr<HostDefined> host_defined)
+        : promise_capability(move(promise_capability))
+        , is_loading(is_loading)
+        , pending_module_count(pending_module_count)
+        , visited(move(visited))
+        , host_defined(move(host_defined))
+    {
+    }
+    virtual void visit_edges(Cell::Visitor&) override;
 };
 
 // 16.2.1.5 Cyclic Module Records, https://tc39.es/ecma262/#cyclic-module-record
@@ -50,7 +65,7 @@ public:
     virtual ThrowCompletionOr<void> link(VM& vm) override final;
     virtual ThrowCompletionOr<Promise*> evaluate(VM& vm) override final;
 
-    virtual PromiseCapability& load_requested_modules(Realm&, Optional<GraphLoadingState::HostDefined>);
+    virtual PromiseCapability& load_requested_modules(Realm&, GCPtr<GraphLoadingState::HostDefined>);
     virtual void inner_module_loading(GraphLoadingState& state);
 
     Vector<ModuleRequest> const& requested_modules() const { return m_requested_modules; }

Userland/Libraries/LibJS/Runtime/VM.h

@@ -228,7 +228,7 @@ public:
     //       Our implementation of this proposal is outdated however, as such we try to adapt the proposal and living standard
     //       to match our implementation for now.
     // 16.2.1.8 HostLoadImportedModule ( referrer, moduleRequest, hostDefined, payload ), https://tc39.es/proposal-import-attributes/#sec-HostLoadImportedModule
-    Function<void(Realm&, Variant<NonnullGCPtr<Script>, NonnullGCPtr<CyclicModule>>, ModuleRequest const&, Optional<GraphLoadingState::HostDefined>, GraphLoadingState&)> host_load_imported_module;
+    Function<void(Realm&, Variant<NonnullGCPtr<Script>, NonnullGCPtr<CyclicModule>>, ModuleRequest const&, GCPtr<GraphLoadingState::HostDefined>, GraphLoadingState&)> host_load_imported_module;
 
     Function<ThrowCompletionOr<NonnullGCPtr<Module>>(ScriptOrModule, ModuleRequest const&)> host_resolve_imported_module;
     Function<ThrowCompletionOr<void>(ScriptOrModule, ModuleRequest, PromiseCapability const&)> host_import_module_dynamically;

Userland/Libraries/LibWeb/Bindings/MainThreadVM.cpp

@@ -404,7 +404,7 @@ ErrorOr<void> initialize_main_thread_vm()
     };
 
     // 8.1.6.5.3 HostLoadImportedModule(referrer, moduleRequest, loadState, payload), https://html.spec.whatwg.org/multipage/webappapis.html#hostloadimportedmodule
-    s_main_thread_vm->host_load_imported_module = [](JS::Realm& realm, Variant<JS::NonnullGCPtr<JS::Script>, JS::NonnullGCPtr<JS::CyclicModule>> referrer, JS::ModuleRequest const& module_request, Optional<JS::GraphLoadingState::HostDefined> load_state, JS::GraphLoadingState& payload) -> void {
+    s_main_thread_vm->host_load_imported_module = [](JS::Realm& realm, Variant<JS::NonnullGCPtr<JS::Script>, JS::NonnullGCPtr<JS::CyclicModule>> referrer, JS::ModuleRequest const& module_request, JS::GCPtr<JS::GraphLoadingState::HostDefined> load_state, JS::GraphLoadingState& payload) -> void {
         // 1. Let settingsObject be the current settings object.
         Optional<HTML::EnvironmentSettingsObject&> settings_object = HTML::current_settings_object();
 
@@ -460,8 +460,8 @@ ErrorOr<void> initialize_main_thread_vm()
         Optional<HTML::EnvironmentSettingsObject&> fetch_client = *settings_object;
 
         // 12. If loadState is not undefined, then:
-        if (load_state.has_value()) {
+        if (load_state) {
-            auto fetch_context = static_cast<HTML::FetchContext&>(load_state.value());
+            auto& fetch_context = static_cast<HTML::FetchContext&>(*load_state);
 
             // 1. Set destination to loadState.[[Destination]].
             destination = fetch_context.destination;
@@ -490,9 +490,9 @@ ErrorOr<void> initialize_main_thread_vm()
                 completion = JS::throw_completion(parse_error);
 
                 // 3. If loadState is not undefined and loadState.[[ParseError]] is null, set loadState.[[ParseError]] to parseError.
-                if (load_state.has_value()) {
+                if (load_state) {
-                    auto load_state_as_fetch_context = static_cast<HTML::FetchContext&>(load_state.value());
+                    auto& load_state_as_fetch_context = static_cast<HTML::FetchContext&>(*load_state);
-                    if (load_state_as_fetch_context.parse_error->is_empty()) {
+                    if (load_state_as_fetch_context.parse_error.is_null()) {
                         load_state_as_fetch_context.parse_error = parse_error;
                     }
                 }

Userland/Libraries/LibWeb/HTML/Scripting/Fetching.cpp

@@ -760,7 +760,7 @@ void fetch_descendants_of_and_link_a_module_script(JS::Realm& realm,
     }
 
     // 3. Let state be Record { [[ParseError]]: null, [[Destination]]: destination, [[PerformFetch]]: null, [[FetchClient]]: fetchClient }.
-    auto state = FetchContext { {}, destination, {}, fetch_client };
+    auto state = realm.heap().allocate_without_realm<FetchContext>(JS::js_null(), destination, nullptr, fetch_client);
 
     // FIXME: 4. If performFetch was given, set state.[[PerformFetch]] to performFetch.
 
@@ -796,8 +796,8 @@ void fetch_descendants_of_and_link_a_module_script(JS::Realm& realm,
     WebIDL::upon_rejection(loading_promise, [&state, &module_script, on_complete](auto const&) -> WebIDL::ExceptionOr<JS::Value> {
         // 1. If state.[[ParseError]] is not null, set moduleScript's error to rethrow to state.[[ParseError]] and run
         //    onComplete given moduleScript.
-        if (state.parse_error != nullptr) {
+        if (!state->parse_error.is_null()) {
-            module_script.set_error_to_rethrow(*state.parse_error);
+            module_script.set_error_to_rethrow(state->parse_error);
 
             on_complete->function()(module_script);
         }

Userland/Libraries/LibWeb/HTML/Scripting/Fetching.h

@@ -54,8 +54,17 @@ struct ScriptFetchOptions {
 // https://html.spec.whatwg.org/multipage/webappapis.html#default-classic-script-fetch-options
 ScriptFetchOptions default_classic_script_fetch_options();
 
-struct FetchContext : JS::GraphLoadingState::HostDefined {
+class FetchContext : public JS::GraphLoadingState::HostDefined {
-    FetchContext(JS::GCPtr<JS::Value> parse_error, Fetch::Infrastructure::Request::Destination destination, JS::GCPtr<JS::Promise> perform_fetch, EnvironmentSettingsObject& fetch_client)
+    JS_CELL(FetchContext, JS::GraphLoadingState::HostDefined);
+
+public:
+    JS::Value parse_error;                                   // [[ParseError]]
+    Fetch::Infrastructure::Request::Destination destination; // [[Destination]]
+    JS::GCPtr<JS::Promise> perform_fetch;                    // [[PerformFetch]]
+    EnvironmentSettingsObject& fetch_client;                 // [[FetchClient]]
+
+private:
+    FetchContext(JS::Value parse_error, Fetch::Infrastructure::Request::Destination destination, JS::GCPtr<JS::Promise> perform_fetch, EnvironmentSettingsObject& fetch_client)
         : parse_error(parse_error)
         , destination(destination)
         , perform_fetch(perform_fetch)
@@ -63,10 +72,13 @@ struct FetchContext : JS::GraphLoadingState::HostDefined {
     {
     }
 
-    JS::GCPtr<JS::Value> parse_error;                        // [[ParseError]]
+    void visit_edges(Visitor& visitor) override
-    Fetch::Infrastructure::Request::Destination destination; // [[Destination]]
+    {
-    JS::GCPtr<JS::Promise> perform_fetch;                    // [[PerformFetch]]
+        Base::visit_edges(visitor);
-    EnvironmentSettingsObject& fetch_client;                 // [[FetchClient]]
+        visitor.visit(parse_error);
+        visitor.visit(perform_fetch);
+        visitor.visit(fetch_client);
+    }
 };
 
 DeprecatedString module_type_from_module_request(JS::ModuleRequest const&);