From 327437cfc69e133b6bd048438dd128069096e31c Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Mon, 15 Jun 2026 11:26:51 +0200 Subject: [PATCH] LibWeb: Commit same-document navigations synchronously Finalize fragment navigations and URL/history updates immediately when no traversal state is active. Keep the queued same-document finalizer as the fallback for reentrant traversal work and child navigables whose nested history is not installed yet. Share the entry-list portion of same-document navigation finalization between the fast path and queued fallback, so append and replace bookkeeping cannot drift. Preserve unrelated ongoing cross-document navigations when a page starts a load and then performs a same-document history update in the same task. This matches Chromium, WebKit, and Gecko: the same-document update must not cancel the pending real navigation. The session-history mirror tests now observe synchronous UI updates. A navigation test covers the pending-load plus pushState race. --- Libraries/LibWeb/HTML/Navigable.cpp | 48 +++-- .../LibWeb/HTML/TraversableNavigable.cpp | 176 +++++++++++++----- Libraries/LibWeb/HTML/TraversableNavigable.h | 1 + .../location-navigate-then-push-state.txt | 1 + .../location-navigate-then-push-state.html | 37 ++++ ...ss-session-history-same-document-back.html | 1 - ...process-session-history-same-document.html | 1 - 7 files changed, 195 insertions(+), 70 deletions(-) create mode 100644 Tests/LibWeb/Text/expected/navigation/location-navigate-then-push-state.txt create mode 100644 Tests/LibWeb/Text/input/navigation/location-navigate-then-push-state.html diff --git a/Libraries/LibWeb/HTML/Navigable.cpp b/Libraries/LibWeb/HTML/Navigable.cpp index fe0ef0b40c..8734aa3ee1 100644 --- a/Libraries/LibWeb/HTML/Navigable.cpp +++ b/Libraries/LibWeb/HTML/Navigable.cpp @@ -2455,18 +2455,24 @@ void Navigable::navigate_to_a_fragment(URL::URL const& url, HistoryHandlingBehav // 16. Let traversable be navigable's traversable navigable. auto traversable = traversable_navigable(); + // AD-HOC: Browser engines commit same-document navigations synchronously when no traversal state is active. Keep + // the spec's queued synchronous-navigation steps as the fallback for reentrant traversal work and child + // navigables whose nested history is not ready yet. // 17. Append the following session history synchronous navigation steps involving navigable to traversable: - traversable->append_session_history_synchronous_navigation_steps(*this, GC::create_function(heap(), [this, traversable, history_entry, entry_to_replace, navigation_id, history_handling, user_involvement](NonnullRefPtr> signal) { - // 1. Finalize a same-document navigation given traversable, navigable, historyEntry, entryToReplace, historyHandling, and userInvolvement. - finalize_a_same_document_navigation(*traversable, *this, history_entry, entry_to_replace, history_handling, user_involvement, - GC::create_function(heap(), [signal](HistoryStepResult) { - signal->resolve({}); - })); + if (!traversable->try_to_synchronously_commit_same_document_navigation(*this, history_entry, entry_to_replace)) { + traversable->append_session_history_synchronous_navigation_steps(*this, GC::create_function(heap(), [this, traversable, history_entry, entry_to_replace, navigation_id, history_handling, user_involvement](NonnullRefPtr> signal) { + // 1. Finalize a same-document navigation given traversable, navigable, historyEntry, entryToReplace, + // historyHandling, and userInvolvement. + finalize_a_same_document_navigation(*traversable, *this, history_entry, entry_to_replace, history_handling, user_involvement, + GC::create_function(heap(), [signal](HistoryStepResult) { + signal->resolve({}); + })); - // FIXME: 2. Invoke WebDriver BiDi fragment navigated with navigable and a new WebDriver BiDi - // navigation status whose id is navigationId, url is url, and status is "complete". - (void)navigation_id; - })); + // FIXME: 2. Invoke WebDriver BiDi fragment navigated with navigable and a new WebDriver BiDi + // navigation status whose id is navigationId, url is url, and status is "complete". + (void)navigation_id; + })); + } } // https://html.spec.whatwg.org/multipage/browsing-the-web.html#evaluate-a-javascript:-url @@ -3018,15 +3024,21 @@ void perform_url_and_history_update_steps(DOM::Document& document, URL::URL new_ // 12. Let traversable be navigable's traversable navigable. auto traversable = navigable->traversable_navigable(); + // AD-HOC: Browser engines commit same-document navigations synchronously when no traversal state is active. Keep + // the spec's queued synchronous-navigation steps as the fallback for reentrant traversal work and child + // navigables whose nested history is not ready yet. // 13. Append the following session history synchronous navigation steps involving navigable to traversable: - traversable->append_session_history_synchronous_navigation_steps(*navigable, GC::create_function(document.realm().heap(), [traversable, navigable, new_entry, entry_to_replace, history_handling](NonnullRefPtr> signal) { - // 1. Finalize a same-document navigation given traversable, navigable, newEntry, entryToReplace, historyHandling, and "none". - finalize_a_same_document_navigation(*traversable, *navigable, new_entry, entry_to_replace, history_handling, UserNavigationInvolvement::None, - GC::create_function(traversable->heap(), [signal](HistoryStepResult) { - signal->resolve({}); - })); - // 2. FIXME: Invoke WebDriver BiDi history updated with navigable. - })); + if (!traversable->try_to_synchronously_commit_same_document_navigation(*navigable, new_entry, entry_to_replace)) { + traversable->append_session_history_synchronous_navigation_steps(*navigable, GC::create_function(document.realm().heap(), [traversable, navigable, new_entry, entry_to_replace, history_handling](NonnullRefPtr> signal) { + // 1. Finalize a same-document navigation given traversable, navigable, newEntry, entryToReplace, + // historyHandling, and "none". + finalize_a_same_document_navigation(*traversable, *navigable, new_entry, entry_to_replace, history_handling, UserNavigationInvolvement::None, + GC::create_function(traversable->heap(), [signal](HistoryStepResult) { + signal->resolve({}); + })); + // 2. FIXME: Invoke WebDriver BiDi history updated with navigable. + })); + } } void Navigable::scroll_offset_did_change() diff --git a/Libraries/LibWeb/HTML/TraversableNavigable.cpp b/Libraries/LibWeb/HTML/TraversableNavigable.cpp index 3b3f33fcac..ec049e121a 100644 --- a/Libraries/LibWeb/HTML/TraversableNavigable.cpp +++ b/Libraries/LibWeb/HTML/TraversableNavigable.cpp @@ -321,6 +321,15 @@ static NonnullRefPtr create_session_history_entry_from_ui_p return entry; } +static bool synchronous_same_document_navigation_must_preserve_ongoing_navigation(Navigable const& navigable) +{ + // AD-HOC: The spec queues same-document history updates because they happen synchronously, outside the traversal + // queue, and must later resolve races with the current history step. If another navigation has already + // claimed the navigable, leave that navigation ID alone. This matches Chromium, WebKit, and Gecko: + // a same-document history update from the same task does not cancel a later cross-document navigation. + return navigable.ongoing_navigation().has(); +} + bool TraversableNavigable::replace_top_level_session_history_entries_from_ui_process(Vector entries_from_ui_process, size_t current_top_level_entry_index) { if (entries_from_ui_process.is_empty() || current_top_level_entry_index >= entries_from_ui_process.size()) @@ -839,14 +848,8 @@ void ApplyHistoryStepState::start() // 8. For each navigable of changingNavigables: auto changing_navigables = m_traversable->get_all_navigables_whose_current_session_history_entry_will_change_or_reload(m_target_step); for (auto& navigable : changing_navigables) { - // https://html.spec.whatwg.org/multipage/browsing-the-web.html#finalize-a-same-document-navigation - // AD-HOC: The spec queues same-document history updates because they happen synchronously, outside the - // traversal queue, and must later resolve races with the current history step. If another navigation - // has already claimed the navigable by the time this queued reconciliation runs, leave that navigation - // ID alone. This matches the observable behavior of Chromium, WebKit, and Gecko: a same-document - // history update does not cancel a later cross-document navigation from the same task. if (m_synchronous_navigation == TraversableNavigable::SynchronousNavigation::Yes - && navigable->ongoing_navigation().has()) { + && synchronous_same_document_navigation_must_preserve_ongoing_navigation(*navigable)) { continue; } @@ -860,6 +863,18 @@ void ApplyHistoryStepState::start() // 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); + // https://html.spec.whatwg.org/multipage/nav-history-apis.html#fire-a-traverse-navigate-event + // NB: Same-document traversals are synchronous in browser engines, but the specification routes them through + // the traversal queue. If a later cross-document navigation has already claimed the navigable by the time + // this queued same-document traversal reaches its bookkeeping step, do not replace that navigation's ID + // with "traversal". The queued traversal is stale reconciliation at that point, and must not cancel the + // newer navigation. + if (m_navigation_type == Bindings::NavigationType::Traverse + && navigable->ongoing_navigation().has() + && target_entry->document_state()->document_id() == navigable->active_document_id()) { + continue; + } + // 2. Set navigable's current session history entry to targetEntry. navigable->set_current_session_history_entry(target_entry); @@ -2119,6 +2134,108 @@ void TraversableNavigable::apply_the_push_or_replace_history_step(int step, Hist apply_the_history_step(step, false, {}, {}, user_involvement, navigation_type, synchronous_navigation, Navigable::NavigationAPIAbortBehavior::Abort, pending_document, on_complete); } +static Optional update_session_history_entries_for_same_document_navigation(TraversableNavigable& traversable, GC::Ref target_navigable, NonnullRefPtr target_entry, RefPtr entry_to_replace) +{ + // NB: This is the entry-list portion of the "finalize a same-document navigation" algorithm. Keep the synchronous + // commit path and the queued fallback sharing it so the two paths cannot drift. + + // 2. If targetNavigable's active session history entry is not targetEntry, then return. + // FIXME: This is a workaround for a spec issue where the early return loses replace entries. + // Revisit when https://github.com/whatwg/html/issues/10232 is resolved. + if (target_navigable->active_session_history_entry() != target_entry) { + if (entry_to_replace) { + auto& target_entries = target_navigable->get_session_history_entries(); + if (auto it = target_entries.find(*entry_to_replace); it != target_entries.end()) { + target_entry->set_step(entry_to_replace->step()); + *it = target_entry; + } + } + return {}; + } + + // 3. Let targetStep be null. + Optional target_step; + + // 4. Let targetEntries be the result of getting session history entries for targetNavigable. + auto& target_entries = target_navigable->get_session_history_entries(); + + // 5. If entryToReplace is null, then: + // FIXME: Checking containment of entryToReplace should not be needed. + // For more details see https://github.com/whatwg/html/issues/10232#issuecomment-2037543137 + if (!entry_to_replace || !target_entries.contains_slow(NonnullRefPtr { *entry_to_replace })) { + // 1. Clear the forward session history of traversable. + traversable.clear_the_forward_session_history(); + + // 2. Set targetStep to traversable's current session history step + 1. + target_step = traversable.current_session_history_step() + 1; + + // 3. Set targetEntry's step to targetStep. + target_entry->set_step(*target_step); + + // 4. Append targetEntry to targetEntries. + target_entries.append(target_entry); + } else { + // 1. Replace entryToReplace with targetEntry in targetEntries. + *(target_entries.find(*entry_to_replace)) = target_entry; + + // 2. Set targetEntry's step to entryToReplace's step. + target_entry->set_step(entry_to_replace->step()); + + // 3. Set targetStep to traversable's current session history step. + target_step = traversable.current_session_history_step(); + } + + return target_step; +} + +bool TraversableNavigable::try_to_synchronously_commit_same_document_navigation(GC::Ref target_navigable, NonnullRefPtr target_entry, RefPtr entry_to_replace) +{ + if (m_apply_history_step_state || m_paused_apply_history_step_state) + return false; + + if (target_navigable->has_been_destroyed()) + return true; + + if (!target_navigable->has_session_history_entry_and_ready_for_navigation()) + return false; + + // https://html.spec.whatwg.org/multipage/browsing-the-web.html#finalize-a-same-document-navigation + auto target_step = update_session_history_entries_for_same_document_navigation(*this, target_navigable, target_entry, entry_to_replace); + if (!target_step.has_value()) + return true; + + target_navigable->set_current_session_history_entry(target_entry); + m_current_session_history_step = get_the_used_step(*target_step); + + // NB: The queued apply-history-step path clears the ongoing navigation when the history step finishes. The + // synchronous fast path has already committed the same-document navigation and the Navigation API entry update + // owns settling its promises/events, so do the same cleanup without reporting an abort to the Navigation API. + if (!synchronous_same_document_navigation_must_preserve_ongoing_navigation(*target_navigable)) + target_navigable->set_ongoing_navigation({}, Navigable::NavigationAPIAbortBehavior::Preserve); + + auto history_object_length_and_index = get_the_history_object_length_and_index(m_current_session_history_step); + if (auto active_document = this->active_document()) { + for (auto const& navigable : active_document->inclusive_descendant_navigables()) { + if (navigable->has_been_destroyed() || !navigable->active_window() || !navigable->active_document()->is_fully_active()) + continue; + + auto document = navigable->active_document(); + document->history()->m_index = history_object_length_and_index.script_history_index; + document->history()->m_length = history_object_length_and_index.script_history_length; + } + } + + if (page().client().should_report_session_history_updates()) { + auto session_history_snapshot = create_session_history_snapshot(SaveActiveEntryPersistedState::Yes); + page().client().page_did_update_session_history(session_history_snapshot.top_level_session_history_entries, session_history_snapshot.used_session_history_steps, session_history_snapshot.current_used_step_index); + } + + VERIFY(session_history_entries().size() > 0); + page().client().page_did_update_navigation_buttons_state(can_go_back(), can_go_forward()); + page().client().page_did_change_url(current_session_history_entry()->url()); + return true; +} + void TraversableNavigable::apply_the_traverse_history_step(int step, GC::Ptr source_snapshot_params, GC::Ptr initiator_to_check, UserNavigationInvolvement user_involvement, GC::Ref> on_complete) { // 1. Return the result of applying the history step step to traversable given true, sourceSnapshotParams, initiatorToCheck, userInvolvement, and "traverse". @@ -2225,53 +2342,12 @@ void finalize_a_same_document_navigation(GC::Ref traversab // 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. - // FIXME: This is a workaround for a spec issue where the early return loses replace entries. - // Revisit when https://github.com/whatwg/html/issues/10232 is resolved. - if (target_navigable->active_session_history_entry() != target_entry) { - if (entry_to_replace) { - auto& target_entries = target_navigable->get_session_history_entries(); - if (auto it = target_entries.find(*entry_to_replace); it != target_entries.end()) { - target_entry->set_step(entry_to_replace->step()); - *it = target_entry; - } - } + auto target_step = update_session_history_entries_for_same_document_navigation(*traversable, target_navigable, target_entry, entry_to_replace); + if (!target_step.has_value()) { on_complete->function()(HistoryStepResult::Applied); return; } - // 3. Let targetStep be null. - Optional target_step; - - // 4. Let targetEntries be the result of getting session history entries for targetNavigable. - auto& target_entries = target_navigable->get_session_history_entries(); - - // 5. If entryToReplace is null, then: - // FIXME: Checking containment of entryToReplace should not be needed. - // For more details see https://github.com/whatwg/html/issues/10232#issuecomment-2037543137 - if (!entry_to_replace || !target_entries.contains_slow(NonnullRefPtr { *entry_to_replace })) { - // 1. Clear the forward session history of traversable. - traversable->clear_the_forward_session_history(); - - // 2. Set targetStep to traversable's current session history step + 1. - target_step = traversable->current_session_history_step() + 1; - - // 3. Set targetEntry's step to targetStep. - target_entry->set_step(*target_step); - - // 4. Append targetEntry to targetEntries. - target_entries.append(target_entry); - } else { - // 1. Replace entryToReplace with targetEntry in targetEntries. - *(target_entries.find(*entry_to_replace)) = target_entry; - - // 2. Set targetEntry's step to entryToReplace's step. - target_entry->set_step(entry_to_replace->step()); - - // 3. Set targetStep to traversable's current session history step. - target_step = traversable->current_session_history_step(); - } - // 6. Apply the push/replace history step targetStep to traversable given historyHandling and userInvolvement. traversable->apply_the_push_or_replace_history_step(*target_step, history_handling, user_involvement, TraversableNavigable::SynchronousNavigation::Yes, nullptr, on_complete); } diff --git a/Libraries/LibWeb/HTML/TraversableNavigable.h b/Libraries/LibWeb/HTML/TraversableNavigable.h index 06596d587b..9679865158 100644 --- a/Libraries/LibWeb/HTML/TraversableNavigable.h +++ b/Libraries/LibWeb/HTML/TraversableNavigable.h @@ -76,6 +76,7 @@ public: Yes, No, }; + [[nodiscard]] bool try_to_synchronously_commit_same_document_navigation(GC::Ref, NonnullRefPtr, RefPtr entry_to_replace); void apply_the_push_or_replace_history_step(int step, HistoryHandlingBehavior history_handling, UserNavigationInvolvement, SynchronousNavigation, GC::Ptr pending_document, GC::Ref on_complete); void update_for_navigable_creation_or_destruction(GC::Ref on_complete); diff --git a/Tests/LibWeb/Text/expected/navigation/location-navigate-then-push-state.txt b/Tests/LibWeb/Text/expected/navigation/location-navigate-then-push-state.txt new file mode 100644 index 0000000000..7ef22e9a43 --- /dev/null +++ b/Tests/LibWeb/Text/expected/navigation/location-navigate-then-push-state.txt @@ -0,0 +1 @@ +PASS diff --git a/Tests/LibWeb/Text/input/navigation/location-navigate-then-push-state.html b/Tests/LibWeb/Text/input/navigation/location-navigate-then-push-state.html new file mode 100644 index 0000000000..e496481964 --- /dev/null +++ b/Tests/LibWeb/Text/input/navigation/location-navigate-then-push-state.html @@ -0,0 +1,37 @@ + + + diff --git a/Tests/LibWeb/Text/input/navigation/ui-process-session-history-same-document-back.html b/Tests/LibWeb/Text/input/navigation/ui-process-session-history-same-document-back.html index 0d7d847e61..ac3a72a981 100644 --- a/Tests/LibWeb/Text/input/navigation/ui-process-session-history-same-document-back.html +++ b/Tests/LibWeb/Text/input/navigation/ui-process-session-history-same-document-back.html @@ -32,7 +32,6 @@ asyncTest(async done => { history.replaceState({ replaced: true }, "", "?same-document-back-replaced"); history.pushState({ pushed: true }, "", "?same-document-back-pushed"); - await internals.flushSessionHistoryTraversalQueue(); const initialUIHistory = currentUIHistory(); if (!internals.dumpSessionHistory().includes("ui-process-session-history-same-document-back.html?same-document-back-pushed (current)") diff --git a/Tests/LibWeb/Text/input/navigation/ui-process-session-history-same-document.html b/Tests/LibWeb/Text/input/navigation/ui-process-session-history-same-document.html index fb4e64b358..1924e92ff5 100644 --- a/Tests/LibWeb/Text/input/navigation/ui-process-session-history-same-document.html +++ b/Tests/LibWeb/Text/input/navigation/ui-process-session-history-same-document.html @@ -28,7 +28,6 @@ asyncTest(async done => { history.replaceState({ replaced: true }, "", "?same-document-replaced"); history.pushState({ pushed: true }, "", "?same-document-pushed"); - await internals.flushSessionHistoryTraversalQueue(); const uiHistory = currentUIHistory(); if (!internals.dumpSessionHistory().includes("ui-process-session-history-same-document.html?same-document-pushed (current)")