From ddf5f1eb2f2ccc52a42e2c40d7857301f3794dd7 Mon Sep 17 00:00:00 2001 From: Gregory Terzian <2792687+gterzian@users.noreply.github.com> Date: Tue, 26 Aug 2025 03:04:39 +0800 Subject: [PATCH] script: when handling page headers, stop if pipeline closed already (#38739) Previously, the script-thread would assert the pipeline was closed if no pending load was found, but it did not check whether the pipeline was closed before processing the page headers. Since incomplete loads are removed only when page headers are processed, this means the page headers were processed even if the pipeline had been closed before the page headers were available. If the pipeline had been closed as part of exiting the constellation, it was possible for the constellation to have exited by the time the page headers became available(since the script-thread closes a pipeline independently from ongoing navigation fetches), which would produce a panic on trying to communicate with the constellation to obtain the browsing context info. Note: due to the nature of the problem, I cannot verify that this fixes the crash test, although logically this appears to make sense, and a couple of days of WPT runs should tell us more. Testing: A crash test was added; unfortunately the crash was intermittent. Fixes: https://github.com/servo/servo/issues/36747 --------- Signed-off-by: gterzian <2792687+gterzian@users.noreply.github.com> --- components/script/script_thread.rs | 81 +++++++++---------- .../trusted-types-navigation.html.ini | 48 ++++++++++- tests/wpt/mozilla/meta/MANIFEST.json | 7 ++ .../mozilla/form-navigate-loop-crash.html | 10 +++ 4 files changed, 102 insertions(+), 44 deletions(-) create mode 100644 tests/wpt/mozilla/tests/mozilla/form-navigate-loop-crash.html diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index afcd816eb1c..e03430f694c 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -2840,52 +2840,51 @@ impl ScriptThread { metadata: Option, can_gc: CanGc, ) -> Option> { - let idx = self + if self.closed_pipelines.borrow().contains(id) { + // If the pipeline closed, do not process the headers. + return None; + } + + let Some(idx) = self .incomplete_loads .borrow() .iter() - .position(|load| load.pipeline_id == *id); - // The matching in progress load structure may not exist if - // the pipeline exited before the page load completed. - match idx { - Some(idx) => { - // https://html.spec.whatwg.org/multipage/#process-a-navigate-response - // 2. If response's status is 204 or 205, then abort these steps. - let is20x = match metadata { - Some(ref metadata) => metadata.status.in_range(204..=205), - _ => false, - }; + .position(|load| load.pipeline_id == *id) + else { + unreachable!("Pipeline shouldn't have finished loading."); + }; - if is20x { - // If we have an existing window that is being navigated: - if let Some(window) = self.documents.borrow().find_window(*id) { - let window_proxy = window.window_proxy(); - // https://html.spec.whatwg.org/multipage/ - // #navigating-across-documents:delaying-load-events-mode-2 - if window_proxy.parent().is_some() { - // The user agent must take this nested browsing context - // out of the delaying load events mode - // when this navigation algorithm later matures, - // or when it terminates (whether due to having run all the steps, - // or being canceled, or being aborted), whichever happens first. - window_proxy.stop_delaying_load_events_mode(); - } - } - self.senders - .pipeline_to_constellation_sender - .send((*id, ScriptToConstellationMessage::AbortLoadUrl)) - .unwrap(); - return None; - }; + // https://html.spec.whatwg.org/multipage/#process-a-navigate-response + // 2. If response's status is 204 or 205, then abort these steps. + let is_204_205 = match metadata { + Some(ref metadata) => metadata.status.in_range(204..=205), + _ => false, + }; - let load = self.incomplete_loads.borrow_mut().remove(idx); - metadata.map(|meta| self.load(meta, load, can_gc)) - }, - None => { - assert!(self.closed_pipelines.borrow().contains(id)); - None - }, - } + if is_204_205 { + // If we have an existing window that is being navigated: + if let Some(window) = self.documents.borrow().find_window(*id) { + let window_proxy = window.window_proxy(); + // https://html.spec.whatwg.org/multipage/ + // #navigating-across-documents:delaying-load-events-mode-2 + if window_proxy.parent().is_some() { + // The user agent must take this nested browsing context + // out of the delaying load events mode + // when this navigation algorithm later matures, + // or when it terminates (whether due to having run all the steps, + // or being canceled, or being aborted), whichever happens first. + window_proxy.stop_delaying_load_events_mode(); + } + } + self.senders + .pipeline_to_constellation_sender + .send((*id, ScriptToConstellationMessage::AbortLoadUrl)) + .unwrap(); + return None; + }; + + let load = self.incomplete_loads.borrow_mut().remove(idx); + metadata.map(|meta| self.load(meta, load, can_gc)) } /// Handles a request for the window title. diff --git a/tests/wpt/meta/trusted-types/trusted-types-navigation.html.ini b/tests/wpt/meta/trusted-types/trusted-types-navigation.html.ini index 6857cf0dd58..1c5656a2cd1 100644 --- a/tests/wpt/meta/trusted-types/trusted-types-navigation.html.ini +++ b/tests/wpt/meta/trusted-types/trusted-types-navigation.html.ini @@ -73,7 +73,21 @@ [trusted-types-navigation.html?26-30] - expected: CRASH + [Navigate a window via form-submission with javascript:-urls w/ default policy in enforcing mode.] + expected: FAIL + + [Navigate a window via form-submission with javascript:-urls in report-only mode.] + expected: FAIL + + [Navigate a window via form-submission with javascript:-urls w/ default policy in report-only mode.] + expected: FAIL + + [Navigate a frame via form-submission with javascript:-urls in enforcing mode.] + expected: FAIL + + [Navigate a frame via form-submission with javascript:-urls w/ default policy in enforcing mode.] + expected: FAIL + [trusted-types-navigation.html?06-10] expected: CRASH @@ -94,7 +108,22 @@ [trusted-types-navigation.html?11-15] - expected: CRASH + expected: TIMEOUT + [Navigate a window via anchor with javascript:-urls w/ a default policy making the URL invalid in enforcing mode.] + expected: FAIL + + [Navigate a window via anchor with javascript:-urls w/ a default policy making the URL invalid in report-only mode.] + expected: FAIL + + [Navigate a window via area with javascript:-urls in enforcing mode.] + expected: FAIL + + [Navigate a window via area with javascript:-urls w/ default policy in enforcing mode.] + expected: TIMEOUT + + [Navigate a window via area with javascript:-urls in report-only mode.] + expected: NOTRUN + [trusted-types-navigation.html?16-20] expected: TIMEOUT @@ -132,4 +161,17 @@ [trusted-types-navigation.html?01-05] - expected: CRASH + [Navigate a window via anchor with javascript:-urls in enforcing mode.] + expected: FAIL + + [Navigate a window via anchor with javascript:-urls w/ default policy in enforcing mode.] + expected: FAIL + + [Navigate a window via anchor with javascript:-urls in report-only mode.] + expected: FAIL + + [Navigate a window via anchor with javascript:-urls w/ default policy in report-only mode.] + expected: FAIL + + [Navigate a frame via anchor with javascript:-urls in enforcing mode.] + expected: FAIL diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json index de12c35ad2c..c866150f461 100644 --- a/tests/wpt/mozilla/meta/MANIFEST.json +++ b/tests/wpt/mozilla/meta/MANIFEST.json @@ -32,6 +32,13 @@ {} ] ], + "form-navigate-loop-crash.html": [ + "1068ccc38eed223cf83a4cbd8fed336b83c0b745", + [ + null, + {} + ] + ], "form_reset-crash.html": [ "b23cbf6aefdef8231e2cc4cb0e6416195d5bdf71", [ diff --git a/tests/wpt/mozilla/tests/mozilla/form-navigate-loop-crash.html b/tests/wpt/mozilla/tests/mozilla/form-navigate-loop-crash.html new file mode 100644 index 00000000000..1068ccc38ee --- /dev/null +++ b/tests/wpt/mozilla/tests/mozilla/form-navigate-loop-crash.html @@ -0,0 +1,10 @@ + + +
+