Webrtc-pc: addIceCandidate may not need to throw an error when no remoteDescription

[Sean-Der]

(This feedback is from Pion not my full-time job, go ahead and delete if that isn't ok)

This is defined here

If remoteDescription is null return a promise rejected with a newly created InvalidStateError.

---

I don't believe this restriction is needed, and many popular WebRTC libraries just circumvent this check. In simple-peer the library caches candidates, and then adds them.

I do the same in Pion.

I can get more datapoints from other implementations if that is helpful. I have seen this pattern in many private code bases as well.

---

From an implementer standpoint this is really easy to handle. If you get an ICE candidate before you have ICE details just cache them, and when you have enough details you can start connectivity checks.

I work with a lot of people learning WebRTC/building things for the first time because of Pion. This error is constant, and is probably my #2 issue (after H264 in Chrome questions/not support all profiles). Especially with the new rush of interest with COVID this is something that I will sometimes have people sharing logs multiple times in Slack a day with this error.

[fippo]

adding a candidate before the remote description is a programming error. Throwing it forces people to fix their programming and/or signaling protocol errors.

[Sean-Der]

It is an error only because this document defines it as such, if the document changes it isn't an error anymore. Developers have enough problems to worry about, why make them suffer from a death by a thousands cuts on stuff like this?

---

With this we require

• Either people put logic in their Client code to cache candidates
• Make their signaling servers smarter, making them cache ice candidates. This prevents people from using simple pub/sub code.

If I am a new developer and I open the firebase example I see that we have this event handler after setRemoteDescription, why not just move it before? There is nothing explicit about this.

---

If we removed this assert and moved the 'candidate caching' into the Clients all this would go away. This restriction also may not show up during local development, then when they go to prod they are stuck debugging this.

[alvestrand]

This happens in practice when the path for signaling candidates and the path for signaling state updates are uncoordinated, so that a remote candidate may race a remote description.

Stats show that rejection because of no remote description happens in 0.197% of AddIceCandidates calls. It's by far the most common reason for failed AddIceCandidates calls.

[fippo]

Stats show that rejection because of no remote description happens in 0.197% of AddIceCandidates calls

You're looking at global metrics which contain a lot of noise. I've seen error rates below 0.023 % (or five sigma) for a particular system. That was after significant efforts to analyse and eliminate other error causes.

[Sean-Der]

@fippo Is that stat from a system you controlled? The only metric that matters here to me is the global one.

When people realize they have this error they will go fix it. That 0.197% represents people having to debug this issue, and then go fix it. This isn't something that sticks around for people, but it has to be frustrating when they discover it.

[jan-ivar]

Hi @Sean-Der I worry this is covering up a bug somewhere. Does your signaling guarantee order?

WebRTC relies on a serialized signaling channel, so queuing ICE candidates is an anti-pattern. It

1. complicates negotiation & renegotiation
2. covers up signs of browser and/or application bugs (go ★ it)
3. defeats Trickle ICE in some cases, slowing down connection

The spec says onicecandidate cannot fire before SLD, guaranteeing this signaling order:

offer candidate candidate candidate

and in the reciprocal direction

answer candidate candidate candidate

This becomes important during rapid re-negotiation and ICE restarts, where we might see:

offer candidate candidate offer candidate candidate candidate

Does the second candidate belong to the first or second offer? Also, most queuing I've seen only works on initial negotiation, which may lead to missed candidates and intermittents if it was wallpapering a real timing hole in a browser or application.

Negotiation being an async state machine, strict state management is one of the few guide-rails we offer, so I don't think we should relax them.

[fippo]

@Sean-Der global stats have proven to be :hankey: in the past, e.g. when chrome rolled out AEC3 (which ended in a desaster on high-price headsets) or in the failed first attempt to deprecate DTLS 1.0.

[Sean-Der]

@jan-ivar This isn't a bug I am having personally. I have an Open Source project Pion and I see lots of developers struggling with this. Many of the people coming to the project are using WebRTC for the first time.

Does your signaling guarantee order?

This is going to bite people. I don't think I have seen a WebRTC tutorial that calls this out explicitly. I think we are asking too much of new developers to deal with this.

covers up signs of browser and/or application bugs (go ★ it)

IMO not a bug. WebRTC implementations more complicated with this requirement. If the ICE agent can just emit candidates when ready things would be better. There is now more logic for the agent to be aware of the PeerConnection state.

defeats Trickle ICE in some cases, slowing down connection

From reading the issue it sounds like this assert is the actual thing causing the issue. If we didn't have this requirement people could emit candidates at any time. The slowing down of trickle ICE (and extra logic) has happened because of the SetRemoteDescription requirement.

most queuing I've seen only works on initial negotiation

This is all I am looking for! I think after the initial negotiation all the logic stays the same.

---

I would just love to see this sharp edge removed (and I am still not convinced it is going to cost anything). If every user has to go look up this detail that is a lot of man hours wasted.

[jan-ivar]

@Sean-Der Please show me a simple example demonstrating the problem.

most queuing I've seen only works on initial negotiation

This is all I am looking for! I think after the initial negotiation all the logic stays the same.

The same methods and logic are used on renegotiation, so we have to think about that as well. E.g.

• App adding new tracks for a participant or screen-sharing sends additional ICE candidates
• App defers adding tracks until after initial negotiation
• App restarts ICE e.g. after connection failure
• App stops a BUNDLE transceiver

[jan-ivar]

Does your signaling guarantee order?

This is going to bite people. I don't think I have seen a WebRTC tutorial that calls this out explicitly. I think we are asking too much of new developers to deal with this.

What signaling channel are people bitten by this using?

[fippo]

This is going to bite people.

Ordering is a pretty natural guarantee actually. See https://tools.ietf.org/html/rfc6120#section-10.1 if you want a more formal description. It is a fundamental assumption in protocol design. For example it ensures that the join confirmation for a chatroom arrives before you get any messages. E.g. you get

C: join X
S: you joined X
S: here is an offer from the room jingle focus

I've only once seen a signaling protocol which didn't have this property. Signaling protocols aren't simple and there is a point where one should strongly consider whether reinventing the wheel makes sense. I'm biased of course and still prefer jingle over sending SDP on the wire :-)

