Closed Bug 932970 Opened 6 years ago Closed 6 years ago
Trickle ICE doesn't always send a null-candidate at the end of the candidate list
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
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.
Attachment #824871 - Flags: review?(ekr)
Comment on attachment 824871 [details] [diff] [review] Possible fix This looks right to me.
Attachment #824871 - Flags: review?(ekr) → review+
Assignee: nobody → mbanner
Target Milestone: --- → mozilla28
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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
Attachment #824871 - Flags: approval-mozilla-aurora?
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.
Attachment #824871 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.