script: abort planned form navigations (#38676)

Servo shows a behavior unlike FF and Safari(I don't have Chrome), where
stopping a window does not cancel planned form navigation, resulting in
an infinite navigation loop. The current behavior of Servo does seem to
follow the wording of the spec, so I will open a [companion issue at the
spec](https://github.com/whatwg/html/issues/11562), and I have also
written a WPT tests for the non-standard but widely followed behavior.
This PR also adds a beginning of an implementation of the "ongoing
navigation" concept, which is used by the spec to cancel navigations,
and which is used in this PR only to cancel planned form navigations.
The generation id concept, which corresponds to the planned navigation
concept in the spec, is turned into a simple struct private cell, and is
documented per the spec.

Testing: A new WPT test is added
Fixes: Only one part of https://github.com/servo/servo/issues/36747

---------

Signed-off-by: gterzian <2792687+gterzian@users.noreply.github.com>
Signed-off-by: Gregory Terzian <2792687+gterzian@users.noreply.github.com>
Co-authored-by: Martin Robinson <mrobinson@igalia.com>

components/script/dom/htmlformelement.rs

@@ -82,9 +82,6 @@ use crate::links::{LinkRelations, get_element_target};
 use crate::script_runtime::CanGc;
 use crate::script_thread::ScriptThread;
 
-#[derive(Clone, Copy, JSTraceable, MallocSizeOf, PartialEq)]
-pub(crate) struct GenerationId(u32);
-
 #[dom_struct]
 pub(crate) struct HTMLFormElement {
     htmlelement: HTMLElement,
@@ -92,7 +89,6 @@ pub(crate) struct HTMLFormElement {
     /// <https://html.spec.whatwg.org/multipage/#constructing-entry-list>
     constructing_entry_list: Cell<bool>,
     elements: DomOnceCell<HTMLFormControlsCollection>,
-    generation_id: Cell<GenerationId>,
     controls: DomRefCell<Vec<Dom<Element>>>,
 
     #[allow(clippy::type_complexity)]
@@ -104,6 +100,9 @@ pub(crate) struct HTMLFormElement {
     firing_submission_events: Cell<bool>,
     rel_list: MutNullableDom<DOMTokenList>,
 
+    /// <https://html.spec.whatwg.org/multipage/#planned-navigation>
+    planned_navigation: Cell<usize>,
+
     #[no_trace]
     relations: Cell<LinkRelations>,
 }
@@ -124,12 +123,12 @@ impl HTMLFormElement {
             marked_for_reset: Cell::new(false),
             constructing_entry_list: Cell::new(false),
             elements: Default::default(),
-            generation_id: Cell::new(GenerationId(0)),
             controls: DomRefCell::new(Vec::new()),
             past_names_map: DomRefCell::new(HashMapTracedValues::new()),
             current_name_generation: Cell::new(0),
             firing_submission_events: Cell::new(false),
             rel_list: Default::default(),
+            planned_navigation: Default::default(),
             relations: Cell::new(LinkRelations::empty()),
         }
     }
@@ -1004,14 +1003,10 @@ impl HTMLFormElement {
 
     /// [Planned navigation](https://html.spec.whatwg.org/multipage/#planned-navigation)
     fn plan_to_navigate(&self, mut load_data: LoadData, target: &Window) {
-        // Step 1
-        // Each planned navigation task is tagged with a generation ID, and
-        // before the task is handled, it first checks whether the HTMLFormElement's
-        // generation ID is the same as its own generation ID.
-        let generation_id = GenerationId(self.generation_id.get().0 + 1);
-        self.generation_id.set(generation_id);
-
-        // Step 2
+        // 1. Let referrerPolicy be the empty string.
+        // 2. If the form element's link types include the noreferrer keyword,
+        //    then set referrerPolicy to "no-referrer".
+        // Note: both steps done below.
         let elem = self.upcast::<Element>();
         let referrer = match elem.get_attribute(&ns!(), &local_name!("rel")) {
             Some(ref link_types) if link_types.Value().contains("noreferrer") => {
@@ -1020,18 +1015,53 @@ impl HTMLFormElement {
             _ => target.as_global_scope().get_referrer(),
         };
 
+        // 3. If the form has a non-null planned navigation, remove it from its task queue.
+        // Note: done by incrementing `planned_navigation`.
+        self.planned_navigation
+            .set(self.planned_navigation.get().wrapping_add(1));
+        let planned_navigation = self.planned_navigation.get();
+
+        // Note: we start to use
+        // the beginnings of an `ongoing_navigation` concept,
+        // to cancel planned navigations as part of
+        // <https://html.spec.whatwg.org/multipage/#nav-stop>
+        //
+        // The concept of ongoing navigation must be separated from the form's
+        // planned navigation concept, because each planned navigation cancels the previous one
+        // for a given form, whereas an ongoing navigation is a per navigable (read: window for now)
+        // concept.
+        //
+        // Setting the ongoing navigation now means the navigation could be cancelled
+        // even if the below task has not run yet. This is not how the spec is written: it
+        // seems instead to imply that a `window.stop` should only cancel the navigation
+        // that has already started (here the task is queued, but the navigation starts only
+        // in the task). See <https://github.com/whatwg/html/issues/11562>.
+        let ongoing_navigation = target.set_ongoing_navigation();
+
         let referrer_policy = target.Document().get_referrer_policy();
         load_data.creator_pipeline_id = Some(target.pipeline_id());
         load_data.referrer = referrer;
         load_data.referrer_policy = referrer_policy;
 
-        // Step 4.
-        let this = Trusted::new(self);
+        // 4. Queue an element task on the DOM manipulation task source
+        // given the form element and the following steps:
+        let form = Trusted::new(self);
         let window = Trusted::new(target);
         let task = task!(navigate_to_form_planned_navigation: move || {
-            if generation_id != this.root().generation_id.get() {
+            // 4.1 Set the form's planned navigation to null.
+            // Note: we implement the equivalent by incrementing the counter above,
+            // and checking it here.
+            if planned_navigation != form.root().planned_navigation.get() {
                 return;
             }
+
+            // Note: we also check if the navigation has been cancelled,
+            // see https://github.com/whatwg/html/issues/11562
+            if ongoing_navigation != window.root().ongoing_navigation() {
+                return;
+            }
+
+            // 4.2 Navigate targetNavigable to url
             window
                 .root()
                 .load_url(
@@ -1042,7 +1072,10 @@ impl HTMLFormElement {
                 );
         });
 
-        // Step 3.
+        // 5. Set the form's planned navigation to the just-queued task.
+        // Done above as part of incrementing the planned navigation counter.
+
+        // Note: task queued here.
         target
             .global()
             .task_manager()

components/script/dom/window.rs

@@ -227,6 +227,11 @@ impl LayoutBlocker {
     }
 }
 
+/// An id used to cancel navigations; for now only used for planned form navigations.
+/// Loosely based on <https://html.spec.whatwg.org/multipage/#ongoing-navigation>.
+#[derive(Clone, Copy, Debug, Default, JSTraceable, MallocSizeOf, PartialEq)]
+pub(crate) struct OngoingNavigation(u32);
+
 type PendingImageRasterizationKey = (PendingImageId, DeviceIntSize);
 
 #[dom_struct]
@@ -267,6 +272,10 @@ pub(crate) struct Window {
     status: DomRefCell<DOMString>,
     trusted_types: MutNullableDom<TrustedTypePolicyFactory>,
 
+    /// The start of something resembling
+    /// <https://html.spec.whatwg.org/multipage/#ongoing-navigation>
+    ongoing_navigation: Cell<OngoingNavigation>,
+
     /// For sending timeline markers. Will be ignored if
     /// no devtools server
     #[no_trace]
@@ -713,6 +722,49 @@ impl Window {
     pub(crate) fn font_context(&self) -> &Arc<FontContext> {
         &self.font_context
     }
+
+    pub(crate) fn ongoing_navigation(&self) -> OngoingNavigation {
+        self.ongoing_navigation.get()
+    }
+
+    /// <https://html.spec.whatwg.org/multipage/#set-the-ongoing-navigation>
+    pub(crate) fn set_ongoing_navigation(&self) -> OngoingNavigation {
+        // Note: since this value, for now, is only used in a single `ScriptThread`,
+        // we just increment it (it is not a uuid), which implies not
+        // using a `newValue` variable.
+        let new_value = self.ongoing_navigation.get().0.wrapping_add(1);
+
+        // 1. If navigable's ongoing navigation is equal to newValue, then return.
+        // Note: cannot happen in the way it is currently used.
+
+        // TODO: 2. Inform the navigation API about aborting navigation given navigable.
+
+        // 3. Set navigable's ongoing navigation to newValue.
+        self.ongoing_navigation.set(OngoingNavigation(new_value));
+
+        // Note: Return the ongoing navigation for the caller to use.
+        OngoingNavigation(new_value)
+    }
+
+    /// <https://html.spec.whatwg.org/multipage/#nav-stop>
+    fn stop_loading(&self, can_gc: CanGc) {
+        // 1. Let document be navigable's active document.
+        let doc = self.Document();
+
+        // 2. If document's unload counter is 0,
+        // and navigable's ongoing navigation is a navigation ID,
+        // then set the ongoing navigation for navigable to null.
+        //
+        // Note: since the concept of `navigable` is nascent in Servo,
+        // for now we do two things:
+        // - increment the `ongoing_navigation`(preventing planned form navigations).
+        // - Send a `AbortLoadUrl` message(in case the navigation
+        // already started at the constellation).
+        self.set_ongoing_navigation();
+
+        // 3. Abort a document and its descendants given document.
+        doc.abort(can_gc);
+    }
 }
 
 // https://html.spec.whatwg.org/multipage/#atob
@@ -868,9 +920,11 @@ impl WindowMethods<crate::DomTypeHolder> for Window {
 
     // https://html.spec.whatwg.org/multipage/#dom-window-stop
     fn Stop(&self, can_gc: CanGc) {
-        // TODO: Cancel ongoing navigation.
-        let doc = self.Document();
-        doc.abort(can_gc);
+        // 1. If this's navigable is null, then return.
+        // Note: Servo doesn't have a concept of navigable yet.
+
+        // 2. Stop loading this's navigable.
+        self.stop_loading(can_gc);
     }
 
     /// <https://html.spec.whatwg.org/multipage/#dom-window-focus>
@@ -3097,6 +3151,7 @@ impl Window {
                 inherited_secure_context,
                 unminify_js,
             ),
+            ongoing_navigation: Default::default(),
             script_chan,
             layout: RefCell::new(layout),
             font_context,

tests/wpt/meta/MANIFEST.json

@@ -698765,6 +698765,13 @@
          {}
         ]
        ],
+       "form-submit-and-window-stop.html": [
+        "b1659d0d2686e8c538f4286b7f88d851ea532f09",
+        [
+         null,
+         {}
+        ]
+       ],
        "grandparent_location_aboutsrcdoc.sub.window.js": [
         "c182c29cfc7d7562de13257c74ff3e8085d110a9",
         [

tests/wpt/tests/html/browsers/browsing-the-web/navigating-across-documents/form-submit-and-window-stop.html

@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>The planned navigation of a form should be cancelled by a window stop
+  in the same task as the form submission</title>
+<link rel="help" href="https://github.com/whatwg/html/issues/3730">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="resources/helpers.js"></script>
+
+<body>
+<script>
+"use strict";
+async_test(t => {
+  let unloaded = false;
+  window.onload = t.step_func(() => {
+    form.submit();
+    window.stop();
+  });
+  window.onunload = () => {
+    unloaded = true;
+  };
+  t.step_timeout(() => {
+    // The document should not have unloaded.
+    assert_equals(unloaded, false);
+    t.done();
+  }, 100);
+});
+</script>
+<form id="form">