From c437f16cc1fedea28e53b7e378afdd01bbb657d4 Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Tue, 5 Sep 2023 23:36:20 +0200 Subject: [PATCH] LibWeb: Early return navigation process if navigable has been destroyed If a navigable has been destroyed during a navigation process, we should early return from it. The introduced checks are not in the spec because, as explained in https://github.com/whatwg/html/issues/9690 the spec is not written with such a level of detail. --- Userland/Libraries/LibWeb/HTML/Navigable.cpp | 24 +++++++++++++++++-- Userland/Libraries/LibWeb/HTML/Navigable.h | 8 +++++++ .../LibWeb/HTML/NavigableContainer.cpp | 4 ++++ .../LibWeb/HTML/TraversableNavigable.cpp | 16 +++++++++++++ 4 files changed, 50 insertions(+), 2 deletions(-) diff --git a/Userland/Libraries/LibWeb/HTML/Navigable.cpp b/Userland/Libraries/LibWeb/HTML/Navigable.cpp index ac44083677..78dd70925c 100644 --- a/Userland/Libraries/LibWeb/HTML/Navigable.cpp +++ b/Userland/Libraries/LibWeb/HTML/Navigable.cpp @@ -51,7 +51,7 @@ private: JS::GCPtr m_response; }; -static HashTable& all_navigables() +HashTable& all_navigables() { static HashTable set; return set; @@ -728,8 +728,16 @@ WebIDL::ExceptionOr Navigable::populate_session_history_entry_document(JS: } } + // NOTE: Not in the spec but queuing task on the next step will fail because active_window() does not exist for destroyed navigable. + if (has_been_destroyed()) + return {}; + // 6. Queue a global task on the navigation and traversal task source, given navigable's active window, to run these steps: queue_global_task(Task::Source::NavigationAndTraversal, *active_window(), [this, entry, navigation_params, navigation_id, completion_steps = move(completion_steps)] { + // NOTE: This check is not in the spec but we should not continue navigation if navigable has been destroyed. + if (has_been_destroyed()) + return; + // 1. If navigable's ongoing navigation no longer equals navigationId, then run completionSteps and return. if (navigation_id.has_value() && (!ongoing_navigation().has() || ongoing_navigation().get() != *navigation_id)) { completion_steps(); @@ -981,6 +989,10 @@ WebIDL::ExceptionOr Navigable::navigate( // 20. In parallel, run these steps: Platform::EventLoopPlugin::the().deferred_invoke([this, source_snapshot_params = move(source_snapshot_params), document_resource, url, navigation_id, referrer_policy, initiator_origin_snapshot, response, history_handling, initiator_base_url_snapshot] { + // NOTE: Not in the spec but subsequent steps will fail because destroyed navigable does not have active document. + if (has_been_destroyed()) + return; + // FIXME: 1. Let unloadPromptCanceled be the result of checking if unloading is user-canceled for navigable's active document's inclusive descendant navigables. // FIXME: 2. If unloadPromptCanceled is true, or navigable's ongoing navigation is no longer navigationId, then: @@ -1035,7 +1047,11 @@ WebIDL::ExceptionOr Navigable::navigate( // targetSnapshotParams, navigationId, navigationParams, cspNavigationType, with allowPOST // set to true and completionSteps set to the following step: populate_session_history_entry_document(history_entry, navigation_params, navigation_id, source_snapshot_params, true, [this, history_entry, history_handling, navigation_id] { - traversable_navigable()->append_session_history_traversal_steps([this, history_entry, history_handling] { + traversable_navigable()->append_session_history_traversal_steps([this, history_entry, history_handling, navigation_id] { + if (this->has_been_destroyed()) { + // NOTE: This check is not in the spec but we should not continue navigation if navigable has been destroyed. + return; + } finalize_a_cross_document_navigation(*this, to_history_handling_behavior(history_handling), history_entry); }); }).release_value_but_fixme_should_propagate_errors(); @@ -1379,6 +1395,10 @@ TargetSnapshotParams Navigable::snapshot_target_snapshot_params() // https://html.spec.whatwg.org/multipage/browsing-the-web.html#finalize-a-cross-document-navigation void finalize_a_cross_document_navigation(JS::NonnullGCPtr navigable, HistoryHandlingBehavior history_handling, JS::NonnullGCPtr history_entry) { + // NOTE: This is not in the spec but we should not navigate destroyed navigable. + if (navigable->has_been_destroyed()) + return; + // 1. FIXME: Assert: this is running on navigable's traversable navigable's session history traversal queue. // 2. Set navigable's is delaying load events to false. diff --git a/Userland/Libraries/LibWeb/HTML/Navigable.h b/Userland/Libraries/LibWeb/HTML/Navigable.h index ea7832c4fc..402646c10e 100644 --- a/Userland/Libraries/LibWeb/HTML/Navigable.h +++ b/Userland/Libraries/LibWeb/HTML/Navigable.h @@ -128,6 +128,10 @@ public: void reload(); + // https://github.com/whatwg/html/issues/9690 + [[nodiscard]] bool has_been_destroyed() const { return m_has_been_destroyed; } + void set_has_been_destroyed() { m_has_been_destroyed = true; } + protected: Navigable(); @@ -160,8 +164,12 @@ private: // Implied link between navigable and its container. JS::GCPtr m_container; + + bool m_has_been_destroyed { false }; }; +HashTable& all_navigables(); + bool navigation_must_be_a_replace(AK::URL const& url, DOM::Document const& document); void finalize_a_cross_document_navigation(JS::NonnullGCPtr, HistoryHandlingBehavior, JS::NonnullGCPtr); void perform_url_and_history_update_steps(DOM::Document& document, AK::URL new_url, HistoryHandlingBehavior history_handling = HistoryHandlingBehavior::Reload); diff --git a/Userland/Libraries/LibWeb/HTML/NavigableContainer.cpp b/Userland/Libraries/LibWeb/HTML/NavigableContainer.cpp index 45e1c4baec..c41de04086 100644 --- a/Userland/Libraries/LibWeb/HTML/NavigableContainer.cpp +++ b/Userland/Libraries/LibWeb/HTML/NavigableContainer.cpp @@ -334,6 +334,10 @@ void NavigableContainer::destroy_the_child_navigable() // 7. Let traversable be container's node navigable's traversable navigable. auto traversable = this->navigable()->traversable_navigable(); + // Not in the spec + navigable->set_has_been_destroyed(); + HTML::all_navigables().remove(navigable); + // 8. Append the following session history traversal steps to traversable: traversable->append_session_history_traversal_steps([traversable] { // 1. Apply pending history changes to traversable. diff --git a/Userland/Libraries/LibWeb/HTML/TraversableNavigable.cpp b/Userland/Libraries/LibWeb/HTML/TraversableNavigable.cpp index 14fca46334..6eb34026c6 100644 --- a/Userland/Libraries/LibWeb/HTML/TraversableNavigable.cpp +++ b/Userland/Libraries/LibWeb/HTML/TraversableNavigable.cpp @@ -278,6 +278,10 @@ void TraversableNavigable::apply_the_history_step(int step, Optionalactive_window(), [&] { + // NOTE: This check is not in the spec but we should not continue navigation if navigable has been destroyed. + if (navigable->has_been_destroyed()) + return; + // 1. Let displayedEntry be navigable's active session history entry. auto displayed_entry = navigable->active_session_history_entry(); @@ -389,6 +393,10 @@ void TraversableNavigable::apply_the_history_step(int step, Optionalhas_been_destroyed()) + continue; + // 7. Set navigable's ongoing navigation to null. navigable->set_ongoing_navigation({}); @@ -401,6 +409,10 @@ void TraversableNavigable::apply_the_history_step(int step, Optionalactive_window(), [&, target_entry, navigable, displayed_document, update_only = changing_navigable_continuation.update_only] { + // NOTE: This check is not in the spec but we should not continue navigation if navigable has been destroyed. + if (navigable->has_been_destroyed()) + return; + // 1. If changingNavigableContinuation's update-only is false, then: if (!update_only) { // 1. Unload displayedDocument given targetEntry's document. @@ -582,6 +594,10 @@ void TraversableNavigable::destroy_top_level_traversable() // https://html.spec.whatwg.org/multipage/browsing-the-web.html#finalize-a-same-document-navigation void finalize_a_same_document_navigation(JS::NonnullGCPtr traversable, JS::NonnullGCPtr target_navigable, JS::NonnullGCPtr target_entry, JS::GCPtr entry_to_replace) { + // NOTE: This is not in the spec but we should not navigate destroyed navigable. + if (target_navigable->has_been_destroyed()) + return; + // FIXME: 1. Assert: this is running on traversable's session history traversal queue. // 2. If targetNavigable's active session history entry is not targetEntry, then return.