1142964 - WebRTC: Firefox sends ICE-CONTROLLING with tie breaker = 0x1 and doesn't respond to 487

Status: RESOLVED FIXED

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

Description

[Florin]

Attachment: role_conflict.pcap

User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2272.89 Safari/537.36

Steps to reproduce:

Setup a WebRTC audio+video (or just audio) session between Firefox(35+) and a media gateway. 

After each peer gather ICE candidates starts ICE checking. The gateway sends binding requests with ICE-CONTROLLING and a random tie breaker value. 
Next steps falls in one of the following types: 



Actual results:

Case 1: 

1. Firefox responds with Binding Success 
2. Firefox sends STUN binding request with ICE-CONTROLLING and tie breaker = 0x01. 
3. Gateway responds with ROLE CONFLICT. 
4. Firefox keeps sending binding requests with ICE-CONTROLLING and tie breaker = 0x01 ignoring ROLE CONFLICT. 
See role_conflict.pcap for the scenario. 

or: 

Case 2:
1. Firefox responds with Binding Success
2. Firefox sends STUN binding request with ICE-CONTROLLING and tie breaker = 0x01.
3. Firefox repeats step 2.
4. Firefox sends again a STUN binding request with ICE-CONTROLLED and a valid tie breaker (before even receive a ROLE CONFLICT response)
See role_change.pcap

In pcap files Firefox is 192.168.3.103 and media gateway is 10.150.3.117.


Expected results:

Firefox should send valid tie breaker values and respond to the ROLE CONFLICT message, respecting RFC 5245 when selecting ICE roles. 


Issue reproduces on Firefox on Windows (x86). Tested on Mac Firefox (same version) and didn't reproduce.

Comment 1

[Florin]

Attachment: role_change.pcap

Comment 2

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

NI to drno - real?  Still an issue?

Comment 3

[Nils Ohlmeier [:drno]]

Confirmed. I see tiebreaker being 0x1 on Windows in 43.0a1, but a random value on Mac.

Comment 4

[Eric Rescorla (:ekr)]

It seems like a debugger is needed.

Comment 5

[Nils Ohlmeier [:drno]]

So it turns out that the problem are the structs nr_stun_client_ice_use_candidate_params and nr_stun_client_ice_binding_request_params being used in a union in nr_stun_client_params (which BTW is a "secret" union as it does not follow the usual .u.x notation). Both structs differ by the one integer 'controlling' here https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/stun_build.h?offset=200#93 which apparently gets differently aligned by Win vs. Mac compilers (32bit vs. 64bit?).
But the two union members (use_candidate and binding_request) are both used at the same time in nr_ice_candidate_pair_create() https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_candidate_pair.c#128

The ugly hack solution I used to verify this is to add a dummy int to the nr_stun_client_ice_use_candidate_params struct to make them identical. The better solution is probably to either have nr_stun_build_use_candidate() take nr_stun_client_ice_binding_request_params as input parameter and then eliminate nr_stun_client_ice_use_candidate_params all together. Or even more extreme replace the nr_stun_build_use_candidate() with nr_stun_build_req_ice().

Byron do you want to look around if you find more problems caused by the secret union nr_stun_client_params?

Comment 6

[Byron Campen [:bwc]]

I don't see anywhere else that uses the |ice_use_candidate| aspect of the union:

https://dxr.mozilla.org/mozilla-central/search?q=%2Bvar-ref%3Anr_stun_client_params_%3A%3Aice_use_candidate

I think it would be fine to just remove nr_stun_client_ice_use_candidate_params. We'll fix this bug and reduce the amount of code at the same time.

Comment 7

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: Bug 1142964: fixed ICE tiebreaker on Windows. r?bwc

Bug 1142964: fixed ICE tiebreaker on Windows. r?bwc

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8651150 [details]
MozReview Request: Bug 1142964: fixed ICE tiebreaker on Windows. r?bwc

https://reviewboard.mozilla.org/r/16853/#review15009

Ship It!

Comment 9

[Nils Ohlmeier [:drno]]

Florin, as we don't have any media gateways to test with could install this windows build http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1137e38413b1/try-win32/firefox-43.0a1.en-US.win32.installer.exe
and report back if that fixes both problems (which I hope should be the case, but I'm not 100% certain).

Comment 10

[Nils Ohlmeier [:drno]]

I don't see any new problems in the try-run.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1eb1c6f76ffc

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1eb1c6f76ffc

Comment 13

[Florin]

Hi all !
Thank you for considering this issue. 
I tested the build. The problem was solved. Can be closed. 
Thank you again.

Comment 14

[Nils Ohlmeier [:drno]]

Thank you for initially reporting and verifying that fix works for you!