shinyoshiaki / werift-webrtc

WebRTC Implementation for TypeScript (Node.js), includes ICE/DTLS/SCTP/RTP/SRTP/WEBM/MP4
MIT License
472 stars 31 forks source link

Fix race conditions by switching to node webcrypto #396

Closed koush closed 1 month ago

koush commented 1 month ago

builds on top of https://github.com/shinyoshiaki/werift-webrtc/pull/395

certificate creation is async now, so the setupCertificate method needs to handle double calls correctly.

the main issue that was uncovered is that localDescription is now undefined when ice candidate gathering starts.

The underlying issue is that ice candidate gathering is being started during a setRemoteDescription offer. Werift (the answer peer) has not created an answer or called setLocalDescription at this point. But it has started gathering peers based on the offer side description.

https://github.com/shinyoshiaki/werift-webrtc/blob/18b4194c271410da132000e9e460424a6c8cc3f5/packages/webrtc/src/peerConnection.ts#L1000-L1012

I believe this behavior is incorrect. Ice candidate gathering should only ever start during createAnswer and createOffer (or possibly setLocalDescription), as that is the typical spec/implementation. And that is also what is expected by onIceCandidate. https://github.com/shinyoshiaki/werift-webrtc/blob/18b4194c271410da132000e9e460424a6c8cc3f5/packages/webrtc/src/peerConnection.ts#L509-L513

koush commented 1 month ago

Another option is to queue the dropped candidates and flush them on setLocalDescription.

koush commented 1 month ago

I've fixed the underlying issue where setRemoteDescription is trigger candidating generation prior to createAnswer.

koush commented 1 month ago

I think this is finished now

shinyoshiaki commented 1 month ago

thanks!!