From d4fc8d027ef97d36d5c11b73e4e510b032cf37c5 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Wed, 17 Jun 2026 01:11:47 +0200 Subject: [PATCH] LibWeb: Preserve live iframe navigations during history Speedometer removes and recreates its benchmark iframe while nested session-history bookkeeping is still queued. A live child-frame commit could find that its nested history list had been pruned and then behave like a stale detached frame. That dropped the real src navigation and left the harness waiting for a load event. Preserve the newest real child navigation until the initial session history entry is ready. Tolerate detached child navigables while history steps scan target entries, and recreate the missing nested history only when the child is still the container's live content navigable. Share the nested-history append path with initial child creation so the normal and recovery paths keep the same step handling. Add iframe remove/recreate coverage for pending child history, same-src load, and repeated pushState removal. --- Libraries/LibWeb/DOM/Document.cpp | 6 +- Libraries/LibWeb/HTML/Navigable.cpp | 118 ++++++++++++++---- Libraries/LibWeb/HTML/Navigable.h | 4 + Libraries/LibWeb/HTML/NavigableContainer.cpp | 44 ++----- .../LibWeb/HTML/TraversableNavigable.cpp | 8 +- ...oad-after-remove-and-recreate-same-src.txt | 2 + ...move-and-recreate-with-pending-history.txt | 2 + .../iframe-pushstate-remove-recreate.txt | 1 + ...ad-after-remove-and-recreate-same-src.html | 30 +++++ ...ove-and-recreate-with-pending-history.html | 37 ++++++ .../iframe-pushstate-remove-recreate.html | 29 +++++ 11 files changed, 222 insertions(+), 59 deletions(-) create mode 100644 Tests/LibWeb/Text/expected/navigation/iframe-load-after-remove-and-recreate-same-src.txt create mode 100644 Tests/LibWeb/Text/expected/navigation/iframe-load-after-remove-and-recreate-with-pending-history.txt create mode 100644 Tests/LibWeb/Text/expected/navigation/iframe-pushstate-remove-recreate.txt create mode 100644 Tests/LibWeb/Text/input/navigation/iframe-load-after-remove-and-recreate-same-src.html create mode 100644 Tests/LibWeb/Text/input/navigation/iframe-load-after-remove-and-recreate-with-pending-history.html create mode 100644 Tests/LibWeb/Text/input/navigation/iframe-pushstate-remove-recreate.html diff --git a/Libraries/LibWeb/DOM/Document.cpp b/Libraries/LibWeb/DOM/Document.cpp index c2edf7e473..aa0f881227 100644 --- a/Libraries/LibWeb/DOM/Document.cpp +++ b/Libraries/LibWeb/DOM/Document.cpp @@ -6675,14 +6675,16 @@ void Document::update_for_history_step_application(NonnullRefPtrnavigable()) navigable->restore_persisted_state_from_session_history_entry(*entry); // 3. Initialize the navigation API entries for a new document given navigation, entriesForNavigationAPI, and entry. - navigation->initialize_the_navigation_api_entries_for_a_new_document(*entries_for_navigation_api, entry); + if (update_navigation_api) + navigation->initialize_the_navigation_api_entries_for_a_new_document( + *entries_for_navigation_api, entry); } } diff --git a/Libraries/LibWeb/HTML/Navigable.cpp b/Libraries/LibWeb/HTML/Navigable.cpp index 7efafd7491..2718a7f115 100644 --- a/Libraries/LibWeb/HTML/Navigable.cpp +++ b/Libraries/LibWeb/HTML/Navigable.cpp @@ -40,6 +40,7 @@ #include #include #include +#include #include #include #include @@ -283,6 +284,49 @@ static Vector>* get_session_history_entries_i return nullptr; } +Vector>* append_nested_history_for_child_navigable( + Navigable& parent_navigable, Navigable& child_navigable, SessionHistoryEntry& history_entry) +{ + VERIFY(child_navigable.parent() == &parent_navigable); + + auto parent_doc_state = parent_navigable.active_session_history_entry()->document_state(); + auto& parent_navigable_entries = parent_navigable.get_session_history_entries(); + auto target_step_entry_iterator = parent_navigable_entries.find_if([parent_doc_state](auto& entry) { + return entry->document_state() == parent_doc_state; + }); + if (target_step_entry_iterator == parent_navigable_entries.end()) + return nullptr; + + history_entry.set_step((*target_step_entry_iterator)->step()); + + DocumentState::NestedHistory nested_history { + .id = child_navigable.id(), + .entries { history_entry }, + }; + parent_doc_state->nested_histories().append(move(nested_history)); + return &parent_doc_state->nested_histories().last().entries; +} + +static Vector>* +recreate_missing_nested_history_for_live_child_navigable(TraversableNavigable& traversable, Navigable& navigable) +{ + VERIFY(&navigable != &traversable); + + auto parent = navigable.parent(); + if (!parent) + return nullptr; + + auto container = navigable.container(); + if (!container || container->content_navigable() != &navigable) + return nullptr; + + auto history_entry = navigable.active_session_history_entry(); + if (!history_entry) + return nullptr; + + return append_nested_history_for_child_navigable(*parent, navigable, *history_entry); +} + // https://html.spec.whatwg.org/multipage/document-sequences.html#child-navigable Vector> Navigable::child_navigables() const { @@ -480,11 +524,9 @@ void Navigable::initialize_navigable(NonnullRefPtr document_state } // https://html.spec.whatwg.org/multipage/browsing-the-web.html#getting-the-target-history-entry -RefPtr Navigable::get_the_target_history_entry(int target_step) const +static RefPtr get_the_target_history_entry_from_entries( + Vector> const& entries, int target_step) { - // 1. Let entries be the result of getting session history entries for navigable. - auto& entries = get_session_history_entries(); - // 2. Return the item in entries that has the greatest step less than or equal to step. RefPtr result = nullptr; for (auto& entry : entries) { @@ -501,6 +543,32 @@ RefPtr Navigable::get_the_target_history_entry(int target_s return result; } +RefPtr Navigable::get_the_target_history_entry(int target_step) const +{ + // 1. Let entries be the result of getting session history entries for navigable. + auto& entries = get_session_history_entries(); + + return get_the_target_history_entry_from_entries(entries, target_step); +} + +RefPtr Navigable::get_the_target_history_entry_if_present(int target_step) const +{ + auto traversable = traversable_navigable(); + Vector>* entries = nullptr; + if (this == traversable.ptr()) + entries = &traversable->session_history_entries(); + else + entries = get_session_history_entries_if_present(*traversable, *this); + + // AD-HOC: The spec asserts that a nested history list is found. During queued navigable creation/destruction + // bookkeeping, engines can still observe a child navigable after its iframe has been removed from the + // parent's nested histories. In that case, the detached child has no observable session history effect. + if (!entries) + return nullptr; + + return get_the_target_history_entry_from_entries(*entries, target_step); +} + // https://html.spec.whatwg.org/multipage/browsing-the-web.html#activate-history-entry void Navigable::activate_history_entry(RefPtr entry, GC::Ref document) { @@ -737,13 +805,10 @@ void Navigable::set_ongoing_navigation(Variant ongoing } // AD-HOC: If we just finished a traversal and there are navigations that were deferred because the traversal was - // ongoing, process them now. - if (was_traversal && !ongoing_navigation.has()) { - while (!m_pending_navigations.is_empty()) { - auto navigation_params = m_pending_navigations.take_first(); - begin_navigation(navigation_params); - } - } + // ongoing, process them now. A freshly-created child navigable can also have pending navigations while + // its initial session history entry is being installed, so only drain once both gates are open. + if (was_traversal && !ongoing_navigation.has() && m_has_session_history_entry_and_ready_for_navigation) + process_pending_navigations(); } void Navigable::queue_pending_navigation(NavigateParams params, PendingNavigationBehavior behavior) @@ -753,6 +818,14 @@ void Navigable::queue_pending_navigation(NavigateParams params, PendingNavigatio m_pending_navigations.append(move(params)); } +void Navigable::process_pending_navigations() +{ + while (!m_pending_navigations.is_empty()) { + auto navigation_params = m_pending_navigations.take_first(); + begin_navigation(navigation_params); + } +} + // https://html.spec.whatwg.org/multipage/document-sequences.html#the-rules-for-choosing-a-navigable Navigable::ChosenNavigable Navigable::choose_a_navigable(StringView name, TokenizedFeature::NoOpener no_opener, ActivateTab activate_tab, Optional window_features) { @@ -2923,14 +2996,18 @@ void finalize_a_cross_document_navigation(GC::Ref navigable, HistoryH } else { target_entries_pointer = get_session_history_entries_if_present(*traversable, navigable); // https://html.spec.whatwg.org/multipage/browsing-the-web.html#getting-session-history-entries - // AD-HOC: The spec asserts that a nested history list is found. A queued child-frame commit can run - // after the iframe has been removed, when there is no remaining list to update. Chromium, - // WebKit, and Gecko bind child-frame commits to the live frame, so a removed frame's late - // commit has no observable session history effect. + // AD-HOC: The spec asserts that targetEntries is not null. A queued child-frame commit can run after the + // iframe was removed and its nested history list was pruned from the parent document state. Chromium, + // WebKit, and Gecko bind child-frame commits to the live frame, so detached frames have no observable + // session history effect. Conversely, if this is still the container's live content navigable, + // preserve the requested navigation by recreating the missing nested history. if (!target_entries_pointer) { - navigable->clear_navigation_load_event_guard(); - on_complete->function()(HistoryStepResult::Applied); - return; + target_entries_pointer = recreate_missing_nested_history_for_live_child_navigable(*traversable, *navigable); + if (!target_entries_pointer) { + navigable->clear_navigation_load_event_guard(); + on_complete->function()(HistoryStepResult::Applied); + return; + } } } auto& target_entries = *target_entries_pointer; @@ -3683,10 +3760,7 @@ void Navigable::stop_loading() void Navigable::set_has_session_history_entry_and_ready_for_navigation() { m_has_session_history_entry_and_ready_for_navigation = true; - while (!m_pending_navigations.is_empty()) { - auto navigation_params = m_pending_navigations.take_first(); - begin_navigation(navigation_params); - } + process_pending_navigations(); } Painting::CompositorSurfaceId Navigable::compositor_surface_id() const diff --git a/Libraries/LibWeb/HTML/Navigable.h b/Libraries/LibWeb/HTML/Navigable.h index 9eb6241872..a1ee42e9bc 100644 --- a/Libraries/LibWeb/HTML/Navigable.h +++ b/Libraries/LibWeb/HTML/Navigable.h @@ -114,6 +114,7 @@ public: GC::Ptr active_window(); RefPtr get_the_target_history_entry(int target_step) const; + RefPtr get_the_target_history_entry_if_present(int target_step) const; void save_persisted_state_to_active_session_history_entry(); void restore_persisted_state_from_session_history_entry(SessionHistoryEntry const&); @@ -311,6 +312,7 @@ private: void begin_navigation(NavigateParams); void queue_pending_navigation(NavigateParams, PendingNavigationBehavior); + void process_pending_navigations(); void navigate_to_a_fragment(URL::URL const&, HistoryHandlingBehavior, UserNavigationInvolvement, GC::Ptr source_element, Optional navigation_api_state, String navigation_id); void navigate_to_a_javascript_url(URL::URL const&, HistoryHandlingBehavior, GC::Ref, URL::Origin const& initiator_origin, UserNavigationInvolvement, ContentSecurityPolicy::Directives::Directive::NavigationType csp_navigation_type, InitialInsertion, String navigation_id); @@ -420,6 +422,8 @@ private: WEB_API HashTable>& all_navigables(); +Vector>* append_nested_history_for_child_navigable( + Navigable& parent_navigable, Navigable& child_navigable, SessionHistoryEntry& history_entry); bool navigation_must_be_a_replace(URL::URL const& url, DOM::Document const& document); void finalize_a_cross_document_navigation(GC::Ref, HistoryHandlingBehavior, UserNavigationInvolvement, NonnullRefPtr, GC::Ptr pending_document, Optional expected_ongoing_navigation_id, GC::Ref on_complete); void perform_url_and_history_update_steps(DOM::Document& document, URL::URL new_url, Optional = {}, HistoryHandlingBehavior history_handling = HistoryHandlingBehavior::Replace); diff --git a/Libraries/LibWeb/HTML/NavigableContainer.cpp b/Libraries/LibWeb/HTML/NavigableContainer.cpp index b0d0e15023..5794ae325f 100644 --- a/Libraries/LibWeb/HTML/NavigableContainer.cpp +++ b/Libraries/LibWeb/HTML/NavigableContainer.cpp @@ -119,28 +119,8 @@ void NavigableContainer::create_new_child_navigable() return; } - // 1. Let parentDocState be parentNavigable's active session history entry's document state. - auto parent_doc_state = parent_navigable->active_session_history_entry()->document_state(); - - // 2. Let parentNavigableEntries be the result of getting session history entries for parentNavigable. - auto parent_navigable_entries = parent_navigable->get_session_history_entries(); - - // 3. Let targetStepSHE be the first session history entry in parentNavigableEntries whose document state equals parentDocState. - auto target_step_she = *parent_navigable_entries.find_if([parent_doc_state](auto& entry) { - return entry->document_state() == parent_doc_state; - }); - - // 4. Set historyEntry's step to targetStepSHE's step. - history_entry->set_step(target_step_she->step()); - - // 5. Let nestedHistory be a new nested history whose id is navigable's id and entries list is « historyEntry ». - DocumentState::NestedHistory nested_history { - .id = navigable->id(), - .entries { *history_entry }, - }; - - // 6. Append nestedHistory to parentDocState's nested histories. - parent_doc_state->nested_histories().append(move(nested_history)); + // 1-6. Append nestedHistory to parentDocState's nested histories. + VERIFY(append_nested_history_for_child_navigable(*parent_navigable, *navigable, *history_entry)); // 7. Update for navigable creation/destruction given traversable traversable->update_for_navigable_creation_or_destruction(GC::create_function(traversable->heap(), [signal](HistoryStepResult) { @@ -221,15 +201,6 @@ Optional NavigableContainer::shared_attribute_processing_steps_for_ifr if (!m_content_navigable) return {}; - // AD-HOC: If the content navigable already has a navigation in progress or pending, - // skip the initial attribute processing. Without this, the about:blank URL update - // from perform_url_and_history_update_steps creates a state machine that clobbers the - // navigable's ongoing_navigation, causing the real navigation to be dropped when its - // populate completion callback checks ongoing_navigation != navigation_id. - if (initial_insertion == InitialInsertion::Yes && (m_content_navigable->has_pending_navigations() || !m_content_navigable->ongoing_navigation().has())) { - return {}; - } - // 1. Let url be the URL record about:blank. auto url = URL::about_blank(); @@ -252,6 +223,17 @@ Optional NavigableContainer::shared_attribute_processing_steps_for_ifr return {}; } + // AD-HOC: If the content navigable already has a navigation in progress or pending, skip the initial + // about:blank URL update. Without this, the URL update creates a state machine that clobbers the + // navigable's ongoing_navigation, causing the real navigation to be dropped when its populate completion + // callback checks ongoing_navigation != navigation_id. Non-blank src navigations must still be processed + // here, and will be queued by Navigable::navigate() until the child navigable is ready for navigation. + if (url_matches_about_blank(url) && initial_insertion == InitialInsertion::Yes + && (m_content_navigable->has_pending_navigations() + || !m_content_navigable->ongoing_navigation().has())) { + return {}; + } + // 4. If url matches about:blank and initialInsertion is true, then perform the URL and history update steps given element's content navigable's active document and url. if (url_matches_about_blank(url) && initial_insertion == InitialInsertion::Yes) { auto& document = *m_content_navigable->active_document(); diff --git a/Libraries/LibWeb/HTML/TraversableNavigable.cpp b/Libraries/LibWeb/HTML/TraversableNavigable.cpp index 40f1a0df7d..f08a358d54 100644 --- a/Libraries/LibWeb/HTML/TraversableNavigable.cpp +++ b/Libraries/LibWeb/HTML/TraversableNavigable.cpp @@ -601,7 +601,7 @@ Vector> TraversableNavigable::get_all_navigables_whose_curre auto navigable = navigables_to_check.take_first(); // 1. Let targetEntry be the result of getting the target history entry given navigable and targetStep. - auto target_entry = navigable->get_the_target_history_entry(target_step); + auto target_entry = navigable->get_the_target_history_entry_if_present(target_step); if (!target_entry) continue; @@ -641,7 +641,7 @@ Vector> TraversableNavigable::get_all_navigables_that_only_n auto navigable = navigables_to_check.take_first(); // 1. Let targetEntry be the result of getting the target history entry given navigable and targetStep. - auto target_entry = navigable->get_the_target_history_entry(target_step); + auto target_entry = navigable->get_the_target_history_entry_if_present(target_step); if (!target_entry) continue; @@ -680,7 +680,7 @@ Vector> TraversableNavigable::get_all_navigables_that_might_ auto navigable = navigables_to_check.take_first(); // 1. Let targetEntry be the result of getting the target history entry given navigable and targetStep. - auto target_entry = navigable->get_the_target_history_entry(target_step); + auto target_entry = navigable->get_the_target_history_entry_if_present(target_step); if (!target_entry) continue; @@ -980,7 +980,7 @@ void ApplyHistoryStepState::start() continue; // 1. Let targetEntry be the result of getting the target history entry given navigable and targetStep. - auto target_entry = navigable->get_the_target_history_entry(m_target_step); + auto target_entry = navigable->get_the_target_history_entry_if_present(m_target_step); if (!target_entry) continue; diff --git a/Tests/LibWeb/Text/expected/navigation/iframe-load-after-remove-and-recreate-same-src.txt b/Tests/LibWeb/Text/expected/navigation/iframe-load-after-remove-and-recreate-same-src.txt new file mode 100644 index 0000000000..9f93138a2b --- /dev/null +++ b/Tests/LibWeb/Text/expected/navigation/iframe-load-after-remove-and-recreate-same-src.txt @@ -0,0 +1,2 @@ +first load +second load diff --git a/Tests/LibWeb/Text/expected/navigation/iframe-load-after-remove-and-recreate-with-pending-history.txt b/Tests/LibWeb/Text/expected/navigation/iframe-load-after-remove-and-recreate-with-pending-history.txt new file mode 100644 index 0000000000..9f93138a2b --- /dev/null +++ b/Tests/LibWeb/Text/expected/navigation/iframe-load-after-remove-and-recreate-with-pending-history.txt @@ -0,0 +1,2 @@ +first load +second load diff --git a/Tests/LibWeb/Text/expected/navigation/iframe-pushstate-remove-recreate.txt b/Tests/LibWeb/Text/expected/navigation/iframe-pushstate-remove-recreate.txt new file mode 100644 index 0000000000..7ef22e9a43 --- /dev/null +++ b/Tests/LibWeb/Text/expected/navigation/iframe-pushstate-remove-recreate.txt @@ -0,0 +1 @@ +PASS diff --git a/Tests/LibWeb/Text/input/navigation/iframe-load-after-remove-and-recreate-same-src.html b/Tests/LibWeb/Text/input/navigation/iframe-load-after-remove-and-recreate-same-src.html new file mode 100644 index 0000000000..c08181353d --- /dev/null +++ b/Tests/LibWeb/Text/input/navigation/iframe-load-after-remove-and-recreate-same-src.html @@ -0,0 +1,30 @@ + + + diff --git a/Tests/LibWeb/Text/input/navigation/iframe-load-after-remove-and-recreate-with-pending-history.html b/Tests/LibWeb/Text/input/navigation/iframe-load-after-remove-and-recreate-with-pending-history.html new file mode 100644 index 0000000000..0a57ddd551 --- /dev/null +++ b/Tests/LibWeb/Text/input/navigation/iframe-load-after-remove-and-recreate-with-pending-history.html @@ -0,0 +1,37 @@ + + + diff --git a/Tests/LibWeb/Text/input/navigation/iframe-pushstate-remove-recreate.html b/Tests/LibWeb/Text/input/navigation/iframe-pushstate-remove-recreate.html new file mode 100644 index 0000000000..6a7ce92ecd --- /dev/null +++ b/Tests/LibWeb/Text/input/navigation/iframe-pushstate-remove-recreate.html @@ -0,0 +1,29 @@ + + +