diff --git a/blink/renderer/core/loader/frame_loader.cc b/blink/renderer/core/loader/frame_loader.cc index 5cd963e32aa..13f0e20238d 100644 --- a/blink/renderer/core/loader/frame_loader.cc +++ b/blink/renderer/core/loader/frame_loader.cc @@ -453,7 +453,7 @@ void FrameLoader::DidFinishNavigation(NavigationFinishState state) { return; if (auto* navigation_api = NavigationApi::navigation(*frame_->DomWindow())) { - if (navigation_api->HasOngoingNavigation()) + if (navigation_api->HasNonDroppedOngoingNavigation()) return; } } @@ -1564,7 +1564,7 @@ void FrameLoader::DidDropNavigation() { return; // TODO(dgozman): should we ClearClientNavigation instead and not // notify the client in response to its own call? - CancelClientNavigation(); + CancelClientNavigation(CancelNavigationReason::kDropped); DidFinishNavigation(FrameLoader::NavigationFinishState::kSuccess); // Forcibly instantiate WindowProxy for initial frame document. @@ -1610,11 +1610,13 @@ void FrameLoader::ClearClientNavigation() { virtual_time_pauser_.UnpauseVirtualTime(); } -void FrameLoader::CancelClientNavigation() { +void FrameLoader::CancelClientNavigation(CancelNavigationReason reason) { if (!client_navigation_) return; + if (auto* navigation_api = NavigationApi::navigation(*frame_->DomWindow())) - navigation_api->InformAboutCanceledNavigation(); + navigation_api->InformAboutCanceledNavigation(reason); + ResourceError error = ResourceError::CancelledError(client_navigation_->url); ClearClientNavigation(); if (WebPluginContainerImpl* plugin = frame_->GetWebPluginContainer()) diff --git a/blink/renderer/core/loader/frame_loader.h b/blink/renderer/core/loader/frame_loader.h index a0d51e2d071..e065aa3fb3f 100644 --- a/blink/renderer/core/loader/frame_loader.h +++ b/blink/renderer/core/loader/frame_loader.h @@ -212,7 +212,8 @@ class CORE_EXPORT FrameLoader final { // Like ClearClientNavigation, but also notifies the client to actually cancel // the navigation. - void CancelClientNavigation(); + void CancelClientNavigation( + CancelNavigationReason reason = CancelNavigationReason::kOther); void Trace(Visitor*) const; diff --git a/blink/renderer/core/loader/frame_loader_types.h b/blink/renderer/core/loader/frame_loader_types.h index 0f05ecec588..641be04f15c 100644 --- a/blink/renderer/core/loader/frame_loader_types.h +++ b/blink/renderer/core/loader/frame_loader_types.h @@ -69,6 +69,14 @@ enum class ClientNavigationReason { kNone }; +enum class CancelNavigationReason { + // The navigation was dropped, e.g. due to a 204, 205, or Content-Disposition: + // attachment. + kDropped, + // Anything else (including error cases that don't drop the navigation). + kOther +}; + enum class CommitReason { // Committing initial empty document. kInitialization, diff --git a/blink/renderer/core/navigation_api/navigation_api.cc b/blink/renderer/core/navigation_api/navigation_api.cc index 55c0ac5139d..43627f7c9ee 100644 --- a/blink/renderer/core/navigation_api/navigation_api.cc +++ b/blink/renderer/core/navigation_api/navigation_api.cc @@ -190,7 +190,8 @@ NavigationApi* NavigationApi::From(LocalDOMWindow& window) { } NavigationApi::NavigationApi(LocalDOMWindow& window) - : Supplement(window) {} + : Supplement(window), + ExecutionContextLifecycleObserver(&window) {} void NavigationApi::setOnnavigate(EventListener* listener) { UseCounter::Count(GetSupplementable(), WebFeature::kAppHistory); @@ -692,7 +693,7 @@ NavigationApi::DispatchResult NavigationApi::DispatchNavigateEvent( // The spec only does the equivalent of CleanupApiNavigation() + resetting // the state, but we need to detach promise resolvers for this case since // we will never resolve the finished/committed promises. - ongoing_navigation_->CleanupForCrossDocument(); + ongoing_navigation_->CleanupForWillNeverSettle(); } return DispatchResult::kContinue; } @@ -758,6 +759,7 @@ NavigationApi::DispatchResult NavigationApi::DispatchNavigateEvent( DCHECK(!ongoing_navigation_signal_); ongoing_navigate_event_ = navigate_event; ongoing_navigation_signal_ = navigate_event->signal(); + has_dropped_navigation_ = false; DispatchEvent(*navigate_event); ongoing_navigate_event_ = nullptr; @@ -804,17 +806,23 @@ NavigationApi::DispatchResult NavigationApi::DispatchNavigateEvent( NavigateReaction::React( script_state, ScriptPromise::All(script_state, tweaked_promise_list), ongoing_navigation_, navigate_event, react_type); - } else if (ongoing_navigation_) { - // The spec assumes it's ok to leave a promise permanently unresolved, but - // ScriptPromiseResolver requires either resolution or explicit detach. - ongoing_navigation_->CleanupForCrossDocument(); } + // Note: we cannot clean up ongoing_navigation_ for cross-document + // navigations, because they might later get interrupted by another + // navigation, in which case we need to reject the promises and so on. + return promise_list.IsEmpty() ? DispatchResult::kContinue : DispatchResult::kTransitionWhile; } -void NavigationApi::InformAboutCanceledNavigation() { +void NavigationApi::InformAboutCanceledNavigation( + CancelNavigationReason reason) { + if (reason == CancelNavigationReason::kDropped) { + has_dropped_navigation_ = true; + return; + } + if (ongoing_navigation_signal_) { auto* script_state = ToScriptStateForMainWorld(GetSupplementable()->GetFrame()); @@ -842,6 +850,15 @@ void NavigationApi::InformAboutCanceledNavigation() { } } +void NavigationApi::ContextDestroyed() { + if (ongoing_navigation_) + ongoing_navigation_->CleanupForWillNeverSettle(); +} + +bool NavigationApi::HasNonDroppedOngoingNavigation() const { + return ongoing_navigation_signal_ && !has_dropped_navigation_; +} + void NavigationApi::RejectPromisesAndFireNavigateErrorEvent( NavigationApiNavigation* navigation, ScriptValue value) { @@ -923,6 +940,7 @@ const AtomicString& NavigationApi::InterfaceName() const { void NavigationApi::Trace(Visitor* visitor) const { EventTargetWithInlineData::Trace(visitor); Supplement::Trace(visitor); + ExecutionContextLifecycleObserver::Trace(visitor); visitor->Trace(entries_); visitor->Trace(transition_); visitor->Trace(ongoing_navigation_); diff --git a/blink/renderer/core/navigation_api/navigation_api.h b/blink/renderer/core/navigation_api/navigation_api.h index 4777089448d..eb56e57c8c6 100644 --- a/blink/renderer/core/navigation_api/navigation_api.h +++ b/blink/renderer/core/navigation_api/navigation_api.h @@ -12,7 +12,9 @@ #include "third_party/blink/renderer/core/core_export.h" #include "third_party/blink/renderer/core/dom/dom_exception.h" #include "third_party/blink/renderer/core/dom/events/event_target.h" +#include "third_party/blink/renderer/core/execution_context/execution_context_lifecycle_observer.h" #include "third_party/blink/renderer/core/frame/local_dom_window.h" +#include "third_party/blink/renderer/core/loader/frame_loader_types.h" #include "third_party/blink/renderer/platform/bindings/exception_state.h" #include "third_party/blink/renderer/platform/heap/collection_support/heap_hash_map.h" #include "third_party/blink/renderer/platform/heap/collection_support/heap_vector.h" @@ -42,8 +44,10 @@ class SerializedScriptValue; enum class UserNavigationInvolvement { kBrowserUI, kActivation, kNone }; enum class NavigateEventType { kFragment, kHistoryApi, kCrossDocument }; -class CORE_EXPORT NavigationApi final : public EventTargetWithInlineData, - public Supplement { +class CORE_EXPORT NavigationApi final + : public EventTargetWithInlineData, + public Supplement, + public ExecutionContextLifecycleObserver { DEFINE_WRAPPERTYPEINFO(); public: @@ -65,7 +69,14 @@ class CORE_EXPORT NavigationApi final : public EventTargetWithInlineData, void SetEntriesForRestore( const mojom::blink::NavigationApiHistoryEntryArraysPtr&); - bool HasOngoingNavigation() const { return ongoing_navigation_signal_; } + // From the navigation API's perspective, a dropped navigation is still + // "ongoing"; that is, ongoing_navigation_signal_ and ongoing_navigation_ are + // non-null. (The impact of this is that another navigation will cancel that + // ongoing navigation, in a web-developer-visible way.) But from the + // perspective of other parts of Chromium which interface with the navigation + // API, e.g. to deal with the loading spinner, dropped navigations are not + // what they care about. + bool HasNonDroppedOngoingNavigation() const; // Web-exposed: NavigationHistoryEntry* currentEntry() const; @@ -109,7 +120,19 @@ class CORE_EXPORT NavigationApi final : public EventTargetWithInlineData, HistoryItem* destination_item, bool is_browser_initiated = false, bool is_synchronously_committed = true); - void InformAboutCanceledNavigation(); + + // In the spec, we are only informed about canceled navigations. But in the + // implementation we need to handle other cases: + // - "Dropped" navigations, e.g. navigations to 204/205/Content-Disposition: + // attachment, need to be tracked for |HasNonDroppedOngoingNavigation()|. + // (See https://github.com/WICG/navigation-api/issues/137 for more on why + // they must be ignored.) This distinction is handled via the |reason| + // argument. + // - If the frame is destroyed without canceling ongoing navigations, e.g. due + // to a cross-document navigation, then we need to detach any outstanding + // promise resolvers. This is handled via |ContextDestroyed()| below. + void InformAboutCanceledNavigation( + CancelNavigationReason reason = CancelNavigationReason::kOther); int GetIndexFor(NavigationHistoryEntry*); @@ -121,6 +144,10 @@ class CORE_EXPORT NavigationApi final : public EventTargetWithInlineData, void Trace(Visitor*) const final; + protected: + // ExecutionContextLifecycleObserver implementation: + void ContextDestroyed() override; + private: friend class NavigateReaction; friend class NavigationApiNavigation; @@ -155,6 +182,7 @@ class CORE_EXPORT NavigationApi final : public EventTargetWithInlineData, HeapVector> entries_; HashMap keys_to_indices_; int current_entry_index_ = -1; + bool has_dropped_navigation_ = false; Member transition_; diff --git a/blink/renderer/core/navigation_api/navigation_api_navigation.cc b/blink/renderer/core/navigation_api/navigation_api_navigation.cc index d433a8cd7e8..9caf3e8ab06 100644 --- a/blink/renderer/core/navigation_api/navigation_api_navigation.cc +++ b/blink/renderer/core/navigation_api/navigation_api_navigation.cc @@ -72,21 +72,21 @@ void NavigationApiNavigation::RejectFinishedPromise(const ScriptValue& value) { if (!navigation_api_) return; - finished_resolver_->Reject(value); - if (committed_resolver_) { // We never hit NotifyAboutTheCommittedToEntry(), so we should reject that // too. committed_resolver_->Reject(value); } + finished_resolver_->Reject(value); + serialized_state_.reset(); navigation_api_->CleanupApiNavigation(*this); navigation_api_ = nullptr; } -void NavigationApiNavigation::CleanupForCrossDocument() { +void NavigationApiNavigation::CleanupForWillNeverSettle() { DCHECK_EQ(committed_to_entry_, nullptr); committed_resolver_->Detach(); diff --git a/blink/renderer/core/navigation_api/navigation_api_navigation.h b/blink/renderer/core/navigation_api/navigation_api_navigation.h index d51b0add448..0de468fb48d 100644 --- a/blink/renderer/core/navigation_api/navigation_api_navigation.h +++ b/blink/renderer/core/navigation_api/navigation_api_navigation.h @@ -32,7 +32,7 @@ class NavigationApiNavigation final void NotifyAboutTheCommittedToEntry(NavigationHistoryEntry*); void ResolveFinishedPromise(); void RejectFinishedPromise(const ScriptValue& value); - void CleanupForCrossDocument(); + void CleanupForWillNeverSettle(); // Note: even though this returns the same NavigationResult every time, the // bindings layer will create a new JS object for each distinct navigation API diff --git a/blink/web_tests/external/wpt/navigation-api/navigation-methods/return-value/back-204-205-download.html b/blink/web_tests/external/wpt/navigation-api/navigation-methods/return-value/back-204-205-download.html new file mode 100644 index 00000000000..5bedbf21e86 --- /dev/null +++ b/blink/web_tests/external/wpt/navigation-api/navigation-methods/return-value/back-204-205-download.html @@ -0,0 +1,52 @@ + + + + + + + + diff --git a/blink/web_tests/external/wpt/navigation-api/navigation-methods/return-value/navigate-204-205-download.html b/blink/web_tests/external/wpt/navigation-api/navigation-methods/return-value/navigate-204-205-download.html new file mode 100644 index 00000000000..f1e89b69405 --- /dev/null +++ b/blink/web_tests/external/wpt/navigation-api/navigation-methods/return-value/navigate-204-205-download.html @@ -0,0 +1,42 @@ + + + + + + + diff --git a/blink/web_tests/external/wpt/navigation-api/navigation-methods/return-value/resources/204-205-download-on-second-visit.py b/blink/web_tests/external/wpt/navigation-api/navigation-methods/return-value/resources/204-205-download-on-second-visit.py new file mode 100644 index 00000000000..c18b0dec3dc --- /dev/null +++ b/blink/web_tests/external/wpt/navigation-api/navigation-methods/return-value/resources/204-205-download-on-second-visit.py @@ -0,0 +1,24 @@ +def main(request, response): + key = request.GET[b"id"] + + # If hit with a POST with ?action=X, store X in the stash + if request.method == "POST": + action = request.GET[b"action"] + request.server.stash.put(key, action) + + return (204, [], "") + + # If hit with a GET, either return a normal initial page, or the abnormal requested response + elif request.method == "GET": + action = request.server.stash.take(key) + + if action is None: + return (200, [("Content-Type", "text/html"), ("Cache-Control", "no-store")], "initial page") + if action == b"204": + return (204, [], "") + if action == b"205": + return (205, [], "") + if action == b"download": + return (200, [("Content-Type", "text/plain"), ("Content-Disposition", "attachment")], "some text to download") + + return (400, [], "") diff --git a/blink/web_tests/external/wpt/navigation-api/ordering-and-transition/navigate-204-205-download-then-same-document.html b/blink/web_tests/external/wpt/navigation-api/ordering-and-transition/navigate-204-205-download-then-same-document.html new file mode 100644 index 00000000000..b7b6283fa70 --- /dev/null +++ b/blink/web_tests/external/wpt/navigation-api/ordering-and-transition/navigate-204-205-download-then-same-document.html @@ -0,0 +1,66 @@ + + + + + diff --git a/blink/web_tests/external/wpt/navigation-api/ordering-and-transition/navigate-cross-document-double.html b/blink/web_tests/external/wpt/navigation-api/ordering-and-transition/navigate-cross-document-double.html index 353cfa1b4e4..147b8d55ef1 100644 --- a/blink/web_tests/external/wpt/navigation-api/ordering-and-transition/navigate-cross-document-double.html +++ b/blink/web_tests/external/wpt/navigation-api/ordering-and-transition/navigate-cross-document-double.html @@ -2,22 +2,48 @@ - diff --git a/blink/web_tests/external/wpt/navigation-api/ordering-and-transition/resources/helpers.mjs b/blink/web_tests/external/wpt/navigation-api/ordering-and-transition/resources/helpers.mjs index 359774de4f7..341befc1056 100644 --- a/blink/web_tests/external/wpt/navigation-api/ordering-and-transition/resources/helpers.mjs +++ b/blink/web_tests/external/wpt/navigation-api/ordering-and-transition/resources/helpers.mjs @@ -7,6 +7,9 @@ export function hasVariant(name) { export class Recorder { #events = []; #errors = []; + #navigationAPI; + #domExceptionConstructor; + #location; #skipCurrentChange; #finalExpectedEvent; #finalExpectedEventCount; @@ -15,16 +18,20 @@ export class Recorder { #readyToAssertResolve; #readyToAssertPromise = new Promise(resolve => { this.#readyToAssertResolve = resolve; }); - constructor({ skipCurrentChange = false, finalExpectedEvent, finalExpectedEventCount = 1 }) { + constructor({ window = self, skipCurrentChange = false, finalExpectedEvent, finalExpectedEventCount = 1 }) { assert_equals(typeof finalExpectedEvent, "string", "Must pass a string for finalExpectedEvent"); + this.#navigationAPI = window.navigation; + this.#domExceptionConstructor = window.DOMException; + this.#location = window.location; + this.#skipCurrentChange = skipCurrentChange; this.#finalExpectedEvent = finalExpectedEvent; this.#finalExpectedEventCount = finalExpectedEventCount; } setUpNavigationAPIListeners() { - navigation.addEventListener("navigate", e => { + this.#navigationAPI.addEventListener("navigate", e => { this.record("navigate"); e.signal.addEventListener("abort", () => { @@ -32,26 +39,26 @@ export class Recorder { }); }); - navigation.addEventListener("navigateerror", e => { + this.#navigationAPI.addEventListener("navigateerror", e => { this.recordWithError("navigateerror", e.error); - navigation.transition?.finished.then( + this.#navigationAPI.transition?.finished.then( () => this.record("transition.finished fulfilled"), err => this.recordWithError("transition.finished rejected", err) ); }); - navigation.addEventListener("navigatesuccess", () => { + this.#navigationAPI.addEventListener("navigatesuccess", () => { this.record("navigatesuccess"); - navigation.transition?.finished.then( + this.#navigationAPI.transition?.finished.then( () => this.record("transition.finished fulfilled"), err => this.recordWithError("transition.finished rejected", err) ); }); if (!this.#skipCurrentChange) { - navigation.addEventListener("currententrychange", () => this.record("currententrychange")); + this.#navigationAPI.addEventListener("currententrychange", () => this.record("currententrychange")); } } @@ -68,12 +75,12 @@ export class Recorder { } record(name) { - const transitionProps = navigation.transition === null ? null : { - from: navigation.transition.from, - navigationType: navigation.transition.navigationType + const transitionProps = this.#navigationAPI.transition === null ? null : { + from: this.#navigationAPI.transition.from, + navigationType: this.#navigationAPI.transition.navigationType }; - this.#events.push({ name, location: location.hash, transitionProps }); + this.#events.push({ name, location: this.#location.hash, transitionProps }); if (name === this.#finalExpectedEvent && ++this.#currentFinalEventCount === this.#finalExpectedEventCount) { this.#readyToAssertResolve(); @@ -169,7 +176,7 @@ export class Recorder { // Assume assert() has been called so all error objects are the same. const { errorObject } = this.#errors[0]; - assert_throws_dom("AbortError", () => { throw errorObject; }); + assert_throws_dom("AbortError", this.#domExceptionConstructor, () => { throw errorObject; }); } assertErrorsAre(expectedErrorObject) {