Explorar el Código

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.
Aliaksandr Kalenik hace 1 año
padre
commit
c437f16cc1

+ 22 - 2
Userland/Libraries/LibWeb/HTML/Navigable.cpp

@@ -51,7 +51,7 @@ private:
     JS::GCPtr<Fetch::Infrastructure::Response> m_response;
 };
 
-static HashTable<Navigable*>& all_navigables()
+HashTable<Navigable*>& all_navigables()
 {
     static HashTable<Navigable*> set;
     return set;
@@ -728,8 +728,16 @@ WebIDL::ExceptionOr<void> 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<String>() || ongoing_navigation().get<String>() != *navigation_id)) {
             completion_steps();
@@ -981,6 +989,10 @@ WebIDL::ExceptionOr<void> 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<void> 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> navigable, HistoryHandlingBehavior history_handling, JS::NonnullGCPtr<SessionHistoryEntry> 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.

+ 8 - 0
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<NavigableContainer> m_container;
+
+    bool m_has_been_destroyed { false };
 };
 
+HashTable<Navigable*>& all_navigables();
+
 bool navigation_must_be_a_replace(AK::URL const& url, DOM::Document const& document);
 void finalize_a_cross_document_navigation(JS::NonnullGCPtr<Navigable>, HistoryHandlingBehavior, JS::NonnullGCPtr<SessionHistoryEntry>);
 void perform_url_and_history_update_steps(DOM::Document& document, AK::URL new_url, HistoryHandlingBehavior history_handling = HistoryHandlingBehavior::Reload);

+ 4 - 0
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.

+ 16 - 0
Userland/Libraries/LibWeb/HTML/TraversableNavigable.cpp

@@ -278,6 +278,10 @@ void TraversableNavigable::apply_the_history_step(int step, Optional<SourceSnaps
     // 12. For each navigable of changingNavigables, queue a global task on the navigation and traversal task source of navigable's active window to run the steps:
     for (auto& navigable : changing_navigables) {
         queue_global_task(Task::Source::NavigationAndTraversal, *navigable->active_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, Optional<SourceSnaps
         // 6. Let navigable be changingNavigableContinuation's navigable.
         auto navigable = changing_navigable_continuation.navigable;
 
+        // NOTE: This check is not in the spec but we should not continue navigation if navigable has been destroyed.
+        if (navigable->has_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, Optional<SourceSnaps
 
         // 10. Queue a global task on the navigation and traversal task source given navigable's active window to run the steps:
         queue_global_task(Task::Source::NavigationAndTraversal, *navigable->active_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<TraversableNavigable> traversable, JS::NonnullGCPtr<Navigable> target_navigable, JS::NonnullGCPtr<SessionHistoryEntry> target_entry, JS::GCPtr<SessionHistoryEntry> 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.