856425 - onicecandidate fails to fire in simple local to remote peer handshake with custom STUN server (see testcase)

Status: VERIFIED FIXED

(Core :: WebRTC, defect, P2)

Description

[Jason Smith [:jsmith]]

Attachment: Test Case

Steps:

1. Load the attached test case in Firefox
2. Watch the alert dialogs that fire
3. Run the same test case in Chrome

Expected:

We should get the same behavior in Chrome and Firefox. On Firefox's side, I would have expected onicecandidate to fire upon setLocalDescription being called on the local peer.

Actual:

onicecandidate never fires in Firefox after calling setLocalDescription, but it will fire in Chrome doing that same test case.

In general, the behavior Firefox does here is almost completely different than Chrome. Firefox basically does:

1. local pc createOffer
2. local pc setLocalDescription
3. remote pc setRemoteDescription
4. remote pc createAnswer
5. remote pc setLocalDescription
6. local pc setRemoteDescription

With each onicecandiate event handler never firing. Chrome's behavior however is entirely different:

1. local pc create offer
2. local pc setLocalDescription
3. local pc onicecandidate fired

console logs:

a=candidate:505434299 1 udp 2113937151 192.168.1.6 62237 typ host generation 0

audio
0

4. local pc onicecandidate fired

console logs:

a=candidate:505434299 1 udp 2113937151 192.168.1.6 62237 typ host generation 0

audio
0

5. local pc onicecandidate fired

console logs:

a=candidate:3078610798 1 udp 1845501695 98.248.43.172 62237 typ srflx raddr 192.168.1.6 rport 62237 generation 0

audio
0

6. local pc onicecandidate fired

console logs:

a=candidate:3078610798 1 udp 1845501695 98.248.43.172 62237 typ srflx raddr 192.168.1.6 rport 62237 generation 0

audio
0

7. local pc onicecandidate fired

console logs:

a=candidate:505434299 1 udp 2113937151 192.168.1.6 62237 typ host generation 0

audio
0

8. local pc onicecandidate fired

console logs:

a=candidate:505434299 1 udp 2113937151 192.168.1.6 62237 typ host generation 0

audio
0

9. local pc onicecandidate fired

candidate is null in this case

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

ekr (or abr): how big an issue is this?

Comment 2

[Eric Rescorla (:ekr)]

We need to generate an end of candidates indication, but not individual candidates.

abr: isn't this a dupe of something else.

Comment 3

[Adam Roach [:abr]]

To be clear, the spec does not say anything about onicecandidate being called with a null candidate once gathering is done; this is just something Chrome has decided to do, and several scripts rely on the behavior as a consequence.

The generation of trickle candidates is covered by bug 842549. Until that lands, the repeated calls to onicecandidate will not happen. What is important, from a "compatibility with Chrome" perspective, is the null candidate generation.

The current workaround, proposed by EKR, is to simply call onicecandidate with a null candidate every time setLocalDescription succeeds, immediately after the setRemoteDescription success callback. This is an easy fix.

Comment 4

[Adam Roach [:abr]]

(In reply to Adam Roach [:abr] from comment #3)
> To be clear, the spec does not say anything about onicecandidate being
> called with a null candidate once gathering is done; this is just something
> Chrome has decided to do, and several scripts rely on the behavior as a
> consequence.

I retract this statement. It appears that the spec *does* document this. It simply does it in such a way that matches none of the search terms I was using.

Comment 5

[Eric Rescorla (:ekr)]

Yes, I think it's clear that this is the required behavior.

Comment 6

[Eric Rescorla (:ekr)]

(In reply to Adam Roach [:abr] from comment #3)
> To be clear, the spec does not say anything about onicecandidate being
> called with a null candidate once gathering is done; this is just something
> Chrome has decided to do, and several scripts rely on the behavior as a
> consequence.
> 
> The generation of trickle candidates is covered by bug 842549. Until that
> lands, the repeated calls to onicecandidate will not happen. What is
> important, from a "compatibility with Chrome" perspective, is the null
> candidate generation.

Note that there is absolutely no requirement that we implement trickle ICE.
The sole requirement is that we generate the final event that signals the
end of gathering.


> The current workaround, proposed by EKR, is to simply call onicecandidate
> with a null candidate every time setLocalDescription succeeds, immediately
> after the setRemoteDescription success callback. This is an easy fix.

Comment 7

[Adam Roach [:abr]]

Attachment: Text case v2

The test case no longer works on nightly, as generation of an offer without any streams and without any constraints results in a failure. This testcase adds an "OfferToReceiveAudio" constraint to make the call succeed.

Comment 8

[Adam Roach [:abr]]

Attachment: Test Case v3

More fixes to the test. We can't really use alert(), since it hangs processing and perturbs the order of events; this outputs steps in the HTML body. This also fixes some issues where getting a null candidate causes the test to throw exceptions.

Comment 9

[Adam Roach [:abr]]

Attachment: Call onicecandidate with a null candidate immediately after setLocalDescription succeeds

Comment 10

[Eric Rescorla (:ekr)]

Comment on attachment 735209 [details] [diff] [review]
Call onicecandidate with a null candidate immediately after setLocalDescription succeeds

Review of attachment 735209 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 11

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/322f2cc8d58a

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/322f2cc8d58a

Comment 13

[Jason Smith [:jsmith]]

Looks okay.

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 735209 [details] [diff] [review]
Call onicecandidate with a null candidate immediately after setLocalDescription succeeds

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a

User impact if declined: Various WebRTC applications tested with Chrome assume that an empty ice candidate message will be sent, making it hard to write applications that work with both FF and Chrome without sniffing and alternative flows.

Testing completed (on m-c, etc.): on m-c

Risk to taking this patch (and alternatives if risky): no risk

String or IDL/UUID changes made by this patch: none

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f765f7510c36