From b67f6daf05afa1f052071740a24d296ee9c5b01d Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Sun, 17 Oct 2021 15:09:47 -0400 Subject: [PATCH] LibWeb: Weakly store NamedNodeMap's & Attribute's associated Element This is similar to how Gecko avoids a reference cycle, where both the NamedNodeMap and Attribute would otherwise store a strong reference to their associated Element. Gecko manually clears stored raw references when an Element is destroyed, whereas we use weak references to do so automatically. Attribute's ownerElement getter and setter are moved out of line to avoid an #include cycle between Element and Attribute. --- Userland/Libraries/LibWeb/DOM/Attribute.cpp | 11 ++++++++++ Userland/Libraries/LibWeb/DOM/Attribute.h | 7 +++--- .../Libraries/LibWeb/DOM/NamedNodeMap.cpp | 22 +++++++++++++++---- Userland/Libraries/LibWeb/DOM/NamedNodeMap.h | 3 ++- 4 files changed, 35 insertions(+), 8 deletions(-) diff --git a/Userland/Libraries/LibWeb/DOM/Attribute.cpp b/Userland/Libraries/LibWeb/DOM/Attribute.cpp index 9933e7f60e..4a870411c8 100644 --- a/Userland/Libraries/LibWeb/DOM/Attribute.cpp +++ b/Userland/Libraries/LibWeb/DOM/Attribute.cpp @@ -6,6 +6,7 @@ #include #include +#include namespace Web::DOM { @@ -22,4 +23,14 @@ Attribute::Attribute(Document& document, FlyString local_name, String value, Ele { } +Element const* Attribute::owner_element() const +{ + return m_owner_element; +} + +void Attribute::set_owner_element(Element const* owner_element) +{ + m_owner_element = owner_element; +} + } diff --git a/Userland/Libraries/LibWeb/DOM/Attribute.h b/Userland/Libraries/LibWeb/DOM/Attribute.h index 850bc4bfa6..2fa50fe72c 100644 --- a/Userland/Libraries/LibWeb/DOM/Attribute.h +++ b/Userland/Libraries/LibWeb/DOM/Attribute.h @@ -7,6 +7,7 @@ #pragma once #include +#include #include #include @@ -31,8 +32,8 @@ public: String const& value() const { return m_value; } void set_value(String value) { m_value = move(value); } - Element const* owner_element() const { return m_owner_element; } - void set_owner_element(Element const* owner_element) { m_owner_element = owner_element; } + Element const* owner_element() const; + void set_owner_element(Element const* owner_element); // Always returns true: https://dom.spec.whatwg.org/#dom-attr-specified constexpr bool specified() const { return true; } @@ -42,7 +43,7 @@ private: QualifiedName m_qualified_name; String m_value; - Element const* m_owner_element { nullptr }; + WeakPtr m_owner_element; }; } diff --git a/Userland/Libraries/LibWeb/DOM/NamedNodeMap.cpp b/Userland/Libraries/LibWeb/DOM/NamedNodeMap.cpp index 5d118ab483..e0670812ef 100644 --- a/Userland/Libraries/LibWeb/DOM/NamedNodeMap.cpp +++ b/Userland/Libraries/LibWeb/DOM/NamedNodeMap.cpp @@ -19,6 +19,8 @@ NonnullRefPtr NamedNodeMap::create(Element const& associated_eleme NamedNodeMap::NamedNodeMap(Element const& associated_element) : m_associated_element(associated_element) { + // Note: To avoid a reference cycle between Element, NamedNodeMap, and Attribute, do not store a + // strong reference to the associated element. } // https://dom.spec.whatwg.org/#ref-for-dfn-supported-property-indices%E2%91%A3 @@ -30,6 +32,10 @@ bool NamedNodeMap::is_supported_property_index(u32 index) const // https://dom.spec.whatwg.org/#ref-for-dfn-supported-property-names%E2%91%A0 Vector NamedNodeMap::supported_property_names() const { + auto associated_element = m_associated_element.strong_ref(); + if (!associated_element) + return {}; + // 1. Let names be the qualified names of the attributes in this NamedNodeMap object’s attribute list, with duplicates omitted, in order. Vector names; names.ensure_capacity(m_attributes.size()); @@ -41,7 +47,7 @@ Vector NamedNodeMap::supported_property_names() const // 2. If this NamedNodeMap object’s element is in the HTML namespace and its node document is an HTML document, then for each name in names: // FIXME: Handle the second condition, assume it is an HTML document for now. - if (m_associated_element.namespace_uri() == Namespace::HTML) { + if (associated_element->namespace_uri() == Namespace::HTML) { // 1. Let lowercaseName be name, in ASCII lowercase. // 2. If lowercaseName is not equal to name, remove name from names. names.remove_all_matching([](auto const& name) { return name != name.to_lowercase(); }); @@ -97,13 +103,17 @@ Attribute* NamedNodeMap::get_attribute(StringView qualified_name, size_t* item_i // https://dom.spec.whatwg.org/#concept-element-attributes-get-by-name Attribute const* NamedNodeMap::get_attribute(StringView qualified_name, size_t* item_index) const { + auto associated_element = m_associated_element.strong_ref(); + if (!associated_element) + return nullptr; + if (item_index) *item_index = 0; // 1. If element is in the HTML namespace and its node document is an HTML document, then set qualifiedName to qualifiedName in ASCII lowercase. // FIXME: Handle the second condition, assume it is an HTML document for now. Optional qualified_name_lowercase; - if (m_associated_element.namespace_uri() == Namespace::HTML) { + if (associated_element->namespace_uri() == Namespace::HTML) { qualified_name_lowercase = qualified_name.to_lowercase_string(); qualified_name = qualified_name_lowercase->view(); } @@ -122,8 +132,12 @@ Attribute const* NamedNodeMap::get_attribute(StringView qualified_name, size_t* // https://dom.spec.whatwg.org/#concept-element-attributes-set ExceptionOr NamedNodeMap::set_attribute(Attribute& attribute) { + auto associated_element = m_associated_element.strong_ref(); + if (!associated_element) + return nullptr; + // 1. If attr’s element is neither null nor element, throw an "InUseAttributeError" DOMException. - if ((attribute.owner_element() != nullptr) && (attribute.owner_element() != &m_associated_element)) + if ((attribute.owner_element() != nullptr) && (attribute.owner_element() != associated_element)) return InUseAttributeError::create("Attribute must not already be in use"sv); // 2. Let oldAttr be the result of getting an attribute given attr’s namespace, attr’s local name, and element. @@ -177,7 +191,7 @@ void NamedNodeMap::append_attribute(Attribute& attribute) m_attributes.append(attribute); // 3. Set attribute’s element to element. - attribute.set_owner_element(&m_associated_element); + attribute.set_owner_element(m_associated_element); } // https://dom.spec.whatwg.org/#concept-element-attributes-remove-by-name diff --git a/Userland/Libraries/LibWeb/DOM/NamedNodeMap.h b/Userland/Libraries/LibWeb/DOM/NamedNodeMap.h index b07b999bd1..e6bf62ebfa 100644 --- a/Userland/Libraries/LibWeb/DOM/NamedNodeMap.h +++ b/Userland/Libraries/LibWeb/DOM/NamedNodeMap.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -50,7 +51,7 @@ public: private: explicit NamedNodeMap(Element const& associated_element); - Element const& m_associated_element; + WeakPtr m_associated_element; NonnullRefPtrVector m_attributes; };