[#26488] Refactored RTCDataChannel for safer dropping and added Promise comment (#37332)

Fixes (partially) #26488 and apply the
https://github.com/servo/servo/pull/37324#discussion_r2133989190
comment.

Testing: No tests added
Fixes: Partially #26488

---------

Signed-off-by: Domenico Rizzo <domenico.rizzo@gmail.com>

components/script/dom/promise.rs

@@ -73,6 +73,9 @@ impl PromiseHelper for Rc<Promise> {
     }
 }
 
+// Promise objects are stored inside Rc values, so Drop is run when the last Rc is dropped,
+// rather than when SpiderMonkey runs a GC. This makes it safe to interact with the JS engine unlike
+// Drop implementations for other DOM types.
 impl Drop for Promise {
     #[allow(unsafe_code)]
     fn drop(&mut self) {

components/script/dom/rtcdatachannel.rs

@@ -12,6 +12,7 @@ use js::jsapi::{JSAutoRealm, JSObject};
 use js::jsval::UndefinedValue;
 use js::rust::CustomAutoRooterGuard;
 use js::typedarray::{ArrayBuffer, ArrayBufferView, CreateWith};
+use script_bindings::weakref::WeakRef;
 use servo_media::webrtc::{
     DataChannelId, DataChannelInit, DataChannelMessage, DataChannelState, WebRtcError,
 };
@@ -37,12 +38,37 @@ use crate::dom::rtcerrorevent::RTCErrorEvent;
 use crate::dom::rtcpeerconnection::RTCPeerConnection;
 use crate::script_runtime::CanGc;
 
+#[derive(JSTraceable, MallocSizeOf)]
+struct DroppableRTCDataChannel {
+    #[ignore_malloc_size_of = "defined in servo-media"]
+    servo_media_id: DataChannelId,
+    peer_connection: WeakRef<RTCPeerConnection>,
+}
+
+impl DroppableRTCDataChannel {
+    fn new(peer_connection: WeakRef<RTCPeerConnection>, servo_media_id: DataChannelId) -> Self {
+        DroppableRTCDataChannel {
+            servo_media_id,
+            peer_connection,
+        }
+    }
+
+    pub(crate) fn get_servo_media_id(&self) -> DataChannelId {
+        self.servo_media_id
+    }
+}
+
+impl Drop for DroppableRTCDataChannel {
+    fn drop(&mut self) {
+        if let Some(root) = self.peer_connection.root() {
+            root.unregister_data_channel(&self.get_servo_media_id());
+        }
+    }
+}
+
 #[dom_struct]
 pub(crate) struct RTCDataChannel {
     eventtarget: EventTarget,
-    #[ignore_malloc_size_of = "defined in servo-media"]
-    servo_media_id: DataChannelId,
-    peer_connection: Dom<RTCPeerConnection>,
     label: USVString,
     ordered: bool,
     max_packet_life_time: Option<u16>,
@@ -52,6 +78,8 @@ pub(crate) struct RTCDataChannel {
     id: Option<u16>,
     ready_state: Cell<RTCDataChannelState>,
     binary_type: DomRefCell<DOMString>,
+    peer_connection: Dom<RTCPeerConnection>,
+    droppable: DroppableRTCDataChannel,
 }
 
 impl RTCDataChannel {
@@ -76,8 +104,6 @@ impl RTCDataChannel {
 
         RTCDataChannel {
             eventtarget: EventTarget::new_inherited(),
-            servo_media_id,
-            peer_connection: Dom::from_ref(peer_connection),
             label,
             ordered: options.ordered,
             max_packet_life_time: options.maxPacketLifeTime,
@@ -87,6 +113,8 @@ impl RTCDataChannel {
             id: options.id,
             ready_state: Cell::new(RTCDataChannelState::Connecting),
             binary_type: DomRefCell::new(DOMString::from("blob")),
+            peer_connection: Dom::from_ref(peer_connection),
+            droppable: DroppableRTCDataChannel::new(WeakRef::new(peer_connection), servo_media_id),
         }
     }
 
@@ -109,11 +137,16 @@ impl RTCDataChannel {
             can_gc,
         );
 
-        peer_connection.register_data_channel(rtc_data_channel.servo_media_id, &rtc_data_channel);
+        peer_connection
+            .register_data_channel(rtc_data_channel.get_servo_media_id(), &rtc_data_channel);
 
         rtc_data_channel
     }
 
+    pub(crate) fn get_servo_media_id(&self) -> DataChannelId {
+        self.droppable.get_servo_media_id()
+    }
+
     pub(crate) fn on_open(&self, can_gc: CanGc) {
         let event = Event::new(
             &self.global(),
@@ -136,7 +169,7 @@ impl RTCDataChannel {
         event.upcast::<Event>().fire(self.upcast(), can_gc);
 
         self.peer_connection
-            .unregister_data_channel(&self.servo_media_id);
+            .unregister_data_channel(&self.get_servo_media_id());
     }
 
     pub(crate) fn on_error(&self, error: WebRtcError, can_gc: CanGc) {
@@ -242,19 +275,12 @@ impl RTCDataChannel {
         controller
             .as_ref()
             .unwrap()
-            .send_data_channel_message(&self.servo_media_id, message);
+            .send_data_channel_message(&self.get_servo_media_id(), message);
 
         Ok(())
     }
 }
 
-impl Drop for RTCDataChannel {
-    fn drop(&mut self) {
-        self.peer_connection
-            .unregister_data_channel(&self.servo_media_id);
-    }
-}
-
 enum SendSource<'a, 'b> {
     String(&'a USVString),
     Blob(&'a Blob),
@@ -332,7 +358,7 @@ impl RTCDataChannelMethods<crate::DomTypeHolder> for RTCDataChannel {
         controller
             .as_ref()
             .unwrap()
-            .close_data_channel(&self.servo_media_id);
+            .close_data_channel(&self.get_servo_media_id());
     }
 
     // https://www.w3.org/TR/webrtc/#dom-datachannel-binarytype

components/script/dom/rtcpeerconnection.rs

@@ -8,6 +8,7 @@ use std::rc::Rc;
 
 use dom_struct::dom_struct;
 use js::rust::HandleObject;
+use script_bindings::weakref::WeakReferenceable;
 use servo_media::ServoMedia;
 use servo_media::streams::MediaStreamType;
 use servo_media::streams::registry::MediaStreamId;
@@ -309,15 +310,16 @@ impl RTCPeerConnection {
                 event.upcast::<Event>().fire(self.upcast(), can_gc);
             },
             _ => {
-                let channel = if let Some(channel) = self.data_channels.borrow().get(&channel_id) {
+                let channel: DomRoot<RTCDataChannel> =
-                    DomRoot::from_ref(&**channel)
+                    if let Some(channel) = self.data_channels.borrow().get(&channel_id) {
-                } else {
+                        DomRoot::from_ref(&**channel)
-                    warn!(
+                    } else {
-                        "Got an event for an unregistered data channel {:?}",
+                        warn!(
-                        channel_id
+                            "Got an event for an unregistered data channel {:?}",
-                    );
+                            channel_id
-                    return;
+                        );
-                };
+                        return;
+                    };
 
                 match event {
                     DataChannelEvent::Open => channel.on_open(can_gc),
@@ -789,6 +791,8 @@ impl RTCPeerConnectionMethods<crate::DomTypeHolder> for RTCPeerConnection {
     }
 }
 
+impl WeakReferenceable for RTCPeerConnection {}
+
 impl Convert<RTCSessionDescriptionInit> for SessionDescription {
     fn convert(self) -> RTCSessionDescriptionInit {
         let type_ = match self.type_ {

components/script_bindings/codegen/Bindings.conf

@@ -559,6 +559,10 @@ DOMInterfaces = {
     'canGc': ['Error', 'Redirect', 'Clone', 'CreateFromJson', 'Text', 'Blob', 'FormData', 'Json', 'ArrayBuffer', 'Headers', 'Bytes'],
 },
 
+'RTCDataChannel': {
+    'weakReferenceable': True, 
+},
+
 'RTCPeerConnection': {
     'inRealms': ['AddIceCandidate', 'CreateAnswer', 'CreateOffer', 'SetLocalDescription', 'SetRemoteDescription'],
     'canGc': ['Close', 'AddIceCandidate', 'CreateAnswer', 'CreateOffer', 'SetLocalDescription', 'SetRemoteDescription'],