From 707ca984bdb1f7d098d9275e319c881d31e211df Mon Sep 17 00:00:00 2001
From: Aliaksandr Kalenik <kalenik.aliaksandr@gmail.com>
Date: Sun, 24 Sep 2023 18:13:39 +0200
Subject: [PATCH] LibWeb: Fix memory leak in CSS::ImageStyleValue

Before this change, whenever ImageStyleValue had a non-null
`m_image_request`, it was always leaked along with everything related
to the document to which this value belongs. The issue arises due to
the use of `JS::Handle` for the image request, as it introduces a
cyclic dependency where `ImageRequest` prevents the `CSSStyleSheet`,
that owns `ImageStyleValue`, from being deallocated:
- ImageRequest
- FetchController
- FetchParams
- Window
- HTMLDocument
- HTMLHtmlElement
- HTMLBodyElement
- Text
- HTMLHeadElement
- Text
- HTMLMetaElement
- Text
- HTMLTitleElement
- Text
- HTMLStyleElement
- CSSStyleSheet

This change solves this by visiting `m_image_request` from
`visit_edges` instead of introducing new heap root by using
`JS::Handle`.
---
 Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.cpp | 10 ++++++++++
 Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.h   |  2 ++
 .../LibWeb/CSS/StyleValues/ImageStyleValue.h          | 11 ++++++++++-
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.cpp b/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.cpp
index f2cc4cf384..693299f573 100644
--- a/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.cpp
+++ b/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.cpp
@@ -9,6 +9,7 @@
 #include <LibWeb/Bindings/Intrinsics.h>
 #include <LibWeb/CSS/CSSStyleDeclaration.h>
 #include <LibWeb/CSS/Parser/Parser.h>
+#include <LibWeb/CSS/StyleValues/ImageStyleValue.h>
 #include <LibWeb/DOM/Document.h>
 #include <LibWeb/DOM/Element.h>
 #include <LibWeb/Infra/Strings.h>
@@ -38,6 +39,15 @@ PropertyOwningCSSStyleDeclaration::PropertyOwningCSSStyleDeclaration(JS::Realm&
 {
 }
 
+void PropertyOwningCSSStyleDeclaration::visit_edges(Cell::Visitor& visitor)
+{
+    Base::visit_edges(visitor);
+    for (auto& property : m_properties) {
+        if (property.value->is_image())
+            property.value->as_image().visit_edges(visitor);
+    }
+}
+
 String PropertyOwningCSSStyleDeclaration::item(size_t index) const
 {
     if (index >= m_properties.size())
diff --git a/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.h b/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.h
index b3b0802cbc..25b0b2e071 100644
--- a/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.h
+++ b/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.h
@@ -85,6 +85,8 @@ protected:
 private:
     bool set_a_css_declaration(PropertyID, NonnullRefPtr<StyleValue const>, Important);
 
+    virtual void visit_edges(Cell::Visitor&) override;
+
     Vector<StyleProperty> m_properties;
     HashMap<DeprecatedString, StyleProperty> m_custom_properties;
 };
diff --git a/Userland/Libraries/LibWeb/CSS/StyleValues/ImageStyleValue.h b/Userland/Libraries/LibWeb/CSS/StyleValues/ImageStyleValue.h
index 6565d4d16d..5650085cf7 100644
--- a/Userland/Libraries/LibWeb/CSS/StyleValues/ImageStyleValue.h
+++ b/Userland/Libraries/LibWeb/CSS/StyleValues/ImageStyleValue.h
@@ -10,9 +10,11 @@
 #pragma once
 
 #include <AK/URL.h>
+#include <LibJS/Heap/Cell.h>
 #include <LibJS/Heap/Handle.h>
 #include <LibWeb/CSS/Enums.h>
 #include <LibWeb/CSS/StyleValues/AbstractImageStyleValue.h>
+#include <LibWeb/HTML/SharedImageRequest.h>
 
 namespace Web::CSS {
 
@@ -26,6 +28,13 @@ public:
     }
     virtual ~ImageStyleValue() override = default;
 
+    void visit_edges(JS::Cell::Visitor& visitor) const
+    {
+        // FIXME: visit_edges in non-GC allocated classes is confusing pattern.
+        //        Consider making StyleValue to be GC allocated instead.
+        visitor.visit(m_image_request);
+    }
+
     virtual String to_string() const override;
     virtual bool equals(StyleValue const& other) const override;
 
@@ -44,7 +53,7 @@ public:
 private:
     ImageStyleValue(AK::URL const&);
 
-    JS::Handle<HTML::SharedImageRequest> m_image_request;
+    JS::GCPtr<HTML::SharedImageRequest> m_image_request;
 
     void animate();
     Gfx::Bitmap const* bitmap(size_t frame_index, Gfx::IntSize = {}) const;