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

@@ -6,6 +6,7 @@
 
 #include <LibWeb/DOM/Attribute.h>
 #include <LibWeb/DOM/Document.h>
+#include <LibWeb/DOM/Element.h>
 
 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;
+}
+
 }

Userland/Libraries/LibWeb/DOM/Attribute.h

@@ -7,6 +7,7 @@
 #pragma once
 
 #include <AK/FlyString.h>
+#include <AK/WeakPtr.h>
 #include <LibWeb/DOM/Node.h>
 #include <LibWeb/QualifiedName.h>
 
@@ -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<Element> m_owner_element;
 };
 
 }

Userland/Libraries/LibWeb/DOM/NamedNodeMap.cpp

@@ -19,6 +19,8 @@ NonnullRefPtr<NamedNodeMap> 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<String> 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<String> names;
     names.ensure_capacity(m_attributes.size());
@@ -41,7 +47,7 @@ Vector<String> 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<String> 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<Attribute const*> 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

Userland/Libraries/LibWeb/DOM/NamedNodeMap.h

@@ -10,6 +10,7 @@
 #include <AK/RefCounted.h>
 #include <AK/String.h>
 #include <AK/StringView.h>
+#include <AK/WeakPtr.h>
 #include <LibWeb/Bindings/Wrappable.h>
 #include <LibWeb/DOM/ExceptionOr.h>
 #include <LibWeb/Forward.h>
@@ -50,7 +51,7 @@ public:
 private:
     explicit NamedNodeMap(Element const& associated_element);
 
-    Element const& m_associated_element;
+    WeakPtr<Element> m_associated_element;
     NonnullRefPtrVector<Attribute> m_attributes;
 };