932970 - Trickle ICE doesn't always send a null-candidate at the end of the candidate list

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Mark Banner (:standard8)]

I've been looking at ICE candidate handling, and I've noticed that null candidates don't always get generated.

Doing some debugging in PeerConnection.js, I saw:

- onSetLocalDescription getting called with the ice gathering state != complete
- two ice candidates being generated
- handleIceStateChanges gets called with "IceWaiting"

at this stage, there's no null candidate being generated.

Further investigations reveal that 'this.localDescription' is never going to be valid, as PeerConnectionObserver has no such property/accessor.

This is the offending code:

http://hg.mozilla.org/mozilla-central/annotate/829d7bef8b0a/dom/media/PeerConnection.js#l1025

Comment 1

[Mark Banner (:standard8)]

Attachment: Possible fix

I think this should fix it - it generates a null ICE candidate for the example I'm seeing locally, and I've confirmed it doesn't generate a null ICE candidate when the candidates are all obtained before a local description has been generated.

Comment 2

[Adam Roach [:abr]]

Comment on attachment 824871 [details] [diff] [review]
Possible fix

This looks right to me.

Comment 3

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e7f975dc8e7

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6e7f975dc8e7

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 824871 [details] [diff] [review]
Possible fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 842549 (this bug implemented the trickle ice feature)
User impact if declined: Developers connecting Firefox WebRTC to servers not implementing trickle ICE won't know when the candidate list has completed, leading to either not being able to connect, or connections taking longer than necessary. 
Testing completed (on m-c, etc.): Landed on m-c, tested fix locally
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Comment 6

[bhavana bajaj [:bajaj]]

Comment on attachment 824871 [details] [diff] [review]
Possible fix

Approving the patch given the low risk vs the user impact. But will be helpful if we can verify this or add a testcase here.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/59513e28638b