Suppose you get candidates with a ufrag X. Then you get a remote description with ufrag Y. Do you reject the remote description or silently discard the generation with ufrag X?

[Sean-Der]

@Sean-Der Please show me a simple example demonstrating the problem.

Something like this? I took this from a highly voted answer on SO here. I add the setTimeOut just to make it happen everytime.

If I am a new developer reading the official webrtc.org docs on Trickle ICE why would I set onicecandidate after SetLocalDescription?

What signaling channel are people bitten by this using?
Disregard this, it is really a niche case. If we discuss this it is distracting from the main discussion IMO. Sorry for bringing it up

The same methods and logic are used on renegotiation, so we have to think about that as well. E.g.

I don't think this adds any new problems for renegotation? After the first remote-description has been received this remote candidate cache is never used again.

[fippo]

Something like this?

That is a known problem but as far as I know it only affects local examples. See https://bugs.chromium.org/p/chromium/issues/detail?id=960718
The tl;dr is that you call SLD and therefore start generating candidates before SRD is called.
With setTimeout you are intentionally screwing up the order. If it happens without the setTimeout... please file a bug!

If I am a new developer reading the official webrtc.org docs

Please note that webrtc.org is not "official". In particular the codelab there contains several errors and antipatterns and clearly lacks review.

[jan-ivar]

Something like this?

That example was correctly written until you added a 1 second delay after SLD. Again, from SO:

1. Send pc.localDescription no later than in the setLocalDescription success callback.
2. Expect pc.onicecandidate to start firing immediately after that callback. Send them.

Signaling is implicit & immediate in local-loop demos, and you never "sent" your offer (by not calling answerer.SRD(offer)) before your 1 second delay, while still sending candidates without delay, thus breaking order. E.g. it's as if you'd signaled:

candidate candidate candidate (1 second pause) offer

