1018473 - Answerer frequently emits duplicate trickle candidates

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, minor)

Description

[Byron Campen [:bwc]]

When we set a remote offer, sipcc constructs a throwaway local description by calling vcmRxAllocICE, which connects to a signal that fires when candidates are gathered. Then, when the answer is created, vcmRxAllocICE is called again, which connects to the same signal again. From there on, any trickled candidate is signaled twice, leading to duplicates.

Comment 1

[Byron Campen [:bwc]]

Attachment: Detect when vcmRxAllocICE has already been called for a given stream, and suppress the (duplicate) connection to SignalCandidate.

Initial cut.

Comment 2

[Byron Campen [:bwc]]

Comment on attachment 8431942 [details] [diff] [review]
Detect when vcmRxAllocICE has already been called for a given stream, and suppress the (duplicate) connection to SignalCandidate.

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

Pushing to try, in case somehow something is relying on the duplicate candidates.

https://tbpl.mozilla.org/?tree=Try&rev=ffe0cf97cb2b

Comment 3

[Byron Campen [:bwc]]

Attachment: Detect when vcmRxAllocICE has already been called for a given stream, and suppress the (duplicate) connection to SignalCandidate.

Go ahead and remove a param that I was worried would change, but that we don't actually use.

Comment 4

[Eric Rescorla (:ekr)]

Attachment: Unit test for duplicate trickle candidates

Comment 5

[Eric Rescorla (:ekr)]

Comment on attachment 8431995 [details] [diff] [review]
Detect when vcmRxAllocICE has already been called for a given stream, and suppress the (duplicate) connection to SignalCandidate.

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

lgtm

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +592,5 @@
>    *candidatesp = nullptr;
>    *candidate_ctp = 0;
>  
> +  // This can be called multiple times; don't connect to the signal more than
> +  // once.

Please add a comment on why and maybe the bug # so people have background.

Comment 6

[Byron Campen [:bwc]]

Attachment: Detect when vcmRxAllocICE has already been called for a given stream, and suppress the (duplicate) connection to SignalCandidate.

Add a pointer to this bug for the sake of explanation.

Comment 7

[Byron Campen [:bwc]]

Comment on attachment 8432169 [details] [diff] [review]
Detect when vcmRxAllocICE has already been called for a given stream, and suppress the (duplicate) connection to SignalCandidate.

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

Carry forward r=ekr

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8432123 [details] [diff] [review]
Unit test for duplicate trickle candidates

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

lgtm

Comment 9

[Eric Rescorla (:ekr)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01cd5e37935a
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c9493f52d19

Comment 10

[Carsten Book [:Tomcat]]

removing checkin needed since it seems ekr landed this in comment 9

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/01cd5e37935a
https://hg.mozilla.org/mozilla-central/rev/7c9493f52d19