1295552 - no relay candidates after ice restarts

Status: VERIFIED FIXED

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

Description

[Philipp Hancke [:fippo]]

Andreas Pehrsons just experienced an ice failure and I noticed something interesting about it. After the ice restart no relay candidates were gathered.

I managed to reproduce this in a fiddle: https://jsfiddle.net/ww22ayuk/4/
(turn credentials might have to be updated soon)

The odd behaviour is that before the restart, there are relay candidates gathered but none after the restart (which happens after about 10 seconds)

Comment 1

[Andreas Pehrson [:pehrsons]]

mjf, I think this one is for you. Wasn't sure if I should make it a P1 or a P2 since it's not a regression but I think a P1 is justified since this kind of breaks the purpose of ICE restart.

Comment 2

[Philipp Hancke [:fippo]]

Attachment: jsfiddle-pcap.pcapng

this is a pcap from captured while running the fiddle. We can see the initial TURN allocation at the beginning of this with the allocation that generates the candidate in packet #17 (I am not sure why it allocates two different ports in packets #18 and 19?!).

There are refresh requests with a lifetime of 0 in packets #34 and #35. This is probably closing the ports from the first generation of ice candidates. But I do not see any attempt to create new allocations which should happen in parallel.

the fiddle does currently not use a stun server, this might be worth checking in addition.

Comment 3

[Michael Froman [:mjf]]

At least in the case of this fiddle, it looks like only the offerer side is actually restarting ice.  I'm getting an rr run to see what is really happening.

Comment 4

[Michael Froman [:mjf]]

Attachment: Bug 1295552 - restart ice ctx needs stun/turn/dns setup to match original ctx.

Review commit: https://reviewboard.mozilla.org/r/73024/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73024/

Comment 5

[Nils Ohlmeier [:drno]]

Comment on attachment 8783052 [details]
Bug 1295552 - restart ice ctx needs stun/turn/dns setup to match original ctx.

https://reviewboard.mozilla.org/r/73024/#review70788

One smaller issue, but otherwise LGTM.

::: media/mtransport/nricectxhandler.cpp:110
(Diff revision 1)
> +  NrIceResolver* resolver =
> +    static_cast<NrIceResolver*>(this->current_ctx->ctx_->resolver->obj);
> +  if (!resolver ||
> +      NS_FAILED(new_ctx->SetResolver(resolver->AllocateResolver()))) {

This makes me wonder if we should try to preserve the DNS lookup results from the old ICE ctx (to avoid looking them up again)?
Might be worth filling a follow up ticket for later optimization.

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:187
(Diff revision 1)
> +        ABORT(R_NO_MEMORY);
> +      }
> +      if (r = r_data_create(&ctx->turn_servers[i].password,
> +                            servers[i].password->data,
> +                            servers[i].password->len)) {
> +        RFREE(ctx->turn_servers[i].username);

You think it's worth trying to free the N's username here, but leave the usernames and password for 1..N-1 allocated?
As your function does not return how many turn servers is copy over at this point I would prefer to either clean up everything, or don't free the username here either (if memory allocation fails we are pretty much doomed any how).

Comment 6

[Michael Froman [:mjf]]

Comment on attachment 8783052 [details]
Bug 1295552 - restart ice ctx needs stun/turn/dns setup to match original ctx.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73024/diff/1-2/

Comment 7

[Michael Froman [:mjf]]

Comment on attachment 8783052 [details]
Bug 1295552 - restart ice ctx needs stun/turn/dns setup to match original ctx.

https://reviewboard.mozilla.org/r/73024/#review70788

> This makes me wonder if we should try to preserve the DNS lookup results from the old ICE ctx (to avoid looking them up again)?
> Might be worth filling a follow up ticket for later optimization.

Created Bug 1296755.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e7b0bdaa0661
restart ice ctx needs stun/turn/dns setup to match original ctx. r=drno

Comment 9

[Iris Hsiao [:ihsiao]]

Sorry had to backout for GTest crashed @mozilla::NrIceCtxHandler::CreateCtx, e.g., https://treeherder.mozilla.org/logviewer.html#?job_id=2286074&repo=autoland#L0-L9855
https://treeherder.mozilla.org/logviewer.html#?job_id=2284755&repo=autoland#L8027

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9f442b970ac1
Backed out changeset e7b0bdaa0661 for GTest crashed @mozilla::NrIceCtxHandler::CreateCtx

Comment 11

[Michael Froman [:mjf]]

Comment on attachment 8783052 [details]
Bug 1295552 - restart ice ctx needs stun/turn/dns setup to match original ctx.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73024/diff/2-3/

Comment 12

[Michael Froman [:mjf]]

(In reply to Iris Hsiao [:ihsiao] from comment #9)
> Sorry had to backout for GTest crashed @mozilla::NrIceCtxHandler::CreateCtx,
> e.g.,
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=2286074&repo=autoland#L0-L9855
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=2284755&repo=autoland#L8027

I have pushed a new change to review that fixes this issue.  Sorry for the need to backout!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60145a6103ee
restart ice ctx needs stun/turn/dns setup to match original ctx. r=drno

Comment 14

[Michael Froman [:mjf]]

Comment on attachment 8783052 [details]
Bug 1295552 - restart ice ctx needs stun/turn/dns setup to match original ctx.

Approval Request Comment
[Feature/regressing bug #]: Bug 906986 - Support ICE restart.
[User impact if declined]: Ice restart through firewall/NAT will fail because no relay candidates will be found.  Without this fix, Ice restart doesn't work, and ICE restart is an important tool for improving call success rates for WebRTC users.
[Describe test coverage new/current, TreeHerder]: GTest for ice_unittest currently tests the restart mechanism, but does not inspect for relay candidates.
[Risks and why]: There is little risk in accepting this patch.  The changes are localized to a method (NrIceCtxHandler::CreateCtx) that is only called during ice restart, plus a new function in ice_ctx.c that is only used from NrIceCtxHandler::CreateCtx.  Normal ice functionality is not affected.
[String/UUID change made/needed]: none

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/60145a6103ee

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Philipp, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I'd like this to stabilize on Nightly for a few days and perhaps uplift early next week.

Comment 18

[Philipp Hancke [:fippo]]

works for me in 51.0a1 (2016-08-28)!

Comment 19

[Michael Froman [:mjf]]

(In reply to Philipp Hancke from comment #18)
> works for me in 51.0a1 (2016-08-28)!

Thanks for verifying!

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8783052 [details]
Bug 1295552 - restart ice ctx needs stun/turn/dns setup to match original ctx.

Fix was verified on Nightly and has stabilized over the weekend, Aurora50+

Comment 21

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/21e634320cc7

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8783052 [details]
Bug 1295552 - restart ice ctx needs stun/turn/dns setup to match original ctx.

Verified fix, sounds like this is unlikely to affect anyone beyond the users who already are not in an ideal situation. Let's take this for beta 9.

Comment 23

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

https://hg.mozilla.org/releases/mozilla-beta/rev/b9fca8a64c7d