From ee990a9b8003860b51eee5eeb3e8cfdc735b8922 Mon Sep 17 00:00:00 2001 From: Johan Klokkhammer Helsing Date: Thu, 22 Dec 2022 17:06:47 +0100 Subject: [PATCH 1/5] wasm: warn when ice candidates are dropped --- .../src/webrtc_socket/wasm/message_loop.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/matchbox_socket/src/webrtc_socket/wasm/message_loop.rs b/matchbox_socket/src/webrtc_socket/wasm/message_loop.rs index e3071a70..683f1e53 100644 --- a/matchbox_socket/src/webrtc_socket/wasm/message_loop.rs +++ b/matchbox_socket/src/webrtc_socket/wasm/message_loop.rs @@ -245,7 +245,12 @@ async fn handshake_offer( // TODO: we should support getting new ICE candidates even after connecting, // since it's possible to return to the ice gathering state // See: - conn.set_onicecandidate(None); + let onicecandidate: Box = + Box::new(move |_event: RtcPeerConnectionIceEvent| { + warn!("received ice candidate event after handshake completed"); + }); + let onicecandidate = Closure::wrap(onicecandidate); + conn.set_onicecandidate(Some(onicecandidate.as_ref().unchecked_ref())); debug!("Ice completed: {:?}", conn.ice_gathering_state()); @@ -391,7 +396,12 @@ async fn handshake_accept( // TODO: we should support getting new ICE candidates even after connecting, // since it's possible to return to the ice gathering state // See: - conn.set_onicecandidate(None); + let onicecandidate: Box = + Box::new(move |_event: RtcPeerConnectionIceEvent| { + warn!("received ice candidate event after handshake completed"); + }); + let onicecandidate = Closure::wrap(onicecandidate); + conn.set_onicecandidate(Some(onicecandidate.as_ref().unchecked_ref())); debug!("Ice completed: {:?}", conn.ice_gathering_state()); From 310306774f066aab032fa08ca53d87fe25b8c9f5 Mon Sep 17 00:00:00 2001 From: Johan Klokkhammer Helsing Date: Thu, 22 Dec 2022 17:17:12 +0100 Subject: [PATCH 2/5] wasm: log when ice connection state changes --- matchbox_socket/Cargo.toml | 1 + .../src/webrtc_socket/wasm/message_loop.rs | 15 ++++++++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/matchbox_socket/Cargo.toml b/matchbox_socket/Cargo.toml index e53e735f..1f9751fd 100644 --- a/matchbox_socket/Cargo.toml +++ b/matchbox_socket/Cargo.toml @@ -40,6 +40,7 @@ web-sys = { version = "0.3.22", default-features = false, features = [ "RtcPeerConnection", "RtcSdpType", "RtcSessionDescription", "RtcSessionDescriptionInit", "RtcIceGatheringState", "RtcIceCandidate", "RtcIceCandidateInit", "RtcPeerConnectionIceEvent", + "RtcIceConnectionState", "RtcConfiguration", "RtcDataChannel", "RtcDataChannelInit", "RtcDataChannelType", ] } serde-wasm-bindgen = { version = "0.4" } diff --git a/matchbox_socket/src/webrtc_socket/wasm/message_loop.rs b/matchbox_socket/src/webrtc_socket/wasm/message_loop.rs index 683f1e53..5f474be2 100644 --- a/matchbox_socket/src/webrtc_socket/wasm/message_loop.rs +++ b/matchbox_socket/src/webrtc_socket/wasm/message_loop.rs @@ -425,7 +425,20 @@ fn create_rtc_peer_connection(config: &WebRtcSocketConfig) -> RtcPeerConnection }; let ice_server_config_list = [ice_server_config]; peer_config.ice_servers(&serde_wasm_bindgen::to_value(&ice_server_config_list).unwrap()); - RtcPeerConnection::new_with_configuration(&peer_config).unwrap() + let connection = RtcPeerConnection::new_with_configuration(&peer_config).unwrap(); + + let connection_1 = connection.clone(); + let oniceconnectionstatechange: Box = Box::new(move |_event: JsValue| { + debug!( + "ice connection state changed: {:?}", + connection_1.ice_connection_state() + ); + }); + let oniceconnectionstatechange = Closure::wrap(oniceconnectionstatechange); + connection + .set_oniceconnectionstatechange(Some(oniceconnectionstatechange.as_ref().unchecked_ref())); + + connection } fn create_data_channel( From bac3f70b6452c4b891c3bd324e138cb9260fc736 Mon Sep 17 00:00:00 2001 From: Johan Klokkhammer Helsing Date: Mon, 26 Dec 2022 12:29:19 +0100 Subject: [PATCH 3/5] wasm: Handle null ice candidate events --- .../src/webrtc_socket/wasm/message_loop.rs | 56 +++++++++++-------- 1 file changed, 32 insertions(+), 24 deletions(-) diff --git a/matchbox_socket/src/webrtc_socket/wasm/message_loop.rs b/matchbox_socket/src/webrtc_socket/wasm/message_loop.rs index 5f474be2..f433137b 100644 --- a/matchbox_socket/src/webrtc_socket/wasm/message_loop.rs +++ b/matchbox_socket/src/webrtc_socket/wasm/message_loop.rs @@ -200,19 +200,20 @@ async fn handshake_offer( // send ICE candidates to remote peer let signal_peer_ice = signal_peer.clone(); let onicecandidate: Box = Box::new( - move |event: RtcPeerConnectionIceEvent| match event.candidate() { - Some(candidate) => { - let candidate = js_sys::JSON::stringify(&candidate.to_json()) + move |event: RtcPeerConnectionIceEvent| { + let candidate_json = match event.candidate() { + Some(candidate) => js_sys::JSON::stringify(&candidate.to_json()) .expect("failed to serialize candidate") .as_string() - .unwrap(); + .unwrap(), + None => { + debug!("Received RtcPeerConnectionIceEvent with no candidate. This means there are no further ice candidates for this session"); + "null".to_string() + } + }; - debug!("sending IceCandidate signal: {candidate:?}"); - signal_peer_ice.send(PeerSignal::IceCandidate(candidate)); - } - None => { - debug!("Received RtcPeerConnectionIceEvent with no candidate. This means there are no further ice candidates for this session"); - } + debug!("sending IceCandidate signal: {candidate_json:?}"); + signal_peer_ice.send(PeerSignal::IceCandidate(candidate_json)); }, ); let onicecandidate = Closure::wrap(onicecandidate); @@ -258,17 +259,23 @@ async fn handshake_offer( } async fn try_add_rtc_ice_candidate(connection: &RtcPeerConnection, candidate_string: &str) { - let candidate_init = match js_sys::JSON::parse(candidate_string).map(RtcIceCandidateInit::from) - { - Ok(js_value) => js_value, + let parsed_candidate = match js_sys::JSON::parse(candidate_string) { + Ok(c) => c, Err(err) => { error!("failed to parse candidate json: {err:?}"); return; } }; + let candidate_init = if parsed_candidate.is_null() { + debug!("Received null ice candidate, this means there are no further ice candidates"); + None + } else { + Some(RtcIceCandidateInit::from(parsed_candidate)) + }; + JsFuture::from( - connection.add_ice_candidate_with_opt_rtc_ice_candidate_init(Some(&candidate_init)), + connection.add_ice_candidate_with_opt_rtc_ice_candidate_init(candidate_init.as_ref()), ) .await .expect("failed to add ice candidate"); @@ -351,19 +358,20 @@ async fn handshake_accept( let signal_peer_ice = signal_peer.clone(); // todo: exactly the same as offer, dedup? let onicecandidate: Box = Box::new( - move |event: RtcPeerConnectionIceEvent| match event.candidate() { - Some(candidate) => { - let candidate = js_sys::JSON::stringify(&candidate.to_json()) + move |event: RtcPeerConnectionIceEvent| { + let candidate_json = match event.candidate() { + Some(candidate) => js_sys::JSON::stringify(&candidate.to_json()) .expect("failed to serialize candidate") .as_string() - .unwrap(); + .unwrap(), + None => { + debug!("Received RtcPeerConnectionIceEvent with no candidate. This means there are no further ice candidates for this session"); + "null".to_string() + } + }; - debug!("sending IceCandidate signal: {candidate:?}"); - signal_peer_ice.send(PeerSignal::IceCandidate(candidate)); - } - None => { - debug!("Received RtcPeerConnectionIceEvent with no candidate. This means there are no further ice candidates for this session"); - } + debug!("sending IceCandidate signal: {candidate_json:?}"); + signal_peer_ice.send(PeerSignal::IceCandidate(candidate_json)); }, ); let onicecandidate = Closure::wrap(onicecandidate); From 4d56a15cad699f61fe0983b0cb7b37dc106a2807 Mon Sep 17 00:00:00 2001 From: Johan Klokkhammer Helsing Date: Mon, 26 Dec 2022 12:40:25 +0100 Subject: [PATCH 4/5] wasm: Add missing .forget() calls for event handlers --- matchbox_socket/src/webrtc_socket/wasm/message_loop.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/matchbox_socket/src/webrtc_socket/wasm/message_loop.rs b/matchbox_socket/src/webrtc_socket/wasm/message_loop.rs index f433137b..6f4b590b 100644 --- a/matchbox_socket/src/webrtc_socket/wasm/message_loop.rs +++ b/matchbox_socket/src/webrtc_socket/wasm/message_loop.rs @@ -218,6 +218,8 @@ async fn handshake_offer( ); let onicecandidate = Closure::wrap(onicecandidate); conn.set_onicecandidate(Some(onicecandidate.as_ref().unchecked_ref())); + // note: we can let rust keep ownership of this closure, since we replace + // the event handler later in this method when ice is finished // handle pending ICE candidates for candidate in received_candidates { @@ -252,6 +254,7 @@ async fn handshake_offer( }); let onicecandidate = Closure::wrap(onicecandidate); conn.set_onicecandidate(Some(onicecandidate.as_ref().unchecked_ref())); + onicecandidate.forget(); debug!("Ice completed: {:?}", conn.ice_gathering_state()); @@ -376,6 +379,8 @@ async fn handshake_accept( ); let onicecandidate = Closure::wrap(onicecandidate); conn.set_onicecandidate(Some(onicecandidate.as_ref().unchecked_ref())); + // note: we can let rust keep ownership of this closure, since we replace + // the event handler later in this method when ice is finished // handle pending ICE candidates for candidate in received_candidates { @@ -410,6 +415,7 @@ async fn handshake_accept( }); let onicecandidate = Closure::wrap(onicecandidate); conn.set_onicecandidate(Some(onicecandidate.as_ref().unchecked_ref())); + onicecandidate.forget(); debug!("Ice completed: {:?}", conn.ice_gathering_state()); @@ -445,6 +451,7 @@ fn create_rtc_peer_connection(config: &WebRtcSocketConfig) -> RtcPeerConnection let oniceconnectionstatechange = Closure::wrap(oniceconnectionstatechange); connection .set_oniceconnectionstatechange(Some(oniceconnectionstatechange.as_ref().unchecked_ref())); + oniceconnectionstatechange.forget(); connection } From 357144fa89861f72274b521d1ab617a52feb011c Mon Sep 17 00:00:00 2001 From: Johan Klokkhammer Helsing Date: Mon, 26 Dec 2022 13:19:31 +0100 Subject: [PATCH 5/5] restore wait for ice gathering to complete --- .../src/webrtc_socket/wasm/message_loop.rs | 52 +++++++++++++++++-- 1 file changed, 48 insertions(+), 4 deletions(-) diff --git a/matchbox_socket/src/webrtc_socket/wasm/message_loop.rs b/matchbox_socket/src/webrtc_socket/wasm/message_loop.rs index 6f4b590b..ca58cbd1 100644 --- a/matchbox_socket/src/webrtc_socket/wasm/message_loop.rs +++ b/matchbox_socket/src/webrtc_socket/wasm/message_loop.rs @@ -12,8 +12,8 @@ use wasm_bindgen::{prelude::*, JsCast, JsValue}; use wasm_bindgen_futures::JsFuture; use web_sys::{ MessageEvent, RtcConfiguration, RtcDataChannel, RtcDataChannelInit, RtcDataChannelType, - RtcIceCandidateInit, RtcPeerConnection, RtcPeerConnectionIceEvent, RtcSdpType, - RtcSessionDescriptionInit, + RtcIceCandidateInit, RtcIceGatheringState, RtcPeerConnection, RtcPeerConnectionIceEvent, + RtcSdpType, RtcSessionDescriptionInit, }; use crate::webrtc_socket::{ @@ -164,6 +164,13 @@ async fn handshake_offer( .await .efix()?; debug!("created offer for new peer"); + + // todo: the point of implementing ice trickle is to avoid this wait... + // however, for some reason removing this wait causes problems with NAT + // punching in practice. + // We should figure out why this is happening. + wait_for_ice_gathering_complete(conn.clone()).await; + signal_peer.send(PeerSignal::Offer(conn.local_description().unwrap().sdp())); let mut received_candidates = vec![]; @@ -256,7 +263,10 @@ async fn handshake_offer( conn.set_onicecandidate(Some(onicecandidate.as_ref().unchecked_ref())); onicecandidate.forget(); - debug!("Ice completed: {:?}", conn.ice_gathering_state()); + debug!( + "handshake_offer completed, ice gathering state: {:?}", + conn.ice_gathering_state() + ); Ok((signal_peer.id, data_channel)) } @@ -354,6 +364,12 @@ async fn handshake_accept( .await .efix()?; + // todo: the point of implementing ice trickle is to avoid this wait... + // however, for some reason removing this wait causes problems with NAT + // punching in practice. + // We should figure out why this is happening. + wait_for_ice_gathering_complete(conn.clone()).await; + let answer = PeerSignal::Answer(conn.local_description().unwrap().sdp()); signal_peer.send(answer); @@ -417,7 +433,10 @@ async fn handshake_accept( conn.set_onicecandidate(Some(onicecandidate.as_ref().unchecked_ref())); onicecandidate.forget(); - debug!("Ice completed: {:?}", conn.ice_gathering_state()); + debug!( + "handshake_accept completed, ice gathering state: {:?}", + conn.ice_gathering_state() + ); Ok((signal_peer.id, data_channel)) } @@ -456,6 +475,31 @@ fn create_rtc_peer_connection(config: &WebRtcSocketConfig) -> RtcPeerConnection connection } +async fn wait_for_ice_gathering_complete(conn: RtcPeerConnection) { + if conn.ice_gathering_state() == RtcIceGatheringState::Complete { + debug!("Ice gathering already completed"); + return; + } + + let (mut tx, mut rx) = futures_channel::mpsc::channel(1); + + let conn_clone = conn.clone(); + let onstatechange: Box = Box::new(move |_| { + if conn_clone.ice_gathering_state() == RtcIceGatheringState::Complete { + tx.try_send(()).unwrap(); + } + }); + + let onstatechange = Closure::wrap(onstatechange); + + conn.set_onicegatheringstatechange(Some(onstatechange.as_ref().unchecked_ref())); + + rx.next().await; + + conn.set_onicegatheringstatechange(None); + debug!("Ice gathering completed"); +} + fn create_data_channel( connection: RtcPeerConnection, incoming_tx: futures_channel::mpsc::UnboundedSender<(PeerId, Packet)>,