Skip to content

Commit ed9c0c1

Browse files
LibWeb: Don't visit registered NavigationObserver from Navigable
NavigationObserver register itself in Navigable from constructor and unregister itself from `finalize()`. The problem is that `finalize()` won't be invoked for as long as NavigationObserver is visited by Navigable, leading to GC leaks.
1 parent 167de08 commit ed9c0c1

File tree

3 files changed

+11
-9
lines changed

3 files changed

+11
-9
lines changed

Libraries/LibWeb/HTML/Navigable.cpp

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,6 @@ void Navigable::visit_edges(Cell::Visitor& visitor)
176176
visitor.visit(m_current_session_history_entry);
177177
visitor.visit(m_active_session_history_entry);
178178
visitor.visit(m_container);
179-
visitor.visit(m_navigation_observers);
180179
visitor.visit(m_backing_store_manager);
181180
m_event_handler.visit_edges(visitor);
182181

@@ -293,9 +292,9 @@ void Navigable::activate_history_entry(GC::Ptr<SessionHistoryEntry> entry)
293292
new_document->make_active();
294293

295294
if (m_ongoing_navigation.has<Empty>()) {
296-
for (auto navigation_observer : m_navigation_observers) {
297-
if (navigation_observer->navigation_complete())
298-
navigation_observer->navigation_complete()->function()();
295+
for (auto& navigation_observer : m_navigation_observers) {
296+
if (navigation_observer.navigation_complete())
297+
navigation_observer.navigation_complete()->function()();
299298
}
300299
}
301300
}
@@ -2591,14 +2590,12 @@ CSSPixelSize Navigable::snapshot_containing_block_size()
25912590

25922591
void Navigable::register_navigation_observer(Badge<NavigationObserver>, NavigationObserver& navigation_observer)
25932592
{
2594-
auto result = m_navigation_observers.set(navigation_observer);
2595-
VERIFY(result == AK::HashSetResult::InsertedNewEntry);
2593+
m_navigation_observers.append(navigation_observer);
25962594
}
25972595

25982596
void Navigable::unregister_navigation_observer(Badge<NavigationObserver>, NavigationObserver& navigation_observer)
25992597
{
2600-
bool was_removed = m_navigation_observers.remove(navigation_observer);
2601-
VERIFY(was_removed);
2598+
m_navigation_observers.remove(navigation_observer);
26022599
}
26032600

26042601
// https://html.spec.whatwg.org/multipage/document-lifecycle.html#nav-stop

Libraries/LibWeb/HTML/Navigable.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#include <LibWeb/HTML/ActivateTab.h>
1919
#include <LibWeb/HTML/HistoryHandlingBehavior.h>
2020
#include <LibWeb/HTML/InitialInsertion.h>
21+
#include <LibWeb/HTML/NavigationObserver.h>
2122
#include <LibWeb/HTML/NavigationParams.h>
2223
#include <LibWeb/HTML/POSTResource.h>
2324
#include <LibWeb/HTML/RenderingThread.h>
@@ -263,7 +264,7 @@ class WEB_API Navigable : public JS::Cell {
263264

264265
GC::Ref<Page> m_page;
265266

266-
HashTable<GC::Ref<NavigationObserver>> m_navigation_observers;
267+
NavigationObserver::NavigationObserversList m_navigation_observers;
267268

268269
bool m_has_been_destroyed { false };
269270

Libraries/LibWeb/HTML/NavigationObserver.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,12 @@ class WEB_API NavigationObserver final : public Bindings::PlatformObject {
2828
virtual void visit_edges(Cell::Visitor&) override;
2929
virtual void finalize() override;
3030

31+
IntrusiveListNode<NavigationObserver> m_list_node;
3132
GC::Ref<Navigable> m_navigable;
3233
GC::Ptr<GC::Function<void()>> m_navigation_complete;
34+
35+
public:
36+
using NavigationObserversList = IntrusiveList<&NavigationObserver::m_list_node>;
3337
};
3438

3539
}

0 commit comments

Comments
 (0)