From d3cfe35fbddf0e8d6c1b2750757965aa8c05f936 Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Fri, 19 Apr 2024 15:38:17 +0200 Subject: [PATCH] LibWeb: Do not destroy document until whole subtree completed unloading Fixes crashing when "unload" event handler tries to access active document that has already been destroyed. --- .../LibWeb/Text/data/iframe-unload-event.html | 10 +++ .../navigation/iframe-unloading-order.txt | 1 + .../navigation/iframe-unloading-order.html | 7 ++ Userland/Libraries/LibWeb/DOM/Document.cpp | 68 ++++++++++++------- Userland/Libraries/LibWeb/HTML/Navigable.cpp | 9 +++ Userland/Libraries/LibWeb/HTML/Navigable.h | 1 + 6 files changed, 70 insertions(+), 26 deletions(-) create mode 100644 Tests/LibWeb/Text/data/iframe-unload-event.html create mode 100644 Tests/LibWeb/Text/expected/navigation/iframe-unloading-order.txt create mode 100644 Tests/LibWeb/Text/input/navigation/iframe-unloading-order.html diff --git a/Tests/LibWeb/Text/data/iframe-unload-event.html b/Tests/LibWeb/Text/data/iframe-unload-event.html new file mode 100644 index 00000000000..3f3f2b9c43f --- /dev/null +++ b/Tests/LibWeb/Text/data/iframe-unload-event.html @@ -0,0 +1,10 @@ + + + diff --git a/Tests/LibWeb/Text/expected/navigation/iframe-unloading-order.txt b/Tests/LibWeb/Text/expected/navigation/iframe-unloading-order.txt new file mode 100644 index 00000000000..3e007972d44 --- /dev/null +++ b/Tests/LibWeb/Text/expected/navigation/iframe-unloading-order.txt @@ -0,0 +1 @@ + Test passes if it is possible to navigate away without crashing. diff --git a/Tests/LibWeb/Text/input/navigation/iframe-unloading-order.html b/Tests/LibWeb/Text/input/navigation/iframe-unloading-order.html new file mode 100644 index 00000000000..93ffd572dfc --- /dev/null +++ b/Tests/LibWeb/Text/input/navigation/iframe-unloading-order.html @@ -0,0 +1,7 @@ + + + diff --git a/Userland/Libraries/LibWeb/DOM/Document.cpp b/Userland/Libraries/LibWeb/DOM/Document.cpp index 5cf166ba853..c049ec4958e 100644 --- a/Userland/Libraries/LibWeb/DOM/Document.cpp +++ b/Userland/Libraries/LibWeb/DOM/Document.cpp @@ -3293,7 +3293,7 @@ void Document::unload(JS::GCPtr) // 19. If oldDocument's salvageable state is false, then destroy oldDocument. if (!m_salvageable) { - destroy(); + // NOTE: Document is destroyed from Document::unload_a_document_and_its_descendants() } // 20. Decrease oldDocument's unload counter by 1. @@ -3308,41 +3308,57 @@ void Document::unload(JS::GCPtr) // https://html.spec.whatwg.org/multipage/document-lifecycle.html#unload-a-document-and-its-descendants void Document::unload_a_document_and_its_descendants(JS::GCPtr new_document, JS::SafeFunction after_all_unloads) { - // 1. FIXME: Assert: this is running within document's node navigable's traversable navigable's session history traversal queue. + // Specification defines this algorithm in the following steps: + // 1. Recursively unload (and destroy) documents in descendant navigables + // 2. Unload (and destroy) this document. + // + // Implementation of the spec will fail in the following scenario: + // 1. Unload iframe's (has attribute name="test") document + // 1.1. Destroy iframe's document + // 2. Unload iframe's parent document + // 2.1. Dispatch "unload" event + // 2.2. In "unload" event handler run `window["test"]` + // 2.2.1. Execute Window::document_tree_child_navigable_target_name_property_set() + // 2.2.1.1. Fail to access iframe's navigable active document because it was destroyed on step 1.1 + // + // We change the algorithm to: + // 1. Unload all descendant documents without destroying them + // 2. Unload this document + // 3. Destroy all descendant documents + // 4. Destroy this document + // + // This way we maintain the invariant that all navigable containers present in the DOM tree + // have an active document while the document is being unloaded. - // 2. Let childNavigables be document's child navigables. - auto child_navigables = document_tree_child_navigables(); - - // 2. Let numberUnloaded be 0. IGNORE_USE_IN_ESCAPING_LAMBDA size_t number_unloaded = 0; - // Spec FIXME: in what order? - // 3. For each childNavigable of childNavigable's, queue a global task on the navigation and traversal task source - // given childNavigable's active window to perform the following steps: - for (auto& child_navigable : child_navigables) { - HTML::queue_global_task(HTML::Task::Source::NavigationAndTraversal, *child_navigable->active_window(), [&number_unloaded, child_navigable = child_navigable.ptr()] { - // 1. Let incrementUnloaded be an algorithm step which increments numberUnloaded. - auto increment_unloaded = [&number_unloaded] { ++number_unloaded; }; + auto navigable = this->navigable(); - // 2. Unload a document and its descendants given childNavigable's active document, null, and incrementUnloaded. - child_navigable->active_document()->unload_a_document_and_its_descendants(nullptr, move(increment_unloaded)); + Vector> descendant_navigables; + for (auto& other_navigable : HTML::all_navigables()) { + if (navigable->is_ancestor_of(*other_navigable)) + descendant_navigables.append(other_navigable); + } + + auto unloaded_documents_count = descendant_navigables.size() + 1; + + HTML::queue_global_task(HTML::Task::Source::NavigationAndTraversal, HTML::relevant_global_object(*this), [&number_unloaded, this, new_document] { + unload(new_document); + ++number_unloaded; + }); + + for (auto& descendant_navigable : descendant_navigables) { + HTML::queue_global_task(HTML::Task::Source::NavigationAndTraversal, *descendant_navigable->active_window(), [&number_unloaded, descendant_navigable = descendant_navigable.ptr()] { + descendant_navigable->active_document()->unload(); + ++number_unloaded; }); } - // 4. Wait until numberUnloaded equals childNavigable's size. HTML::main_thread_event_loop().spin_until([&] { - return number_unloaded == child_navigables.size(); + return number_unloaded == unloaded_documents_count; }); - // 5. Queue a global task on the navigation and traversal task source given document's relevant global object to perform the following steps: - HTML::queue_global_task(HTML::Task::Source::NavigationAndTraversal, HTML::relevant_global_object(*this), [this, new_document, after_all_unloads = move(after_all_unloads)] { - // 1. Unload document, passing along newDocument if it is not null. - unload(new_document); - - // 2. If afterAllUnloads was given, then run it. - if (after_all_unloads) - after_all_unloads(); - }); + destroy_a_document_and_its_descendants(move(after_all_unloads)); } // https://html.spec.whatwg.org/multipage/iframe-embed-object.html#allowed-to-use diff --git a/Userland/Libraries/LibWeb/HTML/Navigable.cpp b/Userland/Libraries/LibWeb/HTML/Navigable.cpp index c5b269298d2..2220a187115 100644 --- a/Userland/Libraries/LibWeb/HTML/Navigable.cpp +++ b/Userland/Libraries/LibWeb/HTML/Navigable.cpp @@ -92,6 +92,15 @@ bool Navigable::is_traversable() const return is(*this); } +bool Navigable::is_ancestor_of(JS::NonnullGCPtr other) const +{ + for (auto ancestor = other->parent(); ancestor; ancestor = ancestor->parent()) { + if (ancestor == this) + return true; + } + return false; +} + Navigable::Navigable() { all_navigables().set(this); diff --git a/Userland/Libraries/LibWeb/HTML/Navigable.h b/Userland/Libraries/LibWeb/HTML/Navigable.h index f44762074a1..6894eadded7 100644 --- a/Userland/Libraries/LibWeb/HTML/Navigable.h +++ b/Userland/Libraries/LibWeb/HTML/Navigable.h @@ -59,6 +59,7 @@ public: String const& id() const { return m_id; } JS::GCPtr parent() const { return m_parent; } + bool is_ancestor_of(JS::NonnullGCPtr) const; bool is_closing() const { return m_closing; } void set_closing(bool value) { m_closing = value; }