From d76b4a14df1f444ba3e84f66dfd80211d9787403 Mon Sep 17 00:00:00 2001 From: Jason Tsai Date: Tue, 27 May 2025 11:27:13 +0900 Subject: [PATCH] refactor: merge `cross_realm_transform_*` fields into one (#37102) In https://github.com/servo/servo/pull/36977, when transferring `TransformStream`, `CrossRealmTransform::Writable` and `CrossRealmTransform::Readable` are set to different message ports. The message port will not be readable and writable at the same time when transferring the stream, so we can now merge `cross_realm_transform_readable` and `cross_realm_transform_writable` into a single field `cross_realm_transform`. Testing: WPT ([passed on try branch](https://github.com/pewsheen/servo/actions/runs/15209389525/job/42784179519)) Fixes: https://github.com/servo/servo/issues/37084 --------- Signed-off-by: Jason Tsai --- components/script/dom/globalscope.rs | 114 +++++++++++------------ components/script/dom/transformstream.rs | 15 +++ 2 files changed, 68 insertions(+), 61 deletions(-) diff --git a/components/script/dom/globalscope.rs b/components/script/dom/globalscope.rs index ec2ed70dd15..b50f0b1e75f 100644 --- a/components/script/dom/globalscope.rs +++ b/components/script/dom/globalscope.rs @@ -74,6 +74,7 @@ use super::bindings::codegen::Bindings::WebGPUBinding::GPUDeviceLostReason; use super::bindings::error::Fallible; use super::bindings::trace::{HashMapTracedValues, RootedTraceableBox}; use super::serviceworkerglobalscope::ServiceWorkerGlobalScope; +use super::transformstream::CrossRealmTransform; use crate::dom::bindings::cell::{DomRefCell, RefMut}; use crate::dom::bindings::codegen::Bindings::BroadcastChannelBinding::BroadcastChannelMethods; use crate::dom::bindings::codegen::Bindings::EventSourceBinding::EventSource_Binding::EventSourceMethods; @@ -458,13 +459,9 @@ pub(crate) struct ManagedMessagePort { /// Whether the port has been closed by script in this global, /// so it can be removed. explicitly_closed: bool, - /// Note: it may seem strange to use a pair of options, versus for example an enum. - /// But it looks like tranform streams will require both of those in their transfer. - /// This will be resolved when we reach that point of the implementation. - /// - cross_realm_transform_readable: Option, - /// - cross_realm_transform_writable: Option, + /// The handler for `message` or `messageerror` used in the cross realm transform, + /// if any was setup with this port. + cross_realm_transform: Option, } /// State representing whether this global is currently managing broadcast channels. @@ -1345,7 +1342,9 @@ impl GlobalScope { unreachable!("Cross realm transform readable must match a managed port"); }; - managed_port.cross_realm_transform_readable = Some(cross_realm_transform_readable.clone()); + managed_port.cross_realm_transform = Some(CrossRealmTransform::Readable( + cross_realm_transform_readable.clone(), + )); } /// @@ -1368,7 +1367,9 @@ impl GlobalScope { unreachable!("Cross realm transform writable must match a managed port"); }; - managed_port.cross_realm_transform_writable = Some(cross_realm_transform_writable.clone()); + managed_port.cross_realm_transform = Some(CrossRealmTransform::Writable( + cross_realm_transform_writable.clone(), + )); } /// Custom routing logic, followed by the task steps of @@ -1380,8 +1381,7 @@ impl GlobalScope { can_gc: CanGc, ) { let cx = GlobalScope::get_cx(); - rooted!(in(*cx) let mut cross_realm_transform_readable = None); - rooted!(in(*cx) let mut cross_realm_transform_writable = None); + rooted!(in(*cx) let mut cross_realm_transform = None); let should_dispatch = if let MessagePortState::Managed(_id, message_ports) = &mut *self.message_port_state.borrow_mut() @@ -1399,10 +1399,7 @@ impl GlobalScope { let to_dispatch = port_impl.handle_incoming(task).map(|to_dispatch| { (DomRoot::from_ref(&*managed_port.dom_port), to_dispatch) }); - cross_realm_transform_readable - .set(managed_port.cross_realm_transform_readable.clone()); - cross_realm_transform_writable - .set(managed_port.cross_realm_transform_writable.clone()); + cross_realm_transform.set(managed_port.cross_realm_transform.clone()); to_dispatch } else { panic!("managed-port has no port-impl."); @@ -1429,10 +1426,6 @@ impl GlobalScope { // Re-ordered because we need to pass it to `structuredclone::read`. rooted!(in(*cx) let mut message_clone = UndefinedValue()); - // Note: if this port is used to transfer a stream, we handle the events in Rust. - let has_cross_realm_tansform = cross_realm_transform_readable.is_some() || - cross_realm_transform_writable.is_some(); - let realm = enter_realm(self); let comp = InRealm::Entered(&realm); @@ -1447,26 +1440,28 @@ impl GlobalScope { // if any, maintaining their relative order. // Note: both done in `structuredclone::read`. if let Ok(ports) = structuredclone::read(self, data, message_clone.handle_mut()) { - // Add a handler for port’s message event with the following steps: - // from - if let Some(transform) = cross_realm_transform_readable.as_ref() { - transform.handle_message( - cx, - self, - &dom_port, - message_clone.handle(), - comp, - can_gc, - ); - } - - // Add a handler for port’s message event with the following steps: - // from - if let Some(transform) = cross_realm_transform_writable.as_ref() { - transform.handle_message(cx, self, message_clone.handle(), comp, can_gc); - } - - if !has_cross_realm_tansform { + // Note: if this port is used to transfer a stream, we handle the events in Rust. + if let Some(transform) = cross_realm_transform.as_ref() { + match transform { + // Add a handler for port’s message event with the following steps: + // from + CrossRealmTransform::Readable(readable) => { + readable.handle_message( + cx, + self, + &dom_port, + message_clone.handle(), + comp, + can_gc, + ); + }, + // Add a handler for port’s message event with the following steps: + // from + CrossRealmTransform::Writable(writable) => { + writable.handle_message(cx, self, message_clone.handle(), comp, can_gc); + }, + } + } else { // Fire an event named message at messageEventTarget, // using MessageEvent, // with the data attribute initialized to messageClone @@ -1481,25 +1476,24 @@ impl GlobalScope { can_gc, ); } + } else if let Some(transform) = cross_realm_transform.as_ref() { + match transform { + // Add a handler for port’s messageerror event with the following steps: + // from + CrossRealmTransform::Readable(readable) => { + readable.handle_error(cx, self, &dom_port, comp, can_gc); + }, + // Add a handler for port’s messageerror event with the following steps: + // from + CrossRealmTransform::Writable(writable) => { + writable.handle_error(cx, self, &dom_port, comp, can_gc); + }, + } } else { - // Add a handler for port’s messageerror event with the following steps: - // from - if let Some(transform) = cross_realm_transform_readable.as_ref() { - transform.handle_error(cx, self, &dom_port, comp, can_gc); - } - - // Add a handler for port’s messageerror event with the following steps: - // from - if let Some(transform) = cross_realm_transform_writable.as_ref() { - transform.handle_error(cx, self, &dom_port, comp, can_gc); - } - - if !has_cross_realm_tansform { - // If this throws an exception, catch it, - // fire an event named messageerror at messageEventTarget, - // using MessageEvent, and then return. - MessageEvent::dispatch_error(message_event_target, self, can_gc); - } + // If this throws an exception, catch it, + // fire an event named messageerror at messageEventTarget, + // using MessageEvent, and then return. + MessageEvent::dispatch_error(message_event_target, self, can_gc); } } } @@ -1689,8 +1683,7 @@ impl GlobalScope { dom_port: Dom::from_ref(dom_port), pending: true, explicitly_closed: false, - cross_realm_transform_readable: None, - cross_realm_transform_writable: None, + cross_realm_transform: None, }, ); @@ -1713,8 +1706,7 @@ impl GlobalScope { dom_port: Dom::from_ref(dom_port), pending: false, explicitly_closed: false, - cross_realm_transform_readable: None, - cross_realm_transform_writable: None, + cross_realm_transform: None, }, ); let _ = self.script_to_constellation_chan().send( diff --git a/components/script/dom/transformstream.rs b/components/script/dom/transformstream.rs index 446bf71f172..30058eca6a8 100644 --- a/components/script/dom/transformstream.rs +++ b/components/script/dom/transformstream.rs @@ -21,7 +21,9 @@ use super::bindings::structuredclone::StructuredData; use super::bindings::transferable::Transferable; use super::messageport::MessagePort; use super::promisenativehandler::Callback; +use super::readablestream::CrossRealmTransformReadable; use super::types::{TransformStreamDefaultController, WritableStream}; +use super::writablestream::CrossRealmTransformWritable; use crate::dom::bindings::cell::DomRefCell; use crate::dom::bindings::codegen::Bindings::QueuingStrategyBinding::QueuingStrategy; use crate::dom::bindings::codegen::Bindings::TransformStreamBinding::TransformStreamMethods; @@ -368,6 +370,19 @@ impl Callback for FlushPromiseRejection { } } +impl js::gc::Rootable for CrossRealmTransform {} + +/// A wrapper to handle `message` and `messageerror` events +/// for the message port used by the transfered stream. +#[derive(Clone, JSTraceable, MallocSizeOf)] +#[cfg_attr(crown, crown::unrooted_must_root_lint::must_root)] +pub(crate) enum CrossRealmTransform { + /// + Readable(CrossRealmTransformReadable), + /// + Writable(CrossRealmTransformWritable), +} + /// #[dom_struct] pub struct TransformStream {