paullouisageneau / libdatachannel

C/C++ WebRTC network library featuring Data Channels, Media Transport, and WebSockets
https://libdatachannel.org/
Mozilla Public License 2.0
1.81k stars 366 forks source link

setLocalDescription's Init parameter may not work. #1218

Closed xicilion closed 5 months ago

xicilion commented 5 months ago

In the following code snippet:

const offerSdp = await peerConnection.createOffer();
const mungedOfferSdp = munge(offerSdp, ufrag);
await peerConnection.setLocalDescription(mungedOfferSdp);

When setLocalDescription internally calls PeerConnection::setLocalDescription and passes the Init parameter, it's because of the presence of the following code(https://github.com/paullouisageneau/libdatachannel/blob/master/src/peerconnection.cpp#L101):

// Only a local offer resets the negotiation needed flag
if (type == Description::Type::Offer && !impl()->negotiationNeeded.exchange(false)) {
PLOG_DEBUG << "No negotiation needed";
return;
}

The method iceTransport->setIceAttributes will not be called.

paullouisageneau commented 5 months ago

Do you use node-datachannel's polyfill? The issue is that the createOffer()/setLocalDescription() process is completely faked in the polyfill.

If I'm not mistaken, under the hood the polyfill relies on createDataChannel() to start the connection process (disableAutoNegotiation is false so it calls setLocalDescription() internally), which triggers onLocalDescription() and populates localOffer, which is returned by createOffer().

Then, the call to setLocalDescription() actually does nothing because the description was already set on createDataChannel() so you can't set it again. I wonder why the call is even here because it is useless and could be replaced by a noop.

The way the polyfill currently works effectively prevents you from calling setLocalDescription() on libdatachannel.

xicilion commented 5 months ago

Yes, you are right. The behavior of createOffer here is implemented based on the node-datachannel's polyfill. But it seems that we can't get the correct localDescription without creating a socket.

xicilion commented 5 months ago

made a little change, just called the iceTransport->setIceAttributes before checking the type, and it seems to work after testing. But I'm not sure if there will be any side effects like this.

paullouisageneau commented 1 week ago

made a little change, just called the iceTransport->setIceAttributes before checking the type, and it seems to work after testing. But I'm not sure if there will be any side effects like this.

You can't do that, it introduces a side effect when it fails and the description ends-up half applied. The polyfill needs to be fixed.