Note it still connects, since local candidates get inserted into the local offer as well, so this is just error spam from unnecessary trickling.

IOW, if you're going to delay initial connection by 1 second, do it anywhere else, since Trickle ICE exists solely to save that second.

I add the setTimeOut just to make it happen everytime.

If it happens without the setTimeout... please file a bug!

Yes please, and test multiple browsers. Without your setTimeout, even the local-loop case should be air tight, thanks to peer connection's operations chain (once SRD() is on that chain, addIceCandidate cannot race it).

[Sean-Der]

if you're going to delay initial connection by 1 second, do it anywhere else
People are going to abuse the API, most times accidentally. I am just trying to demonstrate where the API is needlessly fragile.

This feels like 'Undefined Behavior' conversations. You can tell developers Do not do that, but they are going to do it anyway. Why not design the API to be robust. You can chase help people on SO every time this comes up, but why not fix the root cause?

---

I think we will just have to agree to disagree. Since I don't have W3C membership the conversations ends here, I appreciate you taking your time to discuss it though! Thank you for all your hard work on the webrtc-pc API :)

[jan-ivar]

We've been working over the last year to make the API robust. I recommend the spec example.

why not fix the root cause?

Because there's no guarantee that allowing JS to do things out of order on initial negotiation won't then happen again on renegotiation, as I mentioned https://github.com/w3c/webrtc-pc/issues/2519#issuecomment-622085409. Most likely it would, and then it's much less clear how we'd resolve it correctly, as mentioned in https://github.com/w3c/webrtc-pc/issues/2519#issuecomment-622272086. I'm happy to pick up the conversation there if you have ideas on how to solve it.

I also suspect the "root cause" is or may have been a bug in Chrome. I'm going to rip out some wpt helpers to help find it, since being too lenient in tests appears to be coming back to bite us.

[fippo]

This feels like 'Undefined Behavior' conversations. You can tell developers Do not do that, but they are going to do it anyway. Why not design the API to be robust

This is a philosophical discussion and one which has value. See Postel was wrong for one opinion. I'm on the "throw errors, dont assume the browser implementors are smarter" side.
Doing a timing test for addIceCandidate that reliably covers all native code parts in all browsers is going to be somewhat impossible.

Now where did I put the document with "p2p ice failure rates dropped considerably when postponing addIceCandidate slightly in chrome...
(btw, you can still raise issues even without being a w3c member or invited expert... so far @dontcallmedom hasn't tried to stop me :-))

[jan-ivar]

where did I put the document with "p2p ice failure rates dropped considerably when postponing addIceCandidate slightly in chrome...

@fippo If you have similar data for Firefox that would help show if this is something other than an implementation bug.

[jan-ivar]

From editor's meeting: Agreement that throwing an error is the right thing to do.

We think the root cause was fixed in https://bugs.chromium.org/p/chromium/issues/detail?id=1019222

[Sean-Der]

@jan-ivar This came up again. A user was interfacing with an SFU (not using libwebrtc at all)

I think that adds another layer to the conversation! All the conversations before are in the 'single threaded' world of the browser. What about servers that don't even implement the W3C API?

[aldovincenti]

I tried the perfect negotiation example with a simple signaling server (Node.js + Socket.io, WebSocket protocol), and unfortunately, I found that sometimes I receive ICE candidates before the remote description. I had to queue ICE candidates manually, and only for this limitation. I think this problem is more common than we suppose, not surprisingly, as @Sean-Der mentioned, many libraries implement a queue. Please reconsider this limitation, it makes implementing WebRTC difficult and frustrating.

[jech]

Suppose you delegate caching of ICE candidates to the library. Now what happens if you do:

• pc.setLocalDecription
• pc.setRemoteDescription
• pc.addICECandidate
• pc.restartICE

How would the library know whether the ICE candidate belongs to the first ICE session (and therefore be applied immediately) or to the second ICE session (and therefore be cached)?

[fippo]

How would the library know whether the ICE candidate belongs to the first ICE session

That is why the ufrag was added at some point.