Closed Bug 1018473 Opened 5 years ago Closed 5 years ago

Answerer frequently emits duplicate trickle candidates

Categories

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

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: bwc, Assigned: bwc)

Details

Attachments

(2 files, 2 obsolete files)

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.
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
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
Attachment #8431942 - Flags: review?(ekr)
Go ahead and remove a param that I was worried would change, but that we don't actually use.
Attachment #8431942 - Attachment is obsolete: true
Attachment #8431942 - Flags: review?(ekr)
Attachment #8431995 - Flags: review?(ekr)
Assignee: docfaraday → ekr
Attachment #8432123 - Flags: review?(docfaraday)
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.
Attachment #8431995 - Flags: review?(ekr) → review+
Attachment #8431995 - Attachment is obsolete: true
Assignee: ekr → docfaraday
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
Attachment #8432169 - Flags: review+
Comment on attachment 8432123 [details] [diff] [review]
Unit test for duplicate trickle candidates

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

lgtm
Attachment #8432123 - Flags: review?(docfaraday) → review+
Keywords: checkin-needed
removing checkin needed since it seems ekr landed this in comment 9
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/01cd5e37935a
https://hg.mozilla.org/mozilla-central/rev/7c9493f52d19
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.