From 4cbf47dcd2fa3dc1e84f974c7d407f31f06a363e Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Wed, 30 Jul 2025 00:25:12 +0200 Subject: [PATCH] LibWeb: Unregister ResizeObserver from Document when it has no targets According to the spec, `ResizeObserver` needs to live for as long as it's referenced from script or has observation targets. With this change we make sure that `ResizeObserver` is unregistered from the `Document` when it has no target. Fixes GC leak that caused us to keep all resize observers alive until document they belong to is destroyed. --- Libraries/LibWeb/DOM/Document.cpp | 33 ++++++++++--------- Libraries/LibWeb/DOM/Document.h | 3 +- .../LibWeb/ResizeObserver/ResizeObserver.cpp | 28 ++++++++++++++-- .../LibWeb/ResizeObserver/ResizeObserver.h | 9 ++++- 4 files changed, 53 insertions(+), 20 deletions(-) diff --git a/Libraries/LibWeb/DOM/Document.cpp b/Libraries/LibWeb/DOM/Document.cpp index a8f10fd0df8..0f3ccb4a411 100644 --- a/Libraries/LibWeb/DOM/Document.cpp +++ b/Libraries/LibWeb/DOM/Document.cpp @@ -584,7 +584,9 @@ void Document::visit_edges(Cell::Visitor& visitor) visitor.visit(m_document_observers_being_notified); visitor.visit(m_pending_scroll_event_targets); visitor.visit(m_pending_scrollend_event_targets); - visitor.visit(m_resize_observers); + for (auto& resize_observer : m_resize_observers) { + visitor.visit(resize_observer); + } visitor.visit(m_shared_resource_requests); @@ -4595,9 +4597,7 @@ void Document::register_resize_observer(Badge, R void Document::unregister_resize_observer(Badge, ResizeObserver::ResizeObserver& observer) { - m_resize_observers.remove_first_matching([&](auto& registered_observer) { - return registered_observer.ptr() == &observer; - }); + m_resize_observers.remove(observer); } // https://www.w3.org/TR/intersection-observer/#queue-an-intersection-observer-task @@ -5787,13 +5787,13 @@ void Document::gather_active_observations_at_depth(size_t depth) // 1. Let depth be the depth passed in. // 2. For each observer in [[resizeObservers]] run these steps: - for (auto const& observer : m_resize_observers) { + for (auto& observer : m_resize_observers) { // 1. Clear observer’s [[activeTargets]], and [[skippedTargets]]. - observer->active_targets().clear(); - observer->skipped_targets().clear(); + observer.active_targets().clear(); + observer.skipped_targets().clear(); // 2. For each observation in observer.[[observationTargets]] run this step: - for (auto const& observation : observer->observation_targets()) { + for (auto& observation : observer.observation_targets()) { // 1. If observation.isActive() is true if (observation->is_active()) { // 1. Let targetDepth be result of calculate depth for node for observation.target. @@ -5801,10 +5801,10 @@ void Document::gather_active_observations_at_depth(size_t depth) // 2. If targetDepth is greater than depth then add observation to [[activeTargets]]. if (target_depth > depth) { - observer->active_targets().append(observation); + observer.active_targets().append(observation); } else { // 3. Else add observation to [[skippedTargets]]. - observer->skipped_targets().append(observation); + observer.skipped_targets().append(observation); } } } @@ -5820,7 +5820,10 @@ size_t Document::broadcast_active_resize_observations() // 2. For each observer in document.[[resizeObservers]] run these steps: // NOTE: We make a copy of the resize observers list to avoid modifying it while iterating. - auto resize_observers = GC::RootVector { heap(), m_resize_observers }; + auto resize_observers = GC::RootVector> { heap() }; + for (auto& observer : m_resize_observers) + resize_observers.append(observer); + for (auto const& observer : resize_observers) { // 1. If observer.[[activeTargets]] slot is empty, continue. if (observer->active_targets().is_empty()) { @@ -5878,9 +5881,9 @@ size_t Document::broadcast_active_resize_observations() bool Document::has_active_resize_observations() { // 1. For each observer in [[resizeObservers]] run this step: - for (auto const& observer : m_resize_observers) { + for (auto& observer : m_resize_observers) { // 1. If observer.[[activeTargets]] is not empty, return true. - if (!observer->active_targets().is_empty()) + if (!observer.active_targets().is_empty()) return true; } @@ -5892,9 +5895,9 @@ bool Document::has_active_resize_observations() bool Document::has_skipped_resize_observations() { // 1. For each observer in [[resizeObservers]] run this step: - for (auto const& observer : m_resize_observers) { + for (auto& observer : m_resize_observers) { // 1. If observer.[[skippedTargets]] is not empty, return true. - if (!observer->skipped_targets().is_empty()) + if (!observer.skipped_targets().is_empty()) return true; } diff --git a/Libraries/LibWeb/DOM/Document.h b/Libraries/LibWeb/DOM/Document.h index 75a85a77863..3c9b93f94dd 100644 --- a/Libraries/LibWeb/DOM/Document.h +++ b/Libraries/LibWeb/DOM/Document.h @@ -39,6 +39,7 @@ #include #include #include +#include #include #include #include @@ -1157,7 +1158,7 @@ private: // Each Document has a lazy load intersection observer, initially set to null but can be set to an IntersectionObserver instance. GC::Ptr m_lazy_load_intersection_observer; - Vector> m_resize_observers; + ResizeObserver::ResizeObserver::ResizeObserversList m_resize_observers; // https://html.spec.whatwg.org/multipage/semantics.html#will-declaratively-refresh // A Document object has an associated will declaratively refresh (a boolean). It is initially false. diff --git a/Libraries/LibWeb/ResizeObserver/ResizeObserver.cpp b/Libraries/LibWeb/ResizeObserver/ResizeObserver.cpp index 007fd94de8f..c8398e450fd 100644 --- a/Libraries/LibWeb/ResizeObserver/ResizeObserver.cpp +++ b/Libraries/LibWeb/ResizeObserver/ResizeObserver.cpp @@ -1,6 +1,6 @@ /* * Copyright (c) 2021, Andreas Kling - * Copyright (c) 2024, Aliaksandr Kalenik + * Copyright (c) 2024-2025, Aliaksandr Kalenik * * SPDX-License-Identifier: BSD-2-Clause */ @@ -29,7 +29,6 @@ ResizeObserver::ResizeObserver(JS::Realm& realm, WebIDL::CallbackType* callback) { auto navigable = as(HTML::relevant_global_object(*this)).navigable(); m_document = navigable->active_document().ptr(); - m_document->register_resize_observer({}, *this); } ResizeObserver::~ResizeObserver() = default; @@ -51,7 +50,7 @@ void ResizeObserver::visit_edges(JS::Cell::Visitor& visitor) void ResizeObserver::finalize() { - if (m_document) + if (m_document && m_list_node.is_in_list()) m_document->unregister_resize_observer({}, *this); } @@ -71,6 +70,10 @@ void ResizeObserver::observe(DOM::Element& target, ResizeObserverOptions options // 4. Add the resizeObservation to the [[observationTargets]] slot. m_observation_targets.append(resize_observation); + + if (!m_list_node.is_in_list()) { + m_document->register_resize_observer({}, *this); + } } // https://drafts.csswg.org/resize-observer-1/#dom-resizeobserver-unobserve @@ -85,6 +88,8 @@ void ResizeObserver::unobserve(DOM::Element& target) // 3. Remove observation from [[observationTargets]]. m_observation_targets.remove(observation.index()); + + unregister_observer_if_needed(); } // https://drafts.csswg.org/resize-observer-1/#dom-resizeobserver-disconnect @@ -95,6 +100,8 @@ void ResizeObserver::disconnect() // 2. Clear the [[activeTargets]] list. m_active_targets.clear(); + + unregister_observer_if_needed(); } void ResizeObserver::invoke_callback(ReadonlySpan> entries) const @@ -112,4 +119,19 @@ void ResizeObserver::invoke_callback(ReadonlySpan> (void)WebIDL::invoke_callback(callback, JS::js_undefined(), WebIDL::ExceptionBehavior::Report, { { wrapped_records } }); } +void ResizeObserver::unregister_observer_if_needed() +{ + // https://drafts.csswg.org/resize-observer/#lifetime + // A ResizeObserver will remain alive until both of these conditions are met: + // - there are no scripting references to the observer. + // - the observer is not observing any targets. + + // The first condition from the spec is handled by visiting ResizeObserver from + // JS environment that holds a reference to ResizeObserver. + // Here we handle the second condition. + if (m_observation_targets.is_empty() && m_list_node.is_in_list() && m_document) { + m_document->unregister_resize_observer({}, *this); + } +} + } diff --git a/Libraries/LibWeb/ResizeObserver/ResizeObserver.h b/Libraries/LibWeb/ResizeObserver/ResizeObserver.h index b991c257ad6..b5b6ab1592a 100644 --- a/Libraries/LibWeb/ResizeObserver/ResizeObserver.h +++ b/Libraries/LibWeb/ResizeObserver/ResizeObserver.h @@ -1,6 +1,6 @@ /* * Copyright (c) 2021, Andreas Kling - * Copyright (c) 2024, Aliaksandr Kalenik + * Copyright (c) 2024-2025, Aliaksandr Kalenik * * SPDX-License-Identifier: BSD-2-Clause */ @@ -44,6 +44,8 @@ private: virtual void visit_edges(JS::Cell::Visitor&) override; virtual void finalize() override; + void unregister_observer_if_needed(); + GC::Ptr m_callback; Vector> m_observation_targets; Vector> m_active_targets; @@ -51,6 +53,11 @@ private: // AD-HOC: This is the document where we've registered the observer. WeakPtr m_document; + + IntrusiveListNode m_list_node; + +public: + using ResizeObserversList = IntrusiveList<&ResizeObserver::m_list_node>; }; }