From 76397c9ecd61b865ae7352e173485f49f0af726f Mon Sep 17 00:00:00 2001 From: Shannon Booth Date: Fri, 17 Jan 2025 14:18:36 +1300 Subject: [PATCH] LibWeb: Use finalize for cleaning up all navigables The use of this HashMap looks very spooky, but let's at least use finalize when cleaning them up on destruction to make things slightly less dangerous looking. --- Libraries/LibWeb/DOM/Document.cpp | 4 ++-- Libraries/LibWeb/HTML/Navigable.cpp | 15 +++++++++------ Libraries/LibWeb/HTML/Navigable.h | 3 ++- Libraries/LibWeb/HTML/NavigableContainer.cpp | 2 +- Libraries/LibWeb/HTML/TraversableNavigable.cpp | 2 +- Libraries/LibWeb/WebDriver/Contexts.cpp | 2 +- Services/WebContent/WebDriverConnection.cpp | 2 +- 7 files changed, 17 insertions(+), 13 deletions(-) diff --git a/Libraries/LibWeb/DOM/Document.cpp b/Libraries/LibWeb/DOM/Document.cpp index 970db3ff3be..9855d73a7ba 100644 --- a/Libraries/LibWeb/DOM/Document.cpp +++ b/Libraries/LibWeb/DOM/Document.cpp @@ -3787,8 +3787,8 @@ void Document::destroy() // Not in the spec: for (auto& navigable_container : HTML::NavigableContainer::all_instances()) { - if (&navigable_container->document() == this) - HTML::all_navigables().remove(navigable_container->content_navigable()); + if (&navigable_container->document() == this && navigable_container->content_navigable()) + HTML::all_navigables().remove(*navigable_container->content_navigable()); } // 9. Set document's node navigable's active session history entry's document state's document to null. diff --git a/Libraries/LibWeb/HTML/Navigable.cpp b/Libraries/LibWeb/HTML/Navigable.cpp index 75bf763b09a..3d81830f1f0 100644 --- a/Libraries/LibWeb/HTML/Navigable.cpp +++ b/Libraries/LibWeb/HTML/Navigable.cpp @@ -73,9 +73,9 @@ private: GC_DEFINE_ALLOCATOR(ResponseHolder); -HashTable& all_navigables() +HashTable>& all_navigables() { - static HashTable set; + static HashTable> set; return set; } @@ -111,12 +111,15 @@ Navigable::Navigable(GC::Ref page) : m_page(page) , m_event_handler({}, *this) { - all_navigables().set(this); + all_navigables().set(*this); } -Navigable::~Navigable() +Navigable::~Navigable() = default; + +void Navigable::finalize() { - all_navigables().remove(this); + all_navigables().remove(*this); + Base::finalize(); } void Navigable::visit_edges(Cell::Visitor& visitor) @@ -159,7 +162,7 @@ void Navigable::set_delaying_load_events(bool value) GC::Ptr Navigable::navigable_with_active_document(GC::Ref document) { - for (auto* navigable : all_navigables()) { + for (auto navigable : all_navigables()) { if (navigable->active_document() == document) return navigable; } diff --git a/Libraries/LibWeb/HTML/Navigable.h b/Libraries/LibWeb/HTML/Navigable.h index 627ee960c76..97fad9db3db 100644 --- a/Libraries/LibWeb/HTML/Navigable.h +++ b/Libraries/LibWeb/HTML/Navigable.h @@ -192,6 +192,7 @@ protected: explicit Navigable(GC::Ref); virtual void visit_edges(Cell::Visitor&) override; + virtual void finalize() override; // https://html.spec.whatwg.org/multipage/browsing-the-web.html#ongoing-navigation Variant m_ongoing_navigation; @@ -236,7 +237,7 @@ private: Web::EventHandler m_event_handler; }; -HashTable& all_navigables(); +HashTable>& all_navigables(); bool navigation_must_be_a_replace(URL::URL const& url, DOM::Document const& document); void finalize_a_cross_document_navigation(GC::Ref, HistoryHandlingBehavior, UserNavigationInvolvement, GC::Ref); diff --git a/Libraries/LibWeb/HTML/NavigableContainer.cpp b/Libraries/LibWeb/HTML/NavigableContainer.cpp index 626e3d419ea..4796e2b0f45 100644 --- a/Libraries/LibWeb/HTML/NavigableContainer.cpp +++ b/Libraries/LibWeb/HTML/NavigableContainer.cpp @@ -283,7 +283,7 @@ void NavigableContainer::destroy_the_child_navigable() m_content_navigable = nullptr; // Not in the spec: - HTML::all_navigables().remove(navigable); + HTML::all_navigables().remove(*navigable); // 6. Let parentDocState be container's node navigable's active session history entry's document state. auto parent_doc_state = this->navigable()->active_session_history_entry()->document_state(); diff --git a/Libraries/LibWeb/HTML/TraversableNavigable.cpp b/Libraries/LibWeb/HTML/TraversableNavigable.cpp index df6e0b0e6d3..f27ecfbbe37 100644 --- a/Libraries/LibWeb/HTML/TraversableNavigable.cpp +++ b/Libraries/LibWeb/HTML/TraversableNavigable.cpp @@ -1268,7 +1268,7 @@ void TraversableNavigable::destroy_top_level_traversable() // FIXME: Figure out why we need to do this... we shouldn't be leaking Navigables for all time. // However, without this, we can keep stale destroyed traversables around. set_has_been_destroyed(); - all_navigables().remove(this); + all_navigables().remove(*this); } // https://html.spec.whatwg.org/multipage/browsing-the-web.html#finalize-a-same-document-navigation diff --git a/Libraries/LibWeb/WebDriver/Contexts.cpp b/Libraries/LibWeb/WebDriver/Contexts.cpp index 14c907e19a3..cb7fbd459c9 100644 --- a/Libraries/LibWeb/WebDriver/Contexts.cpp +++ b/Libraries/LibWeb/WebDriver/Contexts.cpp @@ -46,7 +46,7 @@ JsonObject window_proxy_reference_object(HTML::WindowProxy const& window) static GC::Ptr find_navigable_with_handle(StringView handle, bool should_be_top_level) { - for (auto* navigable : Web::HTML::all_navigables()) { + for (auto navigable : Web::HTML::all_navigables()) { if (navigable->is_top_level_traversable() != should_be_top_level) continue; diff --git a/Services/WebContent/WebDriverConnection.cpp b/Services/WebContent/WebDriverConnection.cpp index 42a03be2b5b..1ee29a7c3a9 100644 --- a/Services/WebContent/WebDriverConnection.cpp +++ b/Services/WebContent/WebDriverConnection.cpp @@ -581,7 +581,7 @@ Messages::WebDriverClient::SwitchToWindowResponse WebDriverConnection::switch_to // Otherwise, return error with error code no such window. bool found_matching_context = false; - for (auto* navigable : Web::HTML::all_navigables()) { + for (auto navigable : Web::HTML::all_navigables()) { auto traversable = navigable->top_level_traversable(); if (!traversable || !traversable->active_browsing_context()) continue;