From 696cf7b9fb9a04fab02c63f49f141c19c76303c3 Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Wed, 3 Apr 2024 19:19:40 +0200 Subject: [PATCH] LibWeb: Fix "destroy the child navigable" to call `Document::destroy()` f66d33423bb3ce0526949668d6769e32f76d1908 was not sufficient to ensure document destruction when a child navigable is destroyed. This is because a navigable was remove from the set of all navigables too early which led to `Navigable::navigable_with_active_document()` being unable to find a navigable that is still in the process of destruction. This change solves that by making all steps of a navigable destruction to happen in afterAllDestruction callback. Unfortunately, writing a test to verify document destruction is challenging because no events are emitted to indicate that it has happened. --- .../LibWeb/HTML/NavigableContainer.cpp | 32 ++++++++++--------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/Userland/Libraries/LibWeb/HTML/NavigableContainer.cpp b/Userland/Libraries/LibWeb/HTML/NavigableContainer.cpp index 79df039ee37..609c1b4ce30 100644 --- a/Userland/Libraries/LibWeb/HTML/NavigableContainer.cpp +++ b/Userland/Libraries/LibWeb/HTML/NavigableContainer.cpp @@ -267,31 +267,33 @@ void NavigableContainer::destroy_the_child_navigable() if (navigable->has_been_destroyed()) return; navigable->set_has_been_destroyed(); - HTML::all_navigables().remove(navigable); // FIXME: 4. Inform the navigation API about child navigable destruction given navigable. // 5. Destroy a document and its descendants given navigable's active document. - navigable->active_document()->destroy_a_document_and_its_descendants([this] { + navigable->active_document()->destroy_a_document_and_its_descendants([this, navigable] { // 3. Set container's content navigable to null. m_content_navigable = nullptr; - }); - // 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(); + // Not in the spec: + HTML::all_navigables().remove(navigable); - // 7. Remove the nested history from parentDocState's nested histories whose id equals navigable's id. - parent_doc_state->nested_histories().remove_all_matching([&](auto& nested_history) { - return navigable->id() == nested_history.id; - }); + // 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(); - // 8. Let traversable be container's node navigable's traversable navigable. - auto traversable = this->navigable()->traversable_navigable(); + // 7. Remove the nested history from parentDocState's nested histories whose id equals navigable's id. + parent_doc_state->nested_histories().remove_all_matching([&](auto& nested_history) { + return navigable->id() == nested_history.id; + }); - // 9. Append the following session history traversal steps to traversable: - traversable->append_session_history_traversal_steps([traversable] { - // 1. Update for navigable creation/destruction given traversable. - traversable->update_for_navigable_creation_or_destruction(); + // 8. Let traversable be container's node navigable's traversable navigable. + auto traversable = this->navigable()->traversable_navigable(); + + // 9. Append the following session history traversal steps to traversable: + traversable->append_session_history_traversal_steps([traversable] { + // 1. Update for navigable creation/destruction given traversable. + traversable->update_for_navigable_creation_or_destruction(); + }); }); }