Closed Bug 932970 Opened 10 years ago Closed 10 years ago

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


(Core :: WebRTC: Signaling, defect)

Not set



Tracking Status
firefox26 --- unaffected
firefox27 --- fixed
firefox28 --- fixed


(Reporter: standard8, Assigned: standard8)



(Whiteboard: [qa-])


(1 file)

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:
Attached patch Possible fixSplinter Review
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
Closed: 10 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+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.