Closed Bug 1015409 Opened 6 years ago Closed 6 years ago

Race condition when local candidates are gathered between the creation and setting of the local description.

Categories

(Core :: WebRTC: Signaling, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed
firefox32 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: bwc, Assigned: ekr)

Details

Attachments

(1 file, 1 obsolete file)

When we generate a local description, we fetch the current list of gathered candidates, and connect to a signal for further gathered candidates here:

http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp#601

In the case where we are the answerer, once we set the remote offer, we remain in HAVE_REMOTE_OFFER until we set the local answer (no state change occurs when we create our answer, and attach to the trickle candidate signal). The state machine tables in sipcc discard any local trickle candidates while in this state, which means they can be lost if they are discovered between the time where the local answer is created, and when it is set:

http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c?from=fsmdef_ev_default&case=true#768

In the case that we are the offerer, the trickle candidate signal is not ignored, but it can cause local trickle candidates to be sent to content _before_ the local offer is even set.
Assignee: nobody → ekr
Status: NEW → ASSIGNED
Comment on attachment 8428492 [details] [diff] [review]
Fix trickle between CreateOffer() and SetLocal()

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

Byron,

Here is a WIP patch. LMK what you think?
Attachment #8428492 - Flags: feedback?(docfaraday)
Comment on attachment 8428492 [details] [diff] [review]
Fix trickle between CreateOffer() and SetLocal()

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

r+ with nit

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +4288,5 @@
> +			       (sll_lite_node_t *)buffered_cand) != SLL_LITE_RET_SUCCESS)
> +	    return SM_RC_END;
> +
> +	/* Don't notify upward */
> +	return SM_RC_END;

My only gripe is that you've mixed tabs/spaces in this block of code.
Attachment #8428492 - Flags: review+
Attachment #8428492 - Flags: feedback?(docfaraday)
Attachment #8428492 - Flags: feedback+
Attachment #8428492 - Attachment is obsolete: true
Comment on attachment 8429503 [details] [diff] [review]
Fix trickle between CreateOffer() and SetLocal().

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

carry forward r+ from bwc
Attachment #8429503 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/8797e572b395
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment on attachment 8429503 [details] [diff] [review]
Fix trickle between CreateOffer() and SetLocal().

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

   Bug 842549

User impact if declined: 

   Intermittent failed connectivity establishment, especially when the main thread is busy and experiencing queuing delay.

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

   Hand-testing, unit-testing, and CI testing.

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

   Very low.

String or IDL/UUID changes made by this patch:

   None.
Attachment #8429503 - Flags: approval-mozilla-beta?
Attachment #8429503 - Flags: approval-mozilla-aurora?
Comment on attachment 8429503 [details] [diff] [review]
Fix trickle between CreateOffer() and SetLocal().

We are too close to final 30 Beta to take this when it's not a blocker for release, approving for Aurora though so it can get the next full Beta cycle for more bake time.
Attachment #8429503 - Flags: approval-mozilla-beta?
Attachment #8429503 - Flags: approval-mozilla-beta-
Attachment #8429503 - Flags: approval-mozilla-aurora?
Attachment #8429503 - Flags: approval-mozilla-aurora+
I will point out that, even with its meager size, the change in this patch is radically smaller than it appears. Most of the changes highlighted in the diff are due to a change in indent width. The only functional changes are an update to the FSM table, plus one additional clause on a conditional.
Without this fix WebRTC takes a massive regression to the point of it being unusable for 20% of calls according to reports we got from the vendor. The patch is very unlikely to break anything but WebRTC. Do we really want to break 20% of WebRTC users?
Just for clarity:
We are already seeing the regression in FF 29. We don't know that this fixes the issue,
but we do know that this is a defect (see the attached test case in signaling_unittest
which demonstrates the problem w/o this patch) and we have some hope that this will have
an impact on the failure rate. I agree with Andreas's assessment that this is unlikely
to have any real impact outside of WebRTC.
Comment on attachment 8429503 [details] [diff] [review]
Fix trickle between CreateOffer() and SetLocal().

Since it only affects WEbRTC and of course we do not want 20% of users to not be able to use this excellent technology - we'll go ahead and take this in our final beta.
Attachment #8429503 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.