From ceefe7d858d55f66ea36cb1688820b61a65d2102 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Thu, 26 Dec 2024 18:17:41 +0100 Subject: [PATCH] LibWeb: Make CustomElementDefinition relationship with GC more sane 1. Stop using GC::Root in member variables, since that usually creates a realm leak. 2. Stop putting OrderedHashMap on the stack while setting these up, since that won't protect the objects from GC. --- Libraries/LibWeb/DOM/Element.cpp | 2 +- .../CustomElementDefinition.cpp | 5 +++++ .../CustomElements/CustomElementDefinition.h | 20 +++++++++---------- .../CustomElements/CustomElementRegistry.cpp | 2 +- .../BindingsGenerator/IDLGenerators.cpp | 2 +- 5 files changed, 17 insertions(+), 14 deletions(-) diff --git a/Libraries/LibWeb/DOM/Element.cpp b/Libraries/LibWeb/DOM/Element.cpp index 3e723cea0f4..6a4f1f666a1 100644 --- a/Libraries/LibWeb/DOM/Element.cpp +++ b/Libraries/LibWeb/DOM/Element.cpp @@ -2181,7 +2181,7 @@ JS::ThrowCompletionOr Element::upgrade_element(GC::Refconstruction_stack().append(GC::make_root(this)); + custom_element_definition->construction_stack().append(GC::Ref { *this }); // 7. Let C be definition's constructor. auto& constructor = custom_element_definition->constructor(); diff --git a/Libraries/LibWeb/HTML/CustomElements/CustomElementDefinition.cpp b/Libraries/LibWeb/HTML/CustomElements/CustomElementDefinition.cpp index 066b2e67760..91b56e2b055 100644 --- a/Libraries/LibWeb/HTML/CustomElements/CustomElementDefinition.cpp +++ b/Libraries/LibWeb/HTML/CustomElements/CustomElementDefinition.cpp @@ -13,6 +13,11 @@ void CustomElementDefinition::visit_edges(Visitor& visitor) Base::visit_edges(visitor); visitor.visit(m_constructor); visitor.visit(m_lifecycle_callbacks); + for (auto& entry : m_construction_stack) { + entry.visit( + [&](GC::Ref& element) { visitor.visit(element); }, + [&](AlreadyConstructedCustomElementMarker&) {}); + } } } diff --git a/Libraries/LibWeb/HTML/CustomElements/CustomElementDefinition.h b/Libraries/LibWeb/HTML/CustomElements/CustomElementDefinition.h index 7af0a4a044c..23eacd2276e 100644 --- a/Libraries/LibWeb/HTML/CustomElements/CustomElementDefinition.h +++ b/Libraries/LibWeb/HTML/CustomElements/CustomElementDefinition.h @@ -20,10 +20,7 @@ class CustomElementDefinition : public JS::Cell { GC_CELL(CustomElementDefinition, JS::Cell); GC_DECLARE_ALLOCATOR(CustomElementDefinition); - using LifecycleCallbacksStorage = OrderedHashMap>; - using ConstructionStackStorage = Vector, AlreadyConstructedCustomElementMarker>>; - - static GC::Ref create(JS::Realm& realm, String const& name, String const& local_name, WebIDL::CallbackType& constructor, Vector&& observed_attributes, LifecycleCallbacksStorage&& lifecycle_callbacks, bool form_associated, bool disable_internals, bool disable_shadow) + static GC::Ref create(JS::Realm& realm, String const& name, String const& local_name, WebIDL::CallbackType& constructor, Vector&& observed_attributes, OrderedHashMap> lifecycle_callbacks, bool form_associated, bool disable_internals, bool disable_shadow) { return realm.create(name, local_name, constructor, move(observed_attributes), move(lifecycle_callbacks), form_associated, disable_internals, disable_shadow); } @@ -38,26 +35,27 @@ class CustomElementDefinition : public JS::Cell { Vector const& observed_attributes() const { return m_observed_attributes; } - LifecycleCallbacksStorage const& lifecycle_callbacks() const { return m_lifecycle_callbacks; } + auto const& lifecycle_callbacks() const { return m_lifecycle_callbacks; } - ConstructionStackStorage& construction_stack() { return m_construction_stack; } - ConstructionStackStorage const& construction_stack() const { return m_construction_stack; } + auto& construction_stack() { return m_construction_stack; } + auto const& construction_stack() const { return m_construction_stack; } bool form_associated() const { return m_form_associated; } bool disable_internals() const { return m_disable_internals; } bool disable_shadow() const { return m_disable_shadow; } private: - CustomElementDefinition(String const& name, String const& local_name, WebIDL::CallbackType& constructor, Vector&& observed_attributes, LifecycleCallbacksStorage&& lifecycle_callbacks, bool form_associated, bool disable_internals, bool disable_shadow) + CustomElementDefinition(String const& name, String const& local_name, WebIDL::CallbackType& constructor, Vector&& observed_attributes, OrderedHashMap>&& lifecycle_callbacks, bool form_associated, bool disable_internals, bool disable_shadow) : m_name(name) , m_local_name(local_name) , m_constructor(constructor) , m_observed_attributes(move(observed_attributes)) - , m_lifecycle_callbacks(move(lifecycle_callbacks)) , m_form_associated(form_associated) , m_disable_internals(disable_internals) , m_disable_shadow(disable_shadow) { + for (auto& [key, value] : lifecycle_callbacks) + m_lifecycle_callbacks.set(key, value.cell()); } virtual void visit_edges(Visitor& visitor) override; @@ -87,13 +85,13 @@ private: // "formAssociatedCallback", "formDisabledCallback", "formResetCallback", and "formStateRestoreCallback". // The corresponding values are either a Web IDL Function callback function type value, or null. // By default the value of each entry is null. - LifecycleCallbacksStorage m_lifecycle_callbacks; + OrderedHashMap> m_lifecycle_callbacks; // https://html.spec.whatwg.org/multipage/custom-elements.html#concept-custom-element-definition-construction-stack // A construction stack // A list, initially empty, that is manipulated by the upgrade an element algorithm and the HTML element constructors. // Each entry in the list will be either an element or an already constructed marker. - ConstructionStackStorage m_construction_stack; + Vector, AlreadyConstructedCustomElementMarker>> m_construction_stack; // https://html.spec.whatwg.org/multipage/custom-elements.html#concept-custom-element-definition-form-associated // A form-associated boolean diff --git a/Libraries/LibWeb/HTML/CustomElements/CustomElementRegistry.cpp b/Libraries/LibWeb/HTML/CustomElements/CustomElementRegistry.cpp index 7e84013f235..c5e0ea656e4 100644 --- a/Libraries/LibWeb/HTML/CustomElements/CustomElementRegistry.cpp +++ b/Libraries/LibWeb/HTML/CustomElements/CustomElementRegistry.cpp @@ -185,7 +185,7 @@ JS::ThrowCompletionOr CustomElementRegistry::define(String const& name, We // NOTE: This is not in the spec, but is required because of how we catch the exception by using a lambda, meaning we need to define this // variable outside of it to use it later. - CustomElementDefinition::LifecycleCallbacksStorage lifecycle_callbacks; + OrderedHashMap> lifecycle_callbacks; // 14. Run the following steps while catching any exceptions: auto get_definition_attributes_from_constructor = [&]() -> JS::ThrowCompletionOr { diff --git a/Meta/Lagom/Tools/CodeGenerators/LibWeb/BindingsGenerator/IDLGenerators.cpp b/Meta/Lagom/Tools/CodeGenerators/LibWeb/BindingsGenerator/IDLGenerators.cpp index 33fb846b057..a9c6c236c97 100644 --- a/Meta/Lagom/Tools/CodeGenerators/LibWeb/BindingsGenerator/IDLGenerators.cpp +++ b/Meta/Lagom/Tools/CodeGenerators/LibWeb/BindingsGenerator/IDLGenerators.cpp @@ -2635,7 +2635,7 @@ static void generate_html_constructor(SourceGenerator& generator, IDL::Construct return JS::throw_completion(WebIDL::InvalidStateError::create(realm, "Custom element has already been constructed"_string)); // 12. Perform ? element.[[SetPrototypeOf]](prototype). - auto actual_element = element.get>(); + auto actual_element = element.get>(); TRY(actual_element->internal_set_prototype_of(&prototype.as_object())); // 13. Replace the last entry in definition's construction stack with an already constructed marker.