From 934afcb9d522e09dc51f5b828c2ea298a04a441a Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Fri, 18 Aug 2023 14:11:55 +0200 Subject: [PATCH] LibWeb: Make HTML::SharedImageRequest GC allocated This allows to partially solve the problem of cyclic dependency between HTMLImageElement and SharedImageRequest that prevents all image elements from being deallocated. --- .../LibWeb/CSS/StyleValues/ImageStyleValue.cpp | 2 +- .../LibWeb/CSS/StyleValues/ImageStyleValue.h | 3 ++- Userland/Libraries/LibWeb/HTML/HTMLImageElement.cpp | 10 +++++----- Userland/Libraries/LibWeb/HTML/HTMLObjectElement.cpp | 8 +++++++- Userland/Libraries/LibWeb/HTML/HTMLObjectElement.h | 4 +++- Userland/Libraries/LibWeb/HTML/ImageRequest.cpp | 10 ++++++++-- Userland/Libraries/LibWeb/HTML/ImageRequest.h | 6 ++++-- Userland/Libraries/LibWeb/HTML/SharedImageRequest.cpp | 10 ++++++++-- Userland/Libraries/LibWeb/HTML/SharedImageRequest.h | 11 ++++++++--- 9 files changed, 46 insertions(+), 18 deletions(-) diff --git a/Userland/Libraries/LibWeb/CSS/StyleValues/ImageStyleValue.cpp b/Userland/Libraries/LibWeb/CSS/StyleValues/ImageStyleValue.cpp index cba3190eb45..3e02996b01b 100644 --- a/Userland/Libraries/LibWeb/CSS/StyleValues/ImageStyleValue.cpp +++ b/Userland/Libraries/LibWeb/CSS/StyleValues/ImageStyleValue.cpp @@ -31,7 +31,7 @@ void ImageStyleValue::load_any_resources(DOM::Document& document) return; m_document = &document; - m_image_request = HTML::SharedImageRequest::get_or_create(*document.page(), m_url).release_value_but_fixme_should_propagate_errors(); + m_image_request = HTML::SharedImageRequest::get_or_create(document.realm(), *document.page(), m_url); m_image_request->add_callbacks( [this, weak_this = make_weak_ptr()] { if (!weak_this) diff --git a/Userland/Libraries/LibWeb/CSS/StyleValues/ImageStyleValue.h b/Userland/Libraries/LibWeb/CSS/StyleValues/ImageStyleValue.h index 93462fc03b5..300adea755e 100644 --- a/Userland/Libraries/LibWeb/CSS/StyleValues/ImageStyleValue.h +++ b/Userland/Libraries/LibWeb/CSS/StyleValues/ImageStyleValue.h @@ -10,6 +10,7 @@ #pragma once #include +#include #include #include @@ -43,7 +44,7 @@ public: private: ImageStyleValue(AK::URL const&); - RefPtr m_image_request; + JS::Handle m_image_request; void animate(); Gfx::Bitmap const* bitmap(size_t frame_index, Gfx::IntSize = {}) const; diff --git a/Userland/Libraries/LibWeb/HTML/HTMLImageElement.cpp b/Userland/Libraries/LibWeb/HTML/HTMLImageElement.cpp index 306816133d1..08310a58210 100644 --- a/Userland/Libraries/LibWeb/HTML/HTMLImageElement.cpp +++ b/Userland/Libraries/LibWeb/HTML/HTMLImageElement.cpp @@ -394,7 +394,7 @@ ErrorOr HTMLImageElement::update_the_image_data(bool restart_animations, b restart_the_animation(); // 2. Set current request's current URL to urlString. - m_current_request->set_current_url(url_string); + m_current_request->set_current_url(realm(), url_string); // 3. If maybe omit events is not set or previousURL is not equal to urlString, then fire an event named load at the img element. if (!maybe_omit_events || previous_url != url_string) @@ -433,7 +433,7 @@ after_step_7: // 2. Queue an element task on the DOM manipulation task source given the img element and the following steps: queue_an_element_task(HTML::Task::Source::DOMManipulation, [this, maybe_omit_events, previous_url] { // 1. Change the current request's current URL to the empty string. - m_current_request->set_current_url(""sv); + m_current_request->set_current_url(realm(), ""sv); // 2. If all of the following conditions are true: // - the element has a src attribute or it uses srcset or picture; and @@ -466,7 +466,7 @@ after_step_7: // 4. Queue an element task on the DOM manipulation task source given the img element and the following steps: queue_an_element_task(HTML::Task::Source::DOMManipulation, [this, selected_source, maybe_omit_events, previous_url] { // 1. Change the current request's current URL to selected source. - m_current_request->set_current_url(selected_source.value().url); + m_current_request->set_current_url(realm(), selected_source.value().url); // 2. If maybe omit events is not set or previousURL is not equal to selected source, then fire an event named error at the img element. if (!maybe_omit_events || previous_url != selected_source.value().url) @@ -503,7 +503,7 @@ after_step_7: // 16. Set image request to a new image request whose current URL is urlString. auto image_request = ImageRequest::create(realm(), *document().page()); - image_request->set_current_url(url_string); + image_request->set_current_url(realm(), url_string); // 17. If current request's state is unavailable or broken, then set the current request to image request. // Otherwise, set the pending request to image request. @@ -698,7 +698,7 @@ void HTMLImageElement::react_to_changes_in_the_environment() // 11. ⌛ Let image request be a new image request whose current URL is urlString auto image_request = ImageRequest::create(realm(), *document().page()); - image_request->set_current_url(url_string); + image_request->set_current_url(realm(), url_string); // 12. ⌛ Let the element's pending request be image request. m_pending_request = image_request; diff --git a/Userland/Libraries/LibWeb/HTML/HTMLObjectElement.cpp b/Userland/Libraries/LibWeb/HTML/HTMLObjectElement.cpp index c7eecea8a5b..0fa3c68e1fb 100644 --- a/Userland/Libraries/LibWeb/HTML/HTMLObjectElement.cpp +++ b/Userland/Libraries/LibWeb/HTML/HTMLObjectElement.cpp @@ -39,6 +39,12 @@ void HTMLObjectElement::initialize(JS::Realm& realm) set_prototype(&Bindings::ensure_web_prototype(realm, "HTMLObjectElement")); } +void HTMLObjectElement::visit_edges(Cell::Visitor& visitor) +{ + Base::visit_edges(visitor); + visitor.visit(m_image_request); +} + void HTMLObjectElement::attribute_changed(DeprecatedFlyString const& name, DeprecatedString const& value) { NavigableContainer::attribute_changed(name, value); @@ -316,7 +322,7 @@ void HTMLObjectElement::load_image() // NOTE: This currently reloads the image instead of reusing the resource we've already downloaded. auto data = attribute(HTML::AttributeNames::data); auto url = document().parse_url(data); - m_image_request = HTML::SharedImageRequest::get_or_create(*document().page(), url).release_value_but_fixme_should_propagate_errors(); + m_image_request = HTML::SharedImageRequest::get_or_create(realm(), *document().page(), url); m_image_request->add_callbacks( [this] { run_object_representation_completed_steps(Representation::Image); diff --git a/Userland/Libraries/LibWeb/HTML/HTMLObjectElement.h b/Userland/Libraries/LibWeb/HTML/HTMLObjectElement.h index bd750edb67a..ef5e000aaa7 100644 --- a/Userland/Libraries/LibWeb/HTML/HTMLObjectElement.h +++ b/Userland/Libraries/LibWeb/HTML/HTMLObjectElement.h @@ -45,6 +45,8 @@ public: // https://html.spec.whatwg.org/multipage/forms.html#category-listed virtual bool is_listed() const override { return true; } + virtual void visit_edges(Cell::Visitor&) override; + private: HTMLObjectElement(DOM::Document&, DOM::QualifiedName); @@ -80,7 +82,7 @@ private: RefPtr image_data() const; - RefPtr m_image_request; + JS::GCPtr m_image_request; }; } diff --git a/Userland/Libraries/LibWeb/HTML/ImageRequest.cpp b/Userland/Libraries/LibWeb/HTML/ImageRequest.cpp index fad3b6a1e09..09d577702d5 100644 --- a/Userland/Libraries/LibWeb/HTML/ImageRequest.cpp +++ b/Userland/Libraries/LibWeb/HTML/ImageRequest.cpp @@ -33,6 +33,12 @@ ImageRequest::~ImageRequest() { } +void ImageRequest::visit_edges(JS::Cell::Visitor& visitor) +{ + Base::visit_edges(visitor); + visitor.visit(m_shared_image_request); +} + // https://html.spec.whatwg.org/multipage/images.html#img-available bool ImageRequest::is_available() const { @@ -60,11 +66,11 @@ AK::URL const& ImageRequest::current_url() const return m_current_url; } -void ImageRequest::set_current_url(AK::URL url) +void ImageRequest::set_current_url(JS::Realm& realm, AK::URL url) { m_current_url = move(url); if (m_current_url.is_valid()) - m_shared_image_request = SharedImageRequest::get_or_create(m_page, m_current_url).release_value_but_fixme_should_propagate_errors(); + m_shared_image_request = SharedImageRequest::get_or_create(realm, m_page, m_current_url); } // https://html.spec.whatwg.org/multipage/images.html#abort-the-image-request diff --git a/Userland/Libraries/LibWeb/HTML/ImageRequest.h b/Userland/Libraries/LibWeb/HTML/ImageRequest.h index cdb62c08add..c97812c9183 100644 --- a/Userland/Libraries/LibWeb/HTML/ImageRequest.h +++ b/Userland/Libraries/LibWeb/HTML/ImageRequest.h @@ -40,7 +40,7 @@ public: void set_state(State); AK::URL const& current_url() const; - void set_current_url(AK::URL); + void set_current_url(JS::Realm&, AK::URL); [[nodiscard]] RefPtr image_data() const; void set_image_data(RefPtr); @@ -59,6 +59,8 @@ public: SharedImageRequest const* shared_image_request() const { return m_shared_image_request; } + virtual void visit_edges(JS::Cell::Visitor&) override; + private: explicit ImageRequest(Page&); @@ -84,7 +86,7 @@ private: // which is either a struct consisting of a width and a height or is null. It must initially be null. Optional m_preferred_density_corrected_dimensions; - RefPtr m_shared_image_request; + JS::GCPtr m_shared_image_request; }; // https://html.spec.whatwg.org/multipage/images.html#abort-the-image-request diff --git a/Userland/Libraries/LibWeb/HTML/SharedImageRequest.cpp b/Userland/Libraries/LibWeb/HTML/SharedImageRequest.cpp index 235d7f3f69e..e4a10df1674 100644 --- a/Userland/Libraries/LibWeb/HTML/SharedImageRequest.cpp +++ b/Userland/Libraries/LibWeb/HTML/SharedImageRequest.cpp @@ -24,11 +24,11 @@ static HashMap& shared_image_requests() return requests; } -ErrorOr> SharedImageRequest::get_or_create(Page& page, AK::URL const& url) +JS::NonnullGCPtr SharedImageRequest::get_or_create(JS::Realm& realm, Page& page, AK::URL const& url) { if (auto it = shared_image_requests().find(url); it != shared_image_requests().end()) return *it->value; - auto request = TRY(adopt_nonnull_ref_or_enomem(new (nothrow) SharedImageRequest(page, url))); + auto request = realm.heap().allocate(realm, page, url); shared_image_requests().set(url, request); return request; } @@ -44,6 +44,12 @@ SharedImageRequest::~SharedImageRequest() shared_image_requests().remove(m_url); } +void SharedImageRequest::visit_edges(JS::Cell::Visitor& visitor) +{ + Base::visit_edges(visitor); + visitor.visit(m_fetch_controller); +} + RefPtr SharedImageRequest::image_data() const { return m_image_data; diff --git a/Userland/Libraries/LibWeb/HTML/SharedImageRequest.h b/Userland/Libraries/LibWeb/HTML/SharedImageRequest.h index 1cd9dee9593..e788ef534e8 100644 --- a/Userland/Libraries/LibWeb/HTML/SharedImageRequest.h +++ b/Userland/Libraries/LibWeb/HTML/SharedImageRequest.h @@ -16,9 +16,12 @@ namespace Web::HTML { -class SharedImageRequest : public RefCounted { +class SharedImageRequest : public JS::Cell { + JS_CELL(ImageRequest, JS::Cell); + public: - static ErrorOr> get_or_create(Page&, AK::URL const&); + [[nodiscard]] static JS::NonnullGCPtr get_or_create(JS::Realm&, Page&, AK::URL const&); + ~SharedImageRequest(); AK::URL const& url() const { return m_url; } @@ -35,6 +38,8 @@ public: bool is_fetching() const; bool needs_fetching() const; + virtual void visit_edges(JS::Cell::Visitor&) override; + private: explicit SharedImageRequest(Page&, AK::URL); @@ -60,7 +65,7 @@ private: AK::URL m_url; RefPtr m_image_data; - JS::Handle m_fetch_controller; + JS::GCPtr m_fetch_controller; }; }