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

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

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.

Comment 1

[Byron Campen [:bwc]]

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

Comment 2

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

What's the negative impact of this in practice?  If non-trivial, this should probably be a P1

Comment 3

[Byron Campen [:bwc]]

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).

Comment 4

[Byron Campen [:bwc]]

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.

Comment 5

[Byron Campen [:bwc]]

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.

Comment 6

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1208096: Handle various failure cases for TURN gathering better. r=drno

Bug 1208096: (WIP) Handle various failure cases for TURN gathering better.

Comment 7

[Byron Campen [:bwc]]

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 8

[Byron Campen [:bwc]]

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 9

[Byron Campen [:bwc]]

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 10

[Byron Campen [:bwc]]

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 11

[Byron Campen [:bwc]]

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 12

[Nils Ohlmeier [:drno]]

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?

Comment 13

[Eric Rescorla (:ekr)]

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?

Comment 14

[Byron Campen [:bwc]]

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 15

[Byron Campen [:bwc]]

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

Comment 16

[Byron Campen [:bwc]]

Check back on try.

Comment 17

[Byron Campen [:bwc]]

Lots and lots of orange, but none of it seems to be mine.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a1e69be5cf4

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9a1e69be5cf4