Closed Bug 1208096 Opened 6 years ago Closed 6 years ago

Server-reflexive candidates that are obtained from TURN servers do not always finish initializing

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox41 --- unaffected
firefox42 --- affected
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: bwc, Assigned: bwc)

References

Details

Attachments

(1 file)

This is preventing gathering from finishing for TURN IPv6 candidates. I see at least one place where we aren't firing the appropriate callback once a failure is encountered:

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_candidate.c#761

There may be more. Investigating.
Ok, that first link isn't exactly right, but failure here never appears to be handled: 

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_socket.c?offset=300#345

This is not the cause of the problem we're observing, but it should be fixed. It also appears that failures here are not handled:

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_candidate.c#766
What's the negative impact of this in practice?  If non-trivial, this should probably be a P1
It means that gathering won't finish if there are problems with TURN. Not the end of the world, but some services will care (in particular, those that don't support trickle ICE).
I see what is happening here. When creating a relay candidate, we also create a srflx to go along with it:

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_component.c#269

However, since we want this srflx to piggyback on the STUN requests that will be sent to the TURN server anyway, we prevent the normal init code from running on it:

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_component.c#273
and
https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_component.c?offset=0#626

However, the logic for disabling relay/srflx candidates that have an IP version mismatch lives down in nr_ice_candidate_initialize:
https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_candidate.c#559

This means that the "extra" srflx is left in limbo when the corresponding relay candidate is disabled.
This hints that any failure in the gathering of a relay candidate might end up leaving its corresponding srflx in a similar limbo. Will need to look closer.
Summary: TURN candidates do not always finish initializing → Server-reflexive candidates that are obtained from TURN servers do not always finish initializing
Bug 1208096: (WIP) Handle various failure cases for TURN gathering better.
Comment on attachment 8665745 [details]
MozReview Request: Bug 1208096: Handle various failure cases for TURN gathering better. r=drno

Bug 1208096: Handle various failure cases for TURN gathering better.
Attachment #8665745 - Attachment description: MozReview Request: Bug 1208096: (WIP) Handle various failure cases for TURN gathering better. → MozReview Request: Bug 1208096: Handle various failure cases for TURN gathering better.
Comment on attachment 8665745 [details]
MozReview Request: Bug 1208096: Handle various failure cases for TURN gathering better. r=drno

Bug 1208096: Handle various failure cases for TURN gathering better.
Comment on attachment 8665745 [details]
MozReview Request: Bug 1208096: Handle various failure cases for TURN gathering better. r=drno

Bug 1208096: Handle various failure cases for TURN gathering better.
Comment on attachment 8665745 [details]
MozReview Request: Bug 1208096: Handle various failure cases for TURN gathering better. r=drno

Bug 1208096: Handle various failure cases for TURN gathering better.
Comment on attachment 8665745 [details]
MozReview Request: Bug 1208096: Handle various failure cases for TURN gathering better. r=drno

Bug 1208096: Handle various failure cases for TURN gathering better.
Attachment #8665745 - Flags: review?(drno)
Attachment #8665745 - Flags: review?(drno) → review+
Comment on attachment 8665745 [details]
MozReview Request: Bug 1208096: Handle various failure cases for TURN gathering better. r=drno

https://reviewboard.mozilla.org/r/20293/#review18743

LGTM

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c:278
(Diff revision 5)
> +    if (!cand->done_cb) {

How about:
  if (!cand || !cand->done_cb)
to be safe?

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c:778
(Diff revision 5)
> -    if(_status){
> +    if(_status && (cand->u.relayed.turn->state==NR_TURN_CLIENT_STATE_ALLOCATING)){

If _status is set cand->u.relayed.turn->state can be != NR_TURN_CLIENT_STATE_ALLOCATING. Why do these other failure do not result in nr_turn_client_failed() getting called?
https://reviewboard.mozilla.org/r/20293/#review18743

> How about:
>   if (!cand || !cand->done_cb)
> to be safe?

This seems like it needs an assert. Why would cand be 0?
https://reviewboard.mozilla.org/r/20293/#review18743

> This seems like it needs an assert. Why would cand be 0?

Sure, why not.

> If _status is set cand->u.relayed.turn->state can be != NR_TURN_CLIENT_STATE_ALLOCATING. Why do these other failure do not result in nr_turn_client_failed() getting called?

If nr_turn_client_allocate fails, it will call nr_turn_client_failed, and leave the state in NR_TURN_CLIENT_STATE_FAILED. However, if nr_ice_socket_register_turn_client fails, it will not do any of this.
Comment on attachment 8665745 [details]
MozReview Request: Bug 1208096: Handle various failure cases for TURN gathering better. r=drno

Bug 1208096: Handle various failure cases for TURN gathering better. r=drno
Attachment #8665745 - Attachment description: MozReview Request: Bug 1208096: Handle various failure cases for TURN gathering better. → MozReview Request: Bug 1208096: Handle various failure cases for TURN gathering better. r=drno
Check back on try.
Flags: needinfo?(docfaraday)
Lots and lots of orange, but none of it seems to be mine.
Flags: needinfo?(docfaraday)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9a1e69be5cf4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.