From 7abc813fc38aa8f872089ac4c7c2fd08d16571f8 Mon Sep 17 00:00:00 2001 From: Tim van der Lippe Date: Sun, 21 Sep 2025 14:33:03 +0200 Subject: [PATCH] Abort fetch controller when signal is aborted (#39374) Does not all tests pass because of a mismatch in microtask timing. The promises are resolved/rejected in the wrong order. Part of #34866 Signed-off-by: Tim van der Lippe --- components/script/dom/abortsignal.rs | 16 ++- components/script/fetch.rs | 133 ++++++++++++------ .../meta/fetch/api/abort/general.any.js.ini | 74 ---------- .../meta/fetch/api/abort/keepalive.html.ini | 7 - 4 files changed, 103 insertions(+), 127 deletions(-) delete mode 100644 tests/wpt/meta/fetch/api/abort/keepalive.html.ini diff --git a/components/script/dom/abortsignal.rs b/components/script/dom/abortsignal.rs index 3cce9766741..3444f62958c 100644 --- a/components/script/dom/abortsignal.rs +++ b/components/script/dom/abortsignal.rs @@ -3,6 +3,7 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ use std::cell::{Cell, RefCell}; +use std::sync::{Arc, Mutex}; use dom_struct::dom_struct; use indexmap::IndexSet; @@ -20,6 +21,7 @@ use crate::dom::bindings::root::{Dom, DomRoot}; use crate::dom::eventtarget::EventTarget; use crate::dom::globalscope::GlobalScope; use crate::dom::readablestream::PipeTo; +use crate::fetch::FetchContext; use crate::realms::InRealm; use crate::script_runtime::{CanGc, JSContext as SafeJSContext}; @@ -37,7 +39,11 @@ pub(crate) enum AbortAlgorithm { /// StreamPiping(PipeTo), /// - Fetch, + Fetch( + #[no_trace] + #[conditional_malloc_size_of] + Arc>, + ), } /// @@ -162,6 +168,14 @@ impl AbortSignal { reason.set(self.abort_reason.get()); pipe.abort_with_reason(cx, global, reason.handle(), realm, can_gc); }, + AbortAlgorithm::Fetch(fetch_context) => { + rooted!(in(*cx) let mut reason = UndefinedValue()); + reason.set(self.abort_reason.get()); + fetch_context + .lock() + .unwrap() + .abort_fetch(reason.handle(), cx, can_gc); + }, _ => { // TODO: match on variant and implement algo steps. // See the various items of #34866 diff --git a/components/script/fetch.rs b/components/script/fetch.rs index 3416ec7acef..ae9b401f7e6 100644 --- a/components/script/fetch.rs +++ b/components/script/fetch.rs @@ -21,6 +21,8 @@ use net_traits::{ }; use servo_url::ServoUrl; +use crate::body::BodyMixin; +use crate::dom::abortsignal::AbortAlgorithm; use crate::dom::bindings::codegen::Bindings::AbortSignalBinding::AbortSignalMethods; use crate::dom::bindings::codegen::Bindings::RequestBinding::{ RequestInfo, RequestInit, RequestMethods, @@ -46,12 +48,6 @@ use crate::network_listener::{self, PreInvoke, ResourceTimingListener, submit_ti use crate::realms::{InRealm, enter_realm}; use crate::script_runtime::CanGc; -struct FetchContext { - fetch_promise: Option, - response_object: Trusted, - resource_timing: ResourceFetchTiming, -} - /// RAII fetch canceller object. /// By default initialized to having a /// request associated with it, which can be manually cancelled with `cancel`, @@ -146,22 +142,32 @@ fn request_init_from_request(request: NetTraitsRequest) -> RequestBuilder { /// fn abort_fetch_call( promise: Rc, - _request: &NetTraitsRequest, - _response_object: Option<&Response>, + request: &Request, + response_object: Option<&Response>, abort_reason: HandleValue, + global: &GlobalScope, cx: SafeJSContext, can_gc: CanGc, ) { // Step 1. Reject promise with error. promise.reject(cx, abort_reason, can_gc); // Step 2. If request’s body is non-null and is readable, then cancel request’s body with error. - // TODO + if let Some(body) = request.body() { + if body.is_readable() { + body.cancel(cx, global, abort_reason, can_gc); + } + } // Step 3. If responseObject is null, then return. - // TODO // Step 4. Let response be responseObject’s response. - // TODO + let Some(response) = response_object else { + return; + }; // Step 5. If response’s body is non-null and is readable, then error response’s body with error. - // TODO + if let Some(body) = response.body() { + if body.is_readable() { + body.error(abort_reason, can_gc); + } + } } /// @@ -196,6 +202,7 @@ pub(crate) fn Fetch( // Step 3. Let request be requestObject’s request. let request = request_object.get_request(); let timing_type = request.timing_type(); + let request_id = request.id; // Step 4. If requestObject’s signal is aborted, then: let signal = request_object.Signal(); @@ -205,9 +212,10 @@ pub(crate) fn Fetch( signal.Reason(cx, abort_reason.handle_mut()); abort_fetch_call( promise.clone(), - &request, + &request_object, None, abort_reason.handle(), + global, cx, can_gc, ); @@ -232,43 +240,22 @@ pub(crate) fn Fetch( // Is `comp` as argument // Step 9. Let locallyAborted be false. - // TODO // Step 10. Let controller be null. - // TODO - // Step 11. Add the following abort steps to requestObject’s signal: - // TODO - // Step 11.1. Set locallyAborted to true. - // TODO - // Step 11.2. Assert: controller is non-null. - // TODO - // Step 11.3. Abort controller with requestObject’s signal’s abort reason. - // TODO - // Step 11.4. Abort the fetch() call with p, request, responseObject, and requestObject’s signal’s abort reason. - // TODO - - // Step 12. Set controller to the result of calling fetch given request and - // processResponse given response being these steps: let fetch_context = Arc::new(Mutex::new(FetchContext { fetch_promise: Some(TrustedPromise::new(promise.clone())), response_object: Trusted::new(&*response), + request: Trusted::new(&*request_object), + global: Trusted::new(global), resource_timing: ResourceFetchTiming::new(timing_type), + locally_aborted: false, + canceller: FetchCanceller::new(request_id, global.core_resource_thread()), })); - // Step 12.1. If locallyAborted is true, then abort these steps. - // TODO - // Step 12.2. If response’s aborted flag is set, then: - // TODO - // Step 12.2.1. Let deserializedError be the result of deserialize a serialized - // abort reason given controller’s serialized abort reason and relevantRealm. - // TODO - // Step 12.2.2. Abort the fetch() call with p, request, responseObject, and deserializedError. - // TODO - // Step 12.2.3. Abort these steps. - // TODO + // Step 11. Add the following abort steps to requestObject’s signal: + signal.add(&AbortAlgorithm::Fetch(fetch_context.clone())); - // Step 12.3. If response is a network error, then reject p with a TypeError and abort these steps. - // Step 12.4. Set responseObject to the result of creating a Response object, given response, "immutable", and relevantRealm. - // Step 12.5. Resolve p with responseObject. + // Step 12. Set controller to the result of calling fetch given request and + // processResponse given response being these steps: global.fetch( request_init, fetch_context, @@ -279,8 +266,58 @@ pub(crate) fn Fetch( promise } +#[derive(JSTraceable, MallocSizeOf)] +pub(crate) struct FetchContext { + #[ignore_malloc_size_of = "unclear ownership semantics"] + fetch_promise: Option, + response_object: Trusted, + request: Trusted, + global: Trusted, + #[no_trace] + resource_timing: ResourceFetchTiming, + locally_aborted: bool, + canceller: FetchCanceller, +} + impl PreInvoke for FetchContext {} +impl FetchContext { + /// Step 11 of + pub(crate) fn abort_fetch( + &mut self, + abort_reason: HandleValue, + cx: SafeJSContext, + can_gc: CanGc, + ) { + // Step 11.1. Set locallyAborted to true. + self.locally_aborted = true; + // Step 11.2. Assert: controller is non-null. + // + // N/a, that's self + + // Step 11.3. Abort controller with requestObject’s signal’s abort reason. + self.canceller.cancel(); + + // Step 11.4. Abort the fetch() call with p, request, responseObject, + // and requestObject’s signal’s abort reason. + let promise = self + .fetch_promise + .take() + .expect("fetch promise is missing") + .root(); + abort_fetch_call( + promise, + &self.request.root(), + Some(&self.response_object.root()), + abort_reason, + &self.global.root(), + cx, + can_gc, + ); + } +} + +/// Step 12 of impl FetchResponseListener for FetchContext { fn process_request_body(&mut self, _: RequestId) { // TODO @@ -296,6 +333,10 @@ impl FetchResponseListener for FetchContext { _: RequestId, fetch_metadata: Result, ) { + // Step 12.1. If locallyAborted is true, then abort these steps. + if self.locally_aborted { + return; + } let promise = self .fetch_promise .take() @@ -304,7 +345,8 @@ impl FetchResponseListener for FetchContext { let _ac = enter_realm(&*promise); match fetch_metadata { - // Step 4.1 + // Step 12.3. If response is a network error, then reject + // p with a TypeError and abort these steps. Err(_) => { promise.reject_error( Error::Type("Network error occurred".to_string()), @@ -319,7 +361,8 @@ impl FetchResponseListener for FetchContext { ); return; }, - // Step 4.2 + // Step 12.4. Set responseObject to the result of creating a Response object, + // given response, "immutable", and relevantRealm. Ok(metadata) => match metadata { FetchMetadata::Unfiltered(m) => { fill_headers_with_metadata(self.response_object.root(), m, CanGc::note()); @@ -354,7 +397,7 @@ impl FetchResponseListener for FetchContext { }, } - // Step 4.3 + // Step 12.5. Resolve p with responseObject. promise.resolve_native(&self.response_object.root(), CanGc::note()); self.fetch_promise = Some(TrustedPromise::new(promise)); } diff --git a/tests/wpt/meta/fetch/api/abort/general.any.js.ini b/tests/wpt/meta/fetch/api/abort/general.any.js.ini index 1913b8db956..ce6390511b8 100644 --- a/tests/wpt/meta/fetch/api/abort/general.any.js.ini +++ b/tests/wpt/meta/fetch/api/abort/general.any.js.ini @@ -5,7 +5,6 @@ expected: ERROR [general.any.html] - expected: TIMEOUT [Request is still 'used' if signal is aborted before fetching] expected: FAIL @@ -27,48 +26,11 @@ [Call text() twice on aborted response] expected: FAIL - [Underlying connection is closed when aborting after receiving response] - expected: FAIL - - [Underlying connection is closed when aborting after receiving response - no-cors] - expected: FAIL - - [Fetch aborted & connection closed when aborted after calling response.arrayBuffer()] - expected: TIMEOUT - - [Fetch aborted & connection closed when aborted after calling response.blob()] - expected: NOTRUN - - [Fetch aborted & connection closed when aborted after calling response.formData()] - expected: NOTRUN - - [Fetch aborted & connection closed when aborted after calling response.json()] - expected: NOTRUN - - [Fetch aborted & connection closed when aborted after calling response.text()] - expected: NOTRUN - - [Stream errors once aborted. Underlying connection closed.] - expected: NOTRUN - - [Stream errors once aborted, after reading. Underlying connection closed.] - expected: NOTRUN - - [Stream will not error if body is empty. It's closed with an empty queue before it errors.] - expected: NOTRUN - - [Readable stream synchronously cancels with AbortError if aborted before reading] - expected: NOTRUN - [response.bytes() rejects if already aborted] expected: FAIL - [Fetch aborted & connection closed when aborted after calling response.bytes()] - expected: NOTRUN - [general.any.worker.html] - expected: TIMEOUT [Request is still 'used' if signal is aborted before fetching] expected: FAIL @@ -90,41 +52,5 @@ [Call text() twice on aborted response] expected: FAIL - [Underlying connection is closed when aborting after receiving response] - expected: FAIL - - [Underlying connection is closed when aborting after receiving response - no-cors] - expected: FAIL - - [Fetch aborted & connection closed when aborted after calling response.arrayBuffer()] - expected: TIMEOUT - - [Fetch aborted & connection closed when aborted after calling response.blob()] - expected: NOTRUN - - [Fetch aborted & connection closed when aborted after calling response.formData()] - expected: NOTRUN - - [Fetch aborted & connection closed when aborted after calling response.json()] - expected: NOTRUN - - [Fetch aborted & connection closed when aborted after calling response.text()] - expected: NOTRUN - - [Stream errors once aborted. Underlying connection closed.] - expected: NOTRUN - - [Stream errors once aborted, after reading. Underlying connection closed.] - expected: NOTRUN - - [Stream will not error if body is empty. It's closed with an empty queue before it errors.] - expected: NOTRUN - - [Readable stream synchronously cancels with AbortError if aborted before reading] - expected: NOTRUN - [response.bytes() rejects if already aborted] expected: FAIL - - [Fetch aborted & connection closed when aborted after calling response.bytes()] - expected: NOTRUN diff --git a/tests/wpt/meta/fetch/api/abort/keepalive.html.ini b/tests/wpt/meta/fetch/api/abort/keepalive.html.ini deleted file mode 100644 index 46b5a14febd..00000000000 --- a/tests/wpt/meta/fetch/api/abort/keepalive.html.ini +++ /dev/null @@ -1,7 +0,0 @@ -[keepalive.html] - expected: TIMEOUT - [aborting a keepalive fetch should work] - expected: TIMEOUT - - [aborting a detached keepalive fetch should not do anything] - expected: NOTRUN