fix request signal (#5288)

Fixes #5286.

This makes fetch(request) use the signal stored on the Request object.
It also keeps fetch level signal overrides working the same way.

Tests:
- cargo test -p boa_runtime --lib
- cargo clippy -p boa_runtime --all-features --all-targets -- -D
warnings

Author: Monti-27

core/runtime/src/fetch/mod.rs

@@ -122,6 +122,8 @@ async fn fetch_inner<T: Fetcher>(
 
     // The resource parsing is complicated, so we parse it in Rust here (instead of relying on
     // `TryFromJs` and friends).
+    let mut signal = signal;
+
     let request: Request<Vec<u8>> = match resource {
         Either::Left(url) => {
             let url = url.to_std_string().map_err(JsError::from_rust)?;
@@ -141,10 +143,13 @@ async fn fetch_inner<T: Fetcher>(
                 return Err(js_error!(TypeError: "Request object is already in use"));
             };
 
+            signal = signal.or_else(|| request_ref.data().signal());
             request_ref.data().inner().clone()
         }
     };
 
+    check_abort(signal.as_ref(), &mut context.borrow_mut())?;
+
     let mut request = if let Some(options) = options {
         options.into_request_builder(Some(request))?
     } else {

core/runtime/src/fetch/request.rs

@@ -96,6 +96,7 @@ impl RequestInit {
 pub struct JsRequest {
     #[unsafe_ignore_trace]
     inner: HttpRequest<Vec<u8>>,
+    signal: Option<JsObject>,
 }
 
 impl JsRequest {
@@ -104,11 +105,23 @@ impl JsRequest {
         mem::replace(&mut self.inner, HttpRequest::new(Vec::new()))
     }
 
+    /// Split this request into its HTTP request and abort signal.
+    fn into_parts(mut self) -> (HttpRequest<Vec<u8>>, Option<JsObject>) {
+        let request = mem::replace(&mut self.inner, HttpRequest::new(Vec::new()));
+        let signal = self.signal.take();
+        (request, signal)
+    }
+
     /// Get a reference to the inner `http::Request` object.
     pub fn inner(&self) -> &HttpRequest<Vec<u8>> {
         &self.inner
     }
 
+    /// Get the abort signal associated with this request, if any.
+    pub(crate) fn signal(&self) -> Option<JsObject> {
+        self.signal.clone()
+    }
+
     /// Get the URI of the request.
     pub fn uri(&self) -> &http::Uri {
         self.inner.uri()
@@ -123,33 +136,41 @@ impl JsRequest {
         input: Either<JsString, JsRequest>,
         options: Option<RequestInit>,
     ) -> JsResult<Self> {
-        let request = match input {
+        let (request, signal) = match input {
             Either::Left(uri) => {
                 let uri = http::Uri::try_from(
                     uri.to_std_string()
                         .map_err(|_| js_error!(URIError: "URI cannot have unpaired surrogates"))?,
                 )
                 .map_err(|_| js_error!(URIError: "Invalid URI"))?;
-                http::request::Request::builder()
+                let request = http::request::Request::builder()
                     .uri(uri)
                     .body(Vec::<u8>::new())
-                    .map_err(|_| js_error!(Error: "Cannot construct request"))?
+                    .map_err(|_| js_error!(Error: "Cannot construct request"))?;
+                (request, None)
             }
-            Either::Right(r) => r.into_inner(),
+            Either::Right(r) => r.into_parts(),
         };
 
-        if let Some(options) = options {
+        if let Some(mut options) = options {
+            let signal = options.take_signal().or(signal);
             let inner = options.into_request_builder(Some(request))?;
-            Ok(Self { inner })
+            Ok(Self { inner, signal })
         } else {
-            Ok(Self { inner: request })
+            Ok(Self {
+                inner: request,
+                signal,
+            })
         }
     }
 }
 
 impl From<HttpRequest<Vec<u8>>> for JsRequest {
     fn from(inner: HttpRequest<Vec<u8>>) -> Self {
-        Self { inner }
+        Self {
+            inner,
+            signal: None,
+        }
     }
 }
 
@@ -181,8 +202,6 @@ impl JsRequest {
 
     #[boa(rename = "clone")]
     fn clone_request(&self) -> Self {
-        Self {
-            inner: self.inner.clone(),
-        }
+        self.clone()
     }
 }

core/runtime/src/fetch/tests/request.rs

@@ -2,7 +2,7 @@ use super::TestFetcher;
 use crate::fetch::request::JsRequest;
 use crate::fetch::response::JsResponse;
 use crate::test::{TestAction, run_test_actions};
-use boa_engine::{js_str, js_string};
+use boa_engine::{JsObject, js_str, js_string};
 use either::Either;
 use http::{Response, Uri};
 
@@ -215,3 +215,119 @@ fn request_clone_method_is_independent() {
         }),
     ]);
 }
+
+#[test]
+fn request_stores_signal() {
+    run_test_actions([
+        TestAction::inspect_context(|ctx| {
+            let fetcher = TestFetcher::default();
+            crate::fetch::register(fetcher, None, ctx).expect("failed to register fetch");
+        }),
+        TestAction::run(
+            r#"
+                globalThis.ctrl = new AbortController();
+                globalThis.request = new Request("http://unit.test", {
+                    signal: ctrl.signal,
+                });
+            "#,
+        ),
+        TestAction::inspect_context(|ctx| {
+            let request = ctx.global_object().get(js_str!("request"), ctx).unwrap();
+            let request_obj = request.as_object().unwrap();
+            let request = request_obj.downcast_ref::<JsRequest>().unwrap();
+
+            let signal = ctx.global_object().get(js_str!("ctrl"), ctx).unwrap();
+            let signal = signal
+                .as_object()
+                .unwrap()
+                .get(js_str!("signal"), ctx)
+                .unwrap()
+                .as_object()
+                .unwrap();
+
+            let stored_signal = request.signal().expect("request should keep its signal");
+            assert!(JsObject::equals(&stored_signal, &signal));
+        }),
+    ]);
+}
+
+#[test]
+fn request_clone_preserves_signal_without_override() {
+    run_test_actions([
+        TestAction::inspect_context(|ctx| {
+            let fetcher = TestFetcher::default();
+            crate::fetch::register(fetcher, None, ctx).expect("failed to register fetch");
+        }),
+        TestAction::run(
+            r#"
+                globalThis.ctrl = new AbortController();
+                const original = new Request("http://unit.test", {
+                    signal: ctrl.signal,
+                });
+                globalThis.cloned = new Request(original, {
+                    headers: { "x-test": "1" },
+                });
+            "#,
+        ),
+        TestAction::inspect_context(|ctx| {
+            let cloned = ctx.global_object().get(js_str!("cloned"), ctx).unwrap();
+            let cloned_obj = cloned.as_object().unwrap();
+            let cloned = cloned_obj.downcast_ref::<JsRequest>().unwrap();
+
+            let signal = ctx.global_object().get(js_str!("ctrl"), ctx).unwrap();
+            let signal = signal
+                .as_object()
+                .unwrap()
+                .get(js_str!("signal"), ctx)
+                .unwrap()
+                .as_object()
+                .unwrap();
+
+            let stored_signal = cloned
+                .signal()
+                .expect("cloned request should keep its signal");
+            assert!(JsObject::equals(&stored_signal, &signal));
+        }),
+    ]);
+}
+
+#[test]
+fn request_clone_signal_override() {
+    run_test_actions([
+        TestAction::inspect_context(|ctx| {
+            let fetcher = TestFetcher::default();
+            crate::fetch::register(fetcher, None, ctx).expect("failed to register fetch");
+        }),
+        TestAction::run(
+            r#"
+                globalThis.ctrl1 = new AbortController();
+                globalThis.ctrl2 = new AbortController();
+                const original = new Request("http://unit.test", {
+                    signal: ctrl1.signal,
+                });
+                globalThis.cloned = new Request(original, {
+                    signal: ctrl2.signal,
+                });
+            "#,
+        ),
+        TestAction::inspect_context(|ctx| {
+            let cloned = ctx.global_object().get(js_str!("cloned"), ctx).unwrap();
+            let cloned_obj = cloned.as_object().unwrap();
+            let cloned = cloned_obj.downcast_ref::<JsRequest>().unwrap();
+
+            let signal = ctx.global_object().get(js_str!("ctrl2"), ctx).unwrap();
+            let signal = signal
+                .as_object()
+                .unwrap()
+                .get(js_str!("signal"), ctx)
+                .unwrap()
+                .as_object()
+                .unwrap();
+
+            let stored_signal = cloned
+                .signal()
+                .expect("overridden request should keep the new signal");
+            assert!(JsObject::equals(&stored_signal, &signal));
+        }),
+    ]);
+}

core/runtime/src/fetch/tests/response.rs

@@ -178,6 +178,83 @@ fn response_getter() {
     ]);
 }
 
+#[test]
+fn fetch_request_uses_request_signal() {
+    run_test_actions([
+        TestAction::harness(),
+        TestAction::inspect_context(|ctx| {
+            register(
+                &[("http://unit.test", Response::new(b"Hello World".to_vec()))],
+                ctx,
+            );
+        }),
+        TestAction::run(
+            r#"
+                globalThis.response = (async () => {
+                    const ctrl = new AbortController();
+                    const request = new Request("http://unit.test", { signal: ctrl.signal });
+                    ctrl.abort("stop");
+
+                    try {
+                        await fetch(request);
+                        return "fulfilled";
+                    } catch (e) {
+                        return e;
+                    }
+                })();
+            "#,
+        ),
+        TestAction::inspect_context(|ctx| {
+            let response = ctx.global_object().get(js_str!("response"), ctx).unwrap();
+            let response = response.as_promise().unwrap().await_blocking(ctx).unwrap();
+            assert_eq!(
+                response.as_string().map(|s| s.to_std_string_escaped()),
+                Some("stop".into())
+            );
+        }),
+    ]);
+}
+
+#[test]
+fn fetch_options_signal_overrides_request_signal() {
+    run_test_actions([
+        TestAction::harness(),
+        TestAction::inspect_context(|ctx| {
+            register(
+                &[("http://unit.test", Response::new(b"Hello World".to_vec()))],
+                ctx,
+            );
+        }),
+        TestAction::run(
+            r#"
+                globalThis.response = (async () => {
+                    const requestCtrl = new AbortController();
+                    const fetchCtrl = new AbortController();
+                    const request = new Request("http://unit.test", {
+                        signal: requestCtrl.signal,
+                    });
+                    fetchCtrl.abort("fetch stop");
+
+                    try {
+                        await fetch(request, { signal: fetchCtrl.signal });
+                        return "fulfilled";
+                    } catch (e) {
+                        return e;
+                    }
+                })();
+            "#,
+        ),
+        TestAction::inspect_context(|ctx| {
+            let response = ctx.global_object().get(js_str!("response"), ctx).unwrap();
+            let response = response.as_promise().unwrap().await_blocking(ctx).unwrap();
+            assert_eq!(
+                response.as_string().map(|s| s.to_std_string_escaped()),
+                Some("fetch stop".into())
+            );
+        }),
+    ]);
+}
+
 #[test]
 fn response_redirect_default_status() {
     run_test_actions([