From af5d665efaa35317c89ae3f2dd5ac6ea168a6985 Mon Sep 17 00:00:00 2001 From: Gregory Terzian <2792687+gterzian@users.noreply.github.com> Date: Wed, 30 Apr 2025 12:49:38 +0200 Subject: [PATCH] MessagePort: implement disentanglement (#36654) Implement [disentangle](https://html.spec.whatwg.org/multipage/#disentangle) Remove bespoke gc logic which now becomes unnecessary. Adds a wpt test that hits the "disentangle while in transfer" logic. Updates streams code, fixing an error where disentanglement is conditional on an error. Test coverage: there are existing tests in `/webmessaging/message-channels/close-event/explicitly-closed.tentative.window.js` for the no transfer case, and the simple completed transfer case, and this PR adds a test for the more complicated transfer in progress case. Fix https://github.com/servo/servo/issues/36465 --------- Signed-off-by: gterzian <2792687+gterzian@users.noreply.github.com> --- components/constellation/constellation.rs | 171 +++++++-------- components/constellation/tracing.rs | 2 +- components/script/dom/globalscope.rs | 199 ++++++++++++++---- components/script/dom/messageport.rs | 38 +++- components/script/dom/readablestream.rs | 8 +- .../script/dom/underlyingsourcecontainer.rs | 2 +- components/script/dom/writablestream.rs | 4 +- .../dom/writablestreamdefaultcontroller.rs | 10 +- .../script_bindings/codegen/Bindings.conf | 2 +- .../webidls/MessagePort.webidl | 1 + .../constellation/from_script_message.rs | 16 +- components/shared/constellation/lib.rs | 19 +- .../structured_data/transferable.rs | 7 +- tests/wpt/meta/MANIFEST.json | 4 +- tests/wpt/meta/html/dom/idlharness.any.js.ini | 3 - .../meta/html/dom/idlharness.https.html.ini | 6 - .../explicitly-closed.tentative.window.js.ini | 7 - .../explicitly-closed.tentative.window.js | 10 + .../close-event/resources/helper.js | 38 ++++ 19 files changed, 356 insertions(+), 191 deletions(-) delete mode 100644 tests/wpt/meta/webmessaging/message-channels/close-event/explicitly-closed.tentative.window.js.ini diff --git a/components/constellation/constellation.rs b/components/constellation/constellation.rs index 2f9345c416f..ad89c435717 100644 --- a/components/constellation/constellation.rs +++ b/components/constellation/constellation.rs @@ -115,9 +115,10 @@ use constellation_traits::{ AuxiliaryWebViewCreationRequest, AuxiliaryWebViewCreationResponse, BroadcastMsg, DocumentState, EmbedderToConstellationMessage, IFrameLoadInfo, IFrameLoadInfoWithData, IFrameSandboxState, IFrameSizeMsg, Job, LoadData, LoadOrigin, LogEntry, MessagePortMsg, NavigationHistoryBehavior, - PaintMetricEvent, PortMessageTask, SWManagerMsg, SWManagerSenders, ScriptToConstellationChan, - ScriptToConstellationMessage, ScrollState, ServiceWorkerManagerFactory, ServiceWorkerMsg, - StructuredSerializedData, TraversalDirection, WindowSizeType, + PaintMetricEvent, PortMessageTask, PortTransferInfo, SWManagerMsg, SWManagerSenders, + ScriptToConstellationChan, ScriptToConstellationMessage, ScrollState, + ServiceWorkerManagerFactory, ServiceWorkerMsg, StructuredSerializedData, TraversalDirection, + WindowSizeType, }; use crossbeam_channel::{Receiver, Select, Sender, unbounded}; use devtools_traits::{ @@ -202,9 +203,6 @@ enum TransferState { /// While a completion failed, another global requested to complete the transfer. /// We are still buffering messages, and awaiting the return of the buffer from the global who failed. CompletionRequested(MessagePortRouterId, VecDeque), - /// The entangled port has been removed while the port was in-transfer, - /// the current port should be removed as well once it is managed again. - EntangledRemoved, } #[derive(Debug)] @@ -1524,12 +1522,12 @@ where ScriptToConstellationMessage::NewMessagePort(router_id, port_id) => { self.handle_new_messageport(router_id, port_id); }, - ScriptToConstellationMessage::RemoveMessagePort(port_id) => { - self.handle_remove_messageport(port_id); - }, ScriptToConstellationMessage::EntanglePorts(port1, port2) => { self.handle_entangle_messageports(port1, port2); }, + ScriptToConstellationMessage::DisentanglePorts(port1, port2) => { + self.handle_disentangle_messageports(port1, port2); + }, ScriptToConstellationMessage::NewBroadcastChannelRouter( router_id, response_sender, @@ -2117,17 +2115,6 @@ where Entry::Occupied(entry) => entry, }; match entry.get().state { - TransferState::EntangledRemoved => { - // If the entangled port has been removed while this one was in-transfer, - // remove it now. - if let Some(ipc_sender) = self.message_port_routers.get(&router_id) { - let _ = ipc_sender.send(MessagePortMsg::RemoveMessagePort(port_id)); - } else { - warn!("No message-port sender for {:?}", router_id); - } - entry.remove_entry(); - continue; - }, TransferState::CompletionInProgress(expected_router_id) => { // Here, the transfer was normally completed. @@ -2151,9 +2138,9 @@ where fn handle_message_port_transfer_failed( &mut self, - ports: HashMap>, + ports: HashMap, ) { - for (port_id, mut previous_buffer) in ports.into_iter() { + for (port_id, mut transfer_info) in ports.into_iter() { let entry = match self.message_ports.remove(&port_id) { None => { warn!( @@ -2165,11 +2152,6 @@ where Some(entry) => entry, }; let new_info = match entry.state { - TransferState::EntangledRemoved => { - // If the entangled port has been removed while this one was in-transfer, - // just drop it. - continue; - }, TransferState::CompletionFailed(mut current_buffer) => { // The transfer failed, // and now the global has returned us the buffer we previously sent. @@ -2177,7 +2159,7 @@ where // Tasks in the previous buffer are older, // hence need to be added to the front of the current one. - while let Some(task) = previous_buffer.pop_back() { + while let Some(task) = transfer_info.port_message_queue.pop_back() { current_buffer.push_front(task); } // Update the state to transfer-in-progress. @@ -2196,7 +2178,7 @@ where // Tasks in the previous buffer are older, // hence need to be added to the front of the current one. - while let Some(task) = previous_buffer.pop_back() { + while let Some(task) = transfer_info.port_message_queue.pop_back() { current_buffer.push_front(task); } // Forward the buffered message-queue to complete the current transfer. @@ -2204,7 +2186,10 @@ where if ipc_sender .send(MessagePortMsg::CompletePendingTransfer( port_id, - current_buffer, + PortTransferInfo { + port_message_queue: current_buffer, + disentangled: entry.entangled_with.is_none(), + }, )) .is_err() { @@ -2251,18 +2236,14 @@ where Some(entry) => entry, }; let new_info = match entry.state { - TransferState::EntangledRemoved => { - // If the entangled port has been removed while this one was in-transfer, - // remove it now. - if let Some(ipc_sender) = self.message_port_routers.get(&router_id) { - let _ = ipc_sender.send(MessagePortMsg::RemoveMessagePort(port_id)); - } else { - warn!("No message-port sender for {:?}", router_id); - } - continue; - }, TransferState::TransferInProgress(buffer) => { - response.insert(port_id, buffer); + response.insert( + port_id, + PortTransferInfo { + port_message_queue: buffer, + disentangled: entry.entangled_with.is_none(), + }, + ); // If the port was in transfer, and a global is requesting completion, // we note the start of the completion. @@ -2341,10 +2322,6 @@ where TransferState::TransferInProgress(queue) => queue.push_back(task), TransferState::CompletionFailed(queue) => queue.push_back(task), TransferState::CompletionRequested(_, queue) => queue.push_back(task), - TransferState::EntangledRemoved => warn!( - "Messageport received a message, but entangled has alread been removed {:?}", - port_id - ), } } @@ -2406,59 +2383,6 @@ where } } - #[cfg_attr( - feature = "tracing", - tracing::instrument(skip_all, fields(servo_profiling = true), level = "trace") - )] - fn handle_remove_messageport(&mut self, port_id: MessagePortId) { - let entangled = match self.message_ports.remove(&port_id) { - Some(info) => info.entangled_with, - None => { - return warn!( - "Constellation asked to remove unknown messageport {:?}", - port_id - ); - }, - }; - let entangled_id = match entangled { - Some(id) => id, - None => return, - }; - let info = match self.message_ports.get_mut(&entangled_id) { - Some(info) => info, - None => { - return warn!( - "Constellation asked to remove unknown entangled messageport {:?}", - entangled_id - ); - }, - }; - let router_id = match info.state { - TransferState::EntangledRemoved => { - return warn!( - "Constellation asked to remove entangled messageport by a port that was already removed {:?}", - port_id - ); - }, - TransferState::TransferInProgress(_) | - TransferState::CompletionInProgress(_) | - TransferState::CompletionFailed(_) | - TransferState::CompletionRequested(_, _) => { - // Note: since the port is in-transer, we don't have a router to send it a message - // to let it know that its entangled port has been removed. - // Hence we mark it so that it will be messaged and removed once the transfer completes. - info.state = TransferState::EntangledRemoved; - return; - }, - TransferState::Managed(router_id) => router_id, - }; - if let Some(sender) = self.message_port_routers.get(&router_id) { - let _ = sender.send(MessagePortMsg::RemoveMessagePort(entangled_id)); - } else { - warn!("No message-port sender for {:?}", router_id); - } - } - #[cfg_attr( feature = "tracing", tracing::instrument(skip_all, fields(servo_profiling = true), level = "trace") @@ -2482,6 +2406,57 @@ where } } + #[cfg_attr( + feature = "tracing", + tracing::instrument(skip_all, fields(servo_profiling = true), level = "trace") + )] + /// + fn handle_disentangle_messageports( + &mut self, + port1: MessagePortId, + port2: Option, + ) { + // Disentangle initiatorPort and otherPort, + // so that they are no longer entangled or associated with each other. + // Note: If `port2` is some, then this is the first message + // and `port1` is the initiatorPort, `port2` is the otherPort. + // We can immediately remove the initiator. + let _ = self.message_ports.remove(&port1); + + // Note: the none case is when otherPort sent this message + // in response to completing its own local disentanglement. + let Some(port2) = port2 else { + return; + }; + + // Start disentanglement of the other port. + if let Some(info) = self.message_ports.get_mut(&port2) { + info.entangled_with = None; + match &mut info.state { + TransferState::Managed(router_id) | + TransferState::CompletionInProgress(router_id) => { + // We try to disentangle the other port now, + // and if it has been transfered out by the time the message is received, + // it will be ignored, + // and disentanglement will be completed as part of the transfer. + if let Some(ipc_sender) = self.message_port_routers.get(router_id) { + let _ = ipc_sender.send(MessagePortMsg::CompleteDisentanglement(port2)); + } else { + warn!("No message-port sender for {:?}", router_id); + } + }, + _ => { + // Note: the port is in transfer, disentanglement will complete along with it. + }, + } + } else { + warn!( + "Constellation asked to disentangle unknown messageport: {:?}", + port2 + ); + } + } + /// /// and /// diff --git a/components/constellation/tracing.rs b/components/constellation/tracing.rs index 5c9a09e1f13..eff7f755c6b 100644 --- a/components/constellation/tracing.rs +++ b/components/constellation/tracing.rs @@ -123,8 +123,8 @@ mod from_script { Self::RemoveMessagePortRouter(..) => target!("RemoveMessagePortRouter"), Self::RerouteMessagePort(..) => target!("RerouteMessagePort"), Self::MessagePortShipped(..) => target!("MessagePortShipped"), - Self::RemoveMessagePort(..) => target!("RemoveMessagePort"), Self::EntanglePorts(..) => target!("EntanglePorts"), + Self::DisentanglePorts(..) => target!("DisentanglePorts"), Self::NewBroadcastChannelRouter(..) => target!("NewBroadcastChannelRouter"), Self::RemoveBroadcastChannelRouter(..) => target!("RemoveBroadcastChannelRouter"), Self::NewBroadcastChannelNameInRouter(..) => { diff --git a/components/script/dom/globalscope.rs b/components/script/dom/globalscope.rs index b3345b90fc0..efa9a9a97ab 100644 --- a/components/script/dom/globalscope.rs +++ b/components/script/dom/globalscope.rs @@ -457,8 +457,9 @@ pub(crate) struct ManagedMessagePort { /// and only add them, and ask the constellation to complete the transfer, /// in a subsequent task if the port hasn't been re-transfered. pending: bool, - /// Has the port been closed? If closed, it can be dropped and later GC'ed. - closed: bool, + /// 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. @@ -546,12 +547,17 @@ impl MessageListener { let mut succeeded = vec![]; let mut failed = HashMap::new(); - for (id, buffer) in ports.into_iter() { + for (id, info) in ports.into_iter() { if global.is_managing_port(&id) { succeeded.push(id); - global.complete_port_transfer(id, buffer); + global.complete_port_transfer( + id, + info.port_message_queue, + info.disentangled, + CanGc::note() + ); } else { - failed.insert(id, buffer); + failed.insert(id, info); } } let _ = global.script_to_constellation_chan().send( @@ -560,13 +566,21 @@ impl MessageListener { }) ); }, - MessagePortMsg::CompletePendingTransfer(port_id, buffer) => { + MessagePortMsg::CompletePendingTransfer(port_id, info) => { let context = self.context.clone(); self.task_source.queue(task!(complete_pending: move || { let global = context.root(); - global.complete_port_transfer(port_id, buffer); + global.complete_port_transfer(port_id, info.port_message_queue, info.disentangled, CanGc::note()); })); }, + MessagePortMsg::CompleteDisentanglement(port_id) => { + let context = self.context.clone(); + self.task_source + .queue(task!(try_complete_disentanglement: move || { + let global = context.root(); + global.try_complete_disentanglement(port_id, CanGc::note()); + })); + }, MessagePortMsg::NewTask(port_id, task) => { let context = self.context.clone(); self.task_source.queue(task!(process_new_task: move || { @@ -574,14 +588,6 @@ impl MessageListener { global.route_task_to_port(port_id, task, CanGc::note()); })); }, - MessagePortMsg::RemoveMessagePort(port_id) => { - let context = self.context.clone(); - self.task_source - .queue(task!(process_remove_message_port: move || { - let global = context.root(); - global.note_entangled_port_removed(&port_id); - })); - }, } } } @@ -871,7 +877,13 @@ impl GlobalScope { } /// Complete the transfer of a message-port. - fn complete_port_transfer(&self, port_id: MessagePortId, tasks: VecDeque) { + fn complete_port_transfer( + &self, + port_id: MessagePortId, + tasks: VecDeque, + disentangled: bool, + can_gc: CanGc, + ) { let should_start = if let MessagePortState::Managed(_id, message_ports) = &mut *self.message_port_state.borrow_mut() { @@ -885,6 +897,10 @@ impl GlobalScope { } if let Some(port_impl) = managed_port.port_impl.as_mut() { port_impl.complete_transfer(tasks); + if disentangled { + port_impl.disentangle(); + managed_port.dom_port.disentangle(); + } port_impl.enabled() } else { panic!("managed-port has no port-impl."); @@ -895,7 +911,45 @@ impl GlobalScope { panic!("complete_port_transfer called for an unknown port."); }; if should_start { - self.start_message_port(&port_id); + self.start_message_port(&port_id, can_gc); + } + } + + /// The closing of `otherPort`, if it is in a different global. + /// + fn try_complete_disentanglement(&self, port_id: MessagePortId, can_gc: CanGc) { + let dom_port = if let MessagePortState::Managed(_id, message_ports) = + &mut *self.message_port_state.borrow_mut() + { + let dom_port = if let Some(managed_port) = message_ports.get_mut(&port_id) { + if managed_port.pending { + unreachable!("CompleteDisentanglement msg received for a pending port."); + } + let port_impl = managed_port + .port_impl + .as_mut() + .expect("managed-port has no port-impl."); + port_impl.disentangle(); + managed_port.dom_port.as_rooted() + } else { + // Note: this, and the other return below, + // can happen if the port has already been transferred out of this global, + // in which case the disentanglement will complete along with the transfer. + return; + }; + dom_port + } else { + return; + }; + + // Fire an event named close at otherPort. + dom_port.upcast().fire_event(atom!("close"), can_gc); + + let res = self.script_to_constellation_chan().send( + ScriptToConstellationMessage::DisentanglePorts(port_id, None), + ); + if res.is_err() { + warn!("Sending DisentanglePorts failed"); } } @@ -951,8 +1005,64 @@ impl GlobalScope { } /// - pub(crate) fn disentangle_port(&self, _port: &MessagePort) { - // TODO: #36465 + pub(crate) fn disentangle_port(&self, port: &MessagePort, can_gc: CanGc) { + let initiator_port = port.message_port_id(); + // Let otherPort be the MessagePort which initiatorPort was entangled with. + let Some(other_port) = port.disentangle() else { + // Assert: otherPort exists. + // Note: ignoring the assert, + // because the streams spec seems to disentangle ports that are disentangled already. + return; + }; + + // Disentangle initiatorPort and otherPort, so that they are no longer entangled or associated with each other. + // Note: this is done in part here, and in part at the constellation(if otherPort is in another global). + let dom_port = if let MessagePortState::Managed(_id, message_ports) = + &mut *self.message_port_state.borrow_mut() + { + let mut dom_port = None; + for port_id in &[initiator_port, &other_port] { + match message_ports.get_mut(port_id) { + None => { + continue; + }, + Some(managed_port) => { + let port_impl = managed_port + .port_impl + .as_mut() + .expect("managed-port has no port-impl."); + managed_port.dom_port.disentangle(); + port_impl.disentangle(); + + if **port_id == other_port { + dom_port = Some(managed_port.dom_port.as_rooted()) + } + }, + } + } + dom_port + } else { + panic!("disentangle_port called on a global not managing any ports."); + }; + + // Fire an event named close at `otherPort`. + // Note: done here if the port is managed by the same global as `initialPort`. + if let Some(dom_port) = dom_port { + dom_port.upcast().fire_event(atom!("close"), can_gc); + } + + let chan = self.script_to_constellation_chan().clone(); + let initiator_port = *initiator_port; + self.task_manager() + .port_message_queue() + .queue(task!(post_message: move || { + // Note: we do this in a task to ensure it doesn't affect messages that are still to be routed, + // see the task queueing in `post_messageport_msg`. + let res = chan.send(ScriptToConstellationMessage::DisentanglePorts(initiator_port, Some(other_port))); + if res.is_err() { + warn!("Sending DisentanglePorts failed"); + } + })); } /// @@ -984,18 +1094,6 @@ impl GlobalScope { .send(ScriptToConstellationMessage::EntanglePorts(port1, port2)); } - /// Note that the entangled port of `port_id` has been removed in another global. - pub(crate) fn note_entangled_port_removed(&self, port_id: &MessagePortId) { - // Note: currently this is a no-op, - // as we only use the `close` method to manage the local lifecyle of a port. - // This could be used as part of lifecyle management to determine a port can be GC'ed. - // See https://github.com/servo/servo/issues/25772 - warn!( - "Entangled port of {:?} has been removed in another global", - port_id - ); - } - /// Handle the transfer of a port in the current task. pub(crate) fn mark_port_as_transferred(&self, port_id: &MessagePortId) -> MessagePortImpl { if let MessagePortState::Managed(_id, message_ports) = @@ -1021,20 +1119,21 @@ impl GlobalScope { } /// - pub(crate) fn start_message_port(&self, port_id: &MessagePortId) { - let message_buffer = if let MessagePortState::Managed(_id, message_ports) = + pub(crate) fn start_message_port(&self, port_id: &MessagePortId, can_gc: CanGc) { + let (message_buffer, dom_port) = if let MessagePortState::Managed(_id, message_ports) = &mut *self.message_port_state.borrow_mut() { - match message_ports.get_mut(port_id) { + let (message_buffer, dom_port) = match message_ports.get_mut(port_id) { None => panic!("start_message_port called on a unknown port."), Some(managed_port) => { if let Some(port_impl) = managed_port.port_impl.as_mut() { - port_impl.start() + (port_impl.start(), managed_port.dom_port.as_rooted()) } else { panic!("managed-port has no port-impl."); } }, - } + }; + (message_buffer, dom_port) } else { return warn!("start_message_port called on a global not managing any ports."); }; @@ -1042,6 +1141,18 @@ impl GlobalScope { for task in message_buffer { self.route_task_to_port(*port_id, task, CanGc::note()); } + if dom_port.disentangled() { + // + // Fire an event named close at otherPort. + dom_port.upcast().fire_event(atom!("close"), can_gc); + + let res = self.script_to_constellation_chan().send( + ScriptToConstellationMessage::DisentanglePorts(*port_id, None), + ); + if res.is_err() { + warn!("Sending DisentanglePorts failed"); + } + } } } @@ -1055,7 +1166,7 @@ impl GlobalScope { Some(managed_port) => { if let Some(port_impl) = managed_port.port_impl.as_mut() { port_impl.close(); - managed_port.closed = true; + managed_port.explicitly_closed = true; } else { panic!("managed-port has no port-impl."); } @@ -1436,12 +1547,7 @@ impl GlobalScope { let to_be_removed: Vec = message_ports .iter() .filter_map(|(id, managed_port)| { - if managed_port.closed { - // Let the constellation know to drop this port and the one it is entangled with, - // and to forward this message to the script-process where the entangled is found. - let _ = self - .script_to_constellation_chan() - .send(ScriptToConstellationMessage::RemoveMessagePort(*id)); + if managed_port.explicitly_closed { Some(*id) } else { None @@ -1451,6 +1557,9 @@ impl GlobalScope { for id in to_be_removed { message_ports.remove(&id); } + // Note: ports are only removed throught explicit closure by script in this global. + // TODO: #25772 + // TODO: remove ports when we can be sure their port message queue is empty(via the constellation). message_ports.is_empty() } else { false @@ -1581,7 +1690,7 @@ impl GlobalScope { port_impl: Some(port_impl), dom_port: Dom::from_ref(dom_port), pending: true, - closed: false, + explicitly_closed: false, cross_realm_transform_readable: None, cross_realm_transform_writable: None, }, @@ -1605,7 +1714,7 @@ impl GlobalScope { port_impl: Some(port_impl), dom_port: Dom::from_ref(dom_port), pending: false, - closed: false, + explicitly_closed: false, cross_realm_transform_readable: None, cross_realm_transform_writable: None, }, diff --git a/components/script/dom/messageport.rs b/components/script/dom/messageport.rs index 85d94c1aa7a..d70d3139b96 100644 --- a/components/script/dom/messageport.rs +++ b/components/script/dom/messageport.rs @@ -83,6 +83,20 @@ impl MessagePort { *self.entangled_port.borrow_mut() = Some(other_id); } + /// + pub(crate) fn disentangle(&self) -> Option { + // Disentangle initiatorPort and otherPort, so that they are no longer entangled or associated with each other. + // Note: called from `disentangle_port` in the global, where the rest happens. + self.entangled_port.borrow_mut().take() + } + + /// Has the port been disentangled? + /// Used when starting the port to fire the `close` event, + /// to cover the case of a disentanglement while in transfer. + pub(crate) fn disentangled(&self) -> bool { + self.entangled_port.borrow().is_none() + } + pub(crate) fn message_port_id(&self) -> &MessagePortId { &self.message_port_id } @@ -314,20 +328,28 @@ impl MessagePortMethods for MessagePort { } /// - fn Start(&self) { + fn Start(&self, can_gc: CanGc) { if self.detached.get() { return; } - self.global().start_message_port(self.message_port_id()); + self.global() + .start_message_port(self.message_port_id(), can_gc); } /// - fn Close(&self) { + fn Close(&self, can_gc: CanGc) { if self.detached.get() { return; } + + // Set this's [[Detached]] internal slot value to true. self.detached.set(true); - self.global().close_message_port(self.message_port_id()); + + let global = self.global(); + global.close_message_port(self.message_port_id()); + + // If this is entangled, disentangle it. + global.disentangle_port(self, can_gc); } /// @@ -340,15 +362,19 @@ impl MessagePortMethods for MessagePort { } /// - fn SetOnmessage(&self, listener: Option>) { + fn SetOnmessage(&self, listener: Option>, can_gc: CanGc) { if self.detached.get() { return; } self.set_onmessage(listener); // Note: we cannot use the event_handler macro, due to the need to start the port. - self.global().start_message_port(self.message_port_id()); + self.global() + .start_message_port(self.message_port_id(), can_gc); } // event_handler!(messageerror, GetOnmessageerror, SetOnmessageerror); + + // + event_handler!(close, GetOnclose, SetOnclose); } diff --git a/components/script/dom/readablestream.rs b/components/script/dom/readablestream.rs index 51393ab33ae..4982bfa32e3 100644 --- a/components/script/dom/readablestream.rs +++ b/components/script/dom/readablestream.rs @@ -1825,7 +1825,7 @@ impl ReadableStream { global.note_cross_realm_transform_readable(&cross_realm_transform_readable, port_id); // Enable port’s port message queue. - port.Start(); + port.Start(can_gc); // Perform ! SetUpReadableStreamDefaultController controller @@ -2093,7 +2093,7 @@ impl CrossRealmTransformReadable { self.controller.close(can_gc); // Disentangle port. - global.disentangle_port(port); + global.disentangle_port(port, can_gc); } // Otherwise, if type is "error", @@ -2102,7 +2102,7 @@ impl CrossRealmTransformReadable { self.controller.error(value.handle(), can_gc); // Disentangle port. - global.disentangle_port(port); + global.disentangle_port(port, can_gc); } } @@ -2129,7 +2129,7 @@ impl CrossRealmTransformReadable { self.controller.error(rooted_error.handle(), can_gc); // Disentangle port. - global.disentangle_port(port); + global.disentangle_port(port, can_gc); } } diff --git a/components/script/dom/underlyingsourcecontainer.rs b/components/script/dom/underlyingsourcecontainer.rs index 541a831693a..4acb58bafef 100644 --- a/components/script/dom/underlyingsourcecontainer.rs +++ b/components/script/dom/underlyingsourcecontainer.rs @@ -151,7 +151,7 @@ impl UnderlyingSourceContainer { let result = port.pack_and_post_message_handling_error("error", reason, can_gc); // Disentangle port. - self.global().disentangle_port(port); + self.global().disentangle_port(port, can_gc); let promise = Promise::new(&self.global(), can_gc); diff --git a/components/script/dom/writablestream.rs b/components/script/dom/writablestream.rs index 8c2b2434cd2..1b029f592de 100644 --- a/components/script/dom/writablestream.rs +++ b/components/script/dom/writablestream.rs @@ -893,7 +893,7 @@ impl WritableStream { global.note_cross_realm_transform_writable(&cross_realm_transform_writable, port_id); // Enable port’s port message queue. - port.Start(); + port.Start(can_gc); // Perform ! SetUpWritableStreamDefaultController controller @@ -1202,7 +1202,7 @@ impl CrossRealmTransformWritable { .error_if_needed(cx, rooted_error.handle(), global, can_gc); // Disentangle port. - global.disentangle_port(port); + global.disentangle_port(port, can_gc); } } diff --git a/components/script/dom/writablestreamdefaultcontroller.rs b/components/script/dom/writablestreamdefaultcontroller.rs index 301404ffdb2..084165a6892 100644 --- a/components/script/dom/writablestreamdefaultcontroller.rs +++ b/components/script/dom/writablestreamdefaultcontroller.rs @@ -173,11 +173,11 @@ impl Callback for TransferBackPressurePromiseReaction { self.port .pack_and_post_message_handling_error("chunk", chunk.handle(), can_gc); - // Disentangle port. - global.disentangle_port(&self.port); - // If result is an abrupt completion, if let Err(error) = result { + // Disentangle port. + global.disentangle_port(&self.port, can_gc); + // Return a promise rejected with result.[[Value]]. self.result_promise.reject_error(error, can_gc); } else { @@ -609,7 +609,7 @@ impl WritableStreamDefaultController { let result = port.pack_and_post_message_handling_error("error", reason, can_gc); // Disentangle port. - global.disentangle_port(port); + global.disentangle_port(port, can_gc); let promise = Promise::new(global, can_gc); @@ -752,7 +752,7 @@ impl WritableStreamDefaultController { .expect("Sending close should not fail."); // Disentangle port. - global.disentangle_port(port); + global.disentangle_port(port, can_gc); // Return a promise resolved with undefined. Promise::new_resolved(global, cx, (), can_gc) diff --git a/components/script_bindings/codegen/Bindings.conf b/components/script_bindings/codegen/Bindings.conf index c50bc31a7f5..4ab0b21cabe 100644 --- a/components/script_bindings/codegen/Bindings.conf +++ b/components/script_bindings/codegen/Bindings.conf @@ -474,7 +474,7 @@ DOMInterfaces = { 'MessagePort': { 'weakReferenceable': True, - 'canGc': ['GetOnmessage'], + 'canGc': ['Close', 'GetOnmessage', 'SetOnmessage', 'Start'], }, 'MessageEvent': { diff --git a/components/script_bindings/webidls/MessagePort.webidl b/components/script_bindings/webidls/MessagePort.webidl index 6fc1f432b38..b7082fc7fc3 100644 --- a/components/script_bindings/webidls/MessagePort.webidl +++ b/components/script_bindings/webidls/MessagePort.webidl @@ -16,6 +16,7 @@ interface MessagePort : EventTarget { // event handlers attribute EventHandler onmessage; attribute EventHandler onmessageerror; + attribute EventHandler onclose; }; dictionary StructuredSerializeOptions { diff --git a/components/shared/constellation/from_script_message.rs b/components/shared/constellation/from_script_message.rs index 625d642033e..ddc9f788617 100644 --- a/components/shared/constellation/from_script_message.rs +++ b/components/shared/constellation/from_script_message.rs @@ -4,7 +4,7 @@ //! Messages send from the ScriptThread to the Constellation. -use std::collections::{HashMap, VecDeque}; +use std::collections::HashMap; use std::fmt; use base::Epoch; @@ -35,7 +35,9 @@ use webgpu_traits::{WebGPU, WebGPUAdapterResponse}; use webrender_api::ImageKey; use crate::structured_data::{BroadcastMsg, StructuredSerializedData}; -use crate::{LogEntry, MessagePortMsg, PortMessageTask, TraversalDirection, WindowSizeType}; +use crate::{ + LogEntry, MessagePortMsg, PortMessageTask, PortTransferInfo, TraversalDirection, WindowSizeType, +}; /// A Script to Constellation channel. #[derive(Clone, Debug, Deserialize, Serialize)] @@ -470,7 +472,7 @@ pub enum ScriptToConstellationMessage { /* The ids of ports transferred successfully */ Vec, /* The ids, and buffers, of ports whose transfer failed */ - HashMap>, + HashMap, ), /// A new message-port was created or transferred, with corresponding control-sender. NewMessagePort(MessagePortRouterId, MessagePortId), @@ -482,10 +484,14 @@ pub enum ScriptToConstellationMessage { RerouteMessagePort(MessagePortId, PortMessageTask), /// A message-port was shipped, let the entangled port know. MessagePortShipped(MessagePortId), - /// A message-port has been discarded by script. - RemoveMessagePort(MessagePortId), /// Entangle two message-ports. EntanglePorts(MessagePortId, MessagePortId), + /// Disentangle two message-ports. + /// The first is the initiator, the second the other port, + /// unless the message is sent to complete a disentanglement, + /// in which case the first one is the other port, + /// and the second is none. + DisentanglePorts(MessagePortId, Option), /// A global has started managing broadcast-channels. NewBroadcastChannelRouter( BroadcastChannelRouterId, diff --git a/components/shared/constellation/lib.rs b/components/shared/constellation/lib.rs index b3d4fe525a1..559bc2dd2d1 100644 --- a/components/shared/constellation/lib.rs +++ b/components/shared/constellation/lib.rs @@ -157,18 +157,29 @@ pub struct PortMessageTask { pub data: StructuredSerializedData, } +/// The information needed by a global to process the transfer of a port. +#[derive(Debug, Deserialize, MallocSizeOf, Serialize)] +pub struct PortTransferInfo { + /// + pub port_message_queue: VecDeque, + /// A boolean indicating whether the port has been disentangled while in transfer, + /// if so, the disentanglement should be completed along with the transfer. + /// + pub disentangled: bool, +} + /// Messages for communication between the constellation and a global managing ports. #[derive(Debug, Deserialize, Serialize)] #[allow(clippy::large_enum_variant)] pub enum MessagePortMsg { /// Complete the transfer for a batch of ports. - CompleteTransfer(HashMap>), + CompleteTransfer(HashMap), /// Complete the transfer of a single port, /// whose transfer was pending because it had been requested /// while a previous failed transfer was being rolled-back. - CompletePendingTransfer(MessagePortId, VecDeque), - /// Remove a port, the entangled one doesn't exists anymore. - RemoveMessagePort(MessagePortId), + CompletePendingTransfer(MessagePortId, PortTransferInfo), + /// + CompleteDisentanglement(MessagePortId), /// Handle a new port-message-task. NewTask(MessagePortId, PortMessageTask), } diff --git a/components/shared/constellation/structured_data/transferable.rs b/components/shared/constellation/structured_data/transferable.rs index cd6388f5f34..7e4fe0e6d2d 100644 --- a/components/shared/constellation/structured_data/transferable.rs +++ b/components/shared/constellation/structured_data/transferable.rs @@ -77,7 +77,12 @@ impl MessagePortImpl { self.entangled_port } - /// Entanged this port with another. + /// + pub fn disentangle(&mut self) -> Option { + self.entangled_port.take() + } + + /// pub fn entangle(&mut self, other_id: MessagePortId) { self.entangled_port = Some(other_id); } diff --git a/tests/wpt/meta/MANIFEST.json b/tests/wpt/meta/MANIFEST.json index 750266dc59c..72c930afccf 100644 --- a/tests/wpt/meta/MANIFEST.json +++ b/tests/wpt/meta/MANIFEST.json @@ -518802,7 +518802,7 @@ "close-event": { "resources": { "helper.js": [ - "cb9ea9fe981e95374b836255c752a42de788fc7b", + "48744ac1c5b530ef8d46c3d9a0378c698353a5bc", [] ] } @@ -848537,7 +848537,7 @@ ] ], "explicitly-closed.tentative.window.js": [ - "612003d58eaea908ad93294a7bbf777184356a28", + "12bfa0bd73e9278e39b825d4fa81437f943cbd02", [ "webmessaging/message-channels/close-event/explicitly-closed.tentative.window.html", { diff --git a/tests/wpt/meta/html/dom/idlharness.any.js.ini b/tests/wpt/meta/html/dom/idlharness.any.js.ini index f17466adc7f..ad5e57e0759 100644 --- a/tests/wpt/meta/html/dom/idlharness.any.js.ini +++ b/tests/wpt/meta/html/dom/idlharness.any.js.ini @@ -95,9 +95,6 @@ [History interface: existence and properties of interface object] expected: FAIL - [MessagePort interface: attribute onclose] - expected: FAIL - [WorkerGlobalScope interface: attribute onlanguagechange] expected: FAIL diff --git a/tests/wpt/meta/html/dom/idlharness.https.html.ini b/tests/wpt/meta/html/dom/idlharness.https.html.ini index 64533ac1838..ac7504347d7 100644 --- a/tests/wpt/meta/html/dom/idlharness.https.html.ini +++ b/tests/wpt/meta/html/dom/idlharness.https.html.ini @@ -1517,9 +1517,6 @@ [SVGSVGElement interface: attribute onpagereveal] expected: FAIL - [MessagePort interface: attribute onclose] - expected: FAIL - [NotRestoredReasonDetails interface: existence and properties of interface object] expected: FAIL @@ -5363,9 +5360,6 @@ [Navigator interface: window.navigator must inherit property "pdfViewerEnabled" with the proper type] expected: FAIL - [MessagePort interface: attribute onclose] - expected: FAIL - [SharedWorker interface: existence and properties of interface object] expected: FAIL diff --git a/tests/wpt/meta/webmessaging/message-channels/close-event/explicitly-closed.tentative.window.js.ini b/tests/wpt/meta/webmessaging/message-channels/close-event/explicitly-closed.tentative.window.js.ini deleted file mode 100644 index c625c16f713..00000000000 --- a/tests/wpt/meta/webmessaging/message-channels/close-event/explicitly-closed.tentative.window.js.ini +++ /dev/null @@ -1,7 +0,0 @@ -[explicitly-closed.tentative.window.html] - expected: TIMEOUT - [Close event on port2 is fired when port1 is explicitly closed] - expected: TIMEOUT - - [Close event on port2 is fired when port1, which is in a different window, is explicitly closed.] - expected: TIMEOUT diff --git a/tests/wpt/tests/webmessaging/message-channels/close-event/explicitly-closed.tentative.window.js b/tests/wpt/tests/webmessaging/message-channels/close-event/explicitly-closed.tentative.window.js index 612003d58ea..12bfa0bd73e 100644 --- a/tests/wpt/tests/webmessaging/message-channels/close-event/explicitly-closed.tentative.window.js +++ b/tests/wpt/tests/webmessaging/message-channels/close-event/explicitly-closed.tentative.window.js @@ -33,3 +33,13 @@ promise_test(async t => { }); await closeEventPromise; }, 'Close event on port2 is fired when port1, which is in a different window, is explicitly closed.') + +promise_test(async t => { + const rc = await addWindow(); + const waitForPort = expectMessagePortFromWindowWithoutStartingIt(window); + await createMessageChannelAndSendPortFollowedByClose(rc); + const port = await waitForPort; + const closeEventPromise = createCloseEventPromise(port); + port.start(); + await closeEventPromise; +}, 'Close event on port2 is fired when port1, in a different window, is closed during the transfer of port2.') diff --git a/tests/wpt/tests/webmessaging/message-channels/close-event/resources/helper.js b/tests/wpt/tests/webmessaging/message-channels/close-event/resources/helper.js index cb9ea9fe981..48744ac1c5b 100644 --- a/tests/wpt/tests/webmessaging/message-channels/close-event/resources/helper.js +++ b/tests/wpt/tests/webmessaging/message-channels/close-event/resources/helper.js @@ -21,6 +21,44 @@ function expectMessagePortFromWindow(window) { }); } +/** + * Create a new promise that resolves when the window receives + * the MessagePort and does not start it. + * + * @param {Window} window - The window to wait for the MessagePort. + * @returns {Promise} A promise you should await to ensure the + * window + * receives the MessagePort. + */ +function expectMessagePortFromWindowWithoutStartingIt(window) { + return new Promise(resolve => { + window.onmessage = e => { + try { + assert_true(e.ports[0] instanceof window.MessagePort); + resolve(e.ports[0]); + } catch (e) { + reject(e); + } + }; + }); +} + +/** + * Create a new MessageChannel and transfers one of the ports to + * the window which opened the window with a remote context provided + * as an argument, and immediately closes the entangled port. + * + * @param {RemoteContextWrapper} remoteContextWrapper + */ +async function createMessageChannelAndSendPortFollowedByClose(remoteContextWrapper) { + await remoteContextWrapper.executeScript(() => { + const {port1, port2} = new MessageChannel(); + port1.start(); + window.opener.postMessage({}, '*', [port2]); + port1.close(); + }); +} + /** * Create a new MessageChannel and transfers one of the ports to * the window which opened the window with a remote context provided