Navigation API: ensure that 204/205/downloads never settle

See WICG/navigation-api#137.

This requires distinguishing between "dropped" navigations and other
canceled navigations at the FrameLoader/navigation API interface.

This also makes cross-document ongoing navigations generally abortable,
as can be seen by the revamped navigate-cross-document-double.html test.
Previously, we would clean up the ongoing navigation for any
cross-document nav, but per spec we need to keep it around so that it
can be aborted if someone interrupts it. (This applies both to "normal"
cross-document navigations, and ones to 204s/etc.)

Bug: 1183545
Change-Id: I6f8f1771816ddf454d651f7147d3179fba88979b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3532324
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Auto-Submit: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#984420}

third_party/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())

third_party/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;
 

third_party/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,

third_party/blink/renderer/core/navigation_api/navigation_api.cc

@@ -190,7 +190,8 @@ NavigationApi* NavigationApi::From(LocalDOMWindow& window) {
 }
 
 NavigationApi::NavigationApi(LocalDOMWindow& window)
-    : Supplement<LocalDOMWindow>(window) {}
+    : Supplement<LocalDOMWindow>(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<LocalDOMWindow>::Trace(visitor);
+  ExecutionContextLifecycleObserver::Trace(visitor);
   visitor->Trace(entries_);
   visitor->Trace(transition_);
   visitor->Trace(ongoing_navigation_);

third_party/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<LocalDOMWindow> {
+class CORE_EXPORT NavigationApi final
+    : public EventTargetWithInlineData,
+      public Supplement<LocalDOMWindow>,
+      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<Member<NavigationHistoryEntry>> entries_;
   HashMap<String, int> keys_to_indices_;
   int current_entry_index_ = -1;
+  bool has_dropped_navigation_ = false;
 
   Member<NavigationTransition> transition_;
 

third_party/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();

third_party/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

third_party/blink/web_tests/external/wpt/navigation-api/navigation-methods/return-value/back-204-205-download.html

@@ -0,0 +1,52 @@
+<!doctype html>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="resources/helpers.js"></script>
+<script src="/common/utils.js"></script>
+
+<body>
+<script>
+const tests = [
+  ["204s", "204"],
+  ["205s", "205"],
+  ["Content-Disposition: attachment responses", "download"]
+];
+
+for (const [description, action] of tests) {
+  promise_test(async t => {
+    const id = token();
+
+    const i = document.createElement("iframe");
+    i.src = `resources/204-205-download-on-second-visit.py?id=${id}`;
+    document.body.append(i);
+    await new Promise(r => i.onload = r);
+
+    // Configure it to return a 204 on the next visit
+    await fetch(i.src + `&action=${action}`, { method: "POST" });
+
+    // Now navigate elsewhere
+    i.contentWindow.location.href = "/common/blank.html";
+    await new Promise(r => i.onload = r);
+
+    // Now try going back. It should do nothing (and not tell us about the result).
+
+    const indexBefore = i.contentWindow.navigation.currentEntry.index;
+
+    // One might be surprised that navigate does not fire. (It does fire for the
+    // corresponding tests of navigation.navigate(), i.e., this is
+    // traversal-specific behavior.) See https://github.com/WICG/navigation-api/issues/207
+    // for some discussion.
+    i.contentWindow.navigation.onnavigate = t.unreached_func("onnavigate should not be called");
+    i.contentWindow.navigation.onnavigatesuccess = t.unreached_func("onnavigatesuccess should not be called");
+    i.contentWindow.navigation.onnavigateerror = t.unreached_func("onnavigateerror should not be called");
+
+    const result = i.contentWindow.navigation.back();
+
+    assertNeverSettles(t, result, i.contentWindow);
+
+    await new Promise(resolve => t.step_timeout(resolve, 50));
+    assert_equals(i.contentWindow.navigation.currentEntry.index, indexBefore);
+    assert_equals(i.contentWindow.navigation.transition, null);
+  }, `back() promises to ${description} never settle`);
+}
+</script>

third_party/blink/web_tests/external/wpt/navigation-api/navigation-methods/return-value/navigate-204-205-download.html

@@ -0,0 +1,42 @@
+<!doctype html>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="resources/helpers.js"></script>
+
+<body>
+<script>
+const tests = [
+  ["204s", "/common/blank.html?pipe=status(204)"],
+  ["205s", "/common/blank.html?pipe=status(205)"],
+  ["Content-Disposition: attachment responses", "/common/blank.html?pipe=header(Content-Disposition,attachment)"]
+];
+
+for (const [description, url] of tests) {
+  promise_test(async t => {
+    const i = document.createElement("iframe");
+    i.src = "/common/blank.html";
+    document.body.append(i);
+    await new Promise(resolve => i.onload = resolve);
+
+    // This seems to be important? Without it the (outer) window load event
+    // doesn't fire, and the test harness hangs forever. This is probably a
+    // Chromium bug, but maybe a testharness bug, especially since explicit_done
+    // doesn't seem to help?
+    t.add_cleanup(() => i.remove());
+
+    let navigateCount = 0;
+    i.contentWindow.navigation.onnavigate = () => { ++navigateCount; };
+    i.contentWindow.navigation.onnavigatesuccess = t.unreached_func("onnavigatesuccess should not be called");
+    i.contentWindow.navigation.onnavigateerror = t.unreached_func("onnavigateerror should not be called");
+
+    const result = i.contentWindow.navigation.navigate(url);
+
+    assert_equals(navigateCount, 1);
+    assertNeverSettles(t, result, i.contentWindow);
+
+    await new Promise(resolve => t.step_timeout(resolve, 50));
+    assert_equals(i.contentWindow.location.href, i.src);
+    assert_equals(i.contentWindow.navigation.transition, null);
+  }, `navigate() promises to ${description} never settle`);
+}
+</script>

third_party/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, [], "")