1229633 - Firefox PeerConnection fails to gather on Win with "failed to create UDP candidates with error 6"

Status: RESOLVED FIXED

Description

[yanfzhen]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151112144305

Steps to reproduce:

1. Call createOffer

2. Got onsignalingstatechange event callback: RTCPeerConnection.gotSignalingStateChange: peerId 0 signalingState: have-local-offer iceConnectionState= new iceGatheringState= new

3. Got local offer:

v=0\r\no=mozilla...THIS_IS_SDPARTA-42.0 9112119681318040057 0 IN IP4 0.0.0.0\r\ns=-\r\nt=0 0\r\na=fingerprint:sha-256 AB:41:CB:18:BF:17:E4:39:63:45:2D:03:FB:FF:96:EC:5D:0F:E8:B1:FD:6F:FD:15:01:2E:0E:15:8E:21:42:4E\r\na=ice-options:trickle\r\na=msid-semantic:WMS *\r\nm=video 9 RTP/SAVPF 120 126 97\r\nc=IN IP4 0.0.0.0\r\na=recvonly\r\na=fmtp:120 max-fs=12288;max-fr=60\r\na=fmtp:126 profile-level-id=42e01f;level-asymmetry-allowed=1;packetization-mode=1\r\na=fmtp:97 profile-level-id=42e01f;level-asymmetry-allowed=1\r\na=ice-pwd:321af14dcdc7859498fc03afe628928e\r\na=ice-ufrag:0-eb3c981d\r\na=mid:sdparta_0\r\na=rtcp-fb:120 nack\r\na=rtcp-fb:120 nack pli\r\na=rtcp-fb:120 ccm fir\r\na=rtcp-fb:126 nack\r\na=rtcp-fb:126 nack pli\r\na=rtcp-fb:126 ccm fir\r\na=rtcp-fb:97 nack\r\na=rtcp-fb:97 nack pli\r\na=rtcp-fb:97 ccm fir\r\na=rtcp-mux\r\na=rtpmap:120 VP8/90000\r\na=rtpmap:126 H264/90000\r\na=rtpmap:97 H264/90000\r\na=setup:actpass\r\na=ssrc:1955173493 cname:{8365168d-fac6-4e68-8ad8-0cdad2e753b1}\r\n

4. Got ICE failed from oniceconnectionstatechange event callback:

RTCPeerConnection.gotIceConnectionChange: peerId 0 iceConnectionState= failed

5. Got remote answer sdp:

v=0\r\no=- 6608001165902521337 2 IN IP4 127.0.0.1\r\ns=-\r\nc=IN IP4 10.224.196.188\r\nt=0 0\r\na=fingerprint:sha-256 44:43:B1:EB:F8:CD:03:35:40:9D:65:C9:DC:DD:64:20:1F:91:A1:D4:DA:DF:D7:27:BE:3E:AA:20:EE:33:AE:BF\r\nm=video 9000 RTP/SAVPF 126\r\na=rtpmap:126 H264/90000\r\na=rtcp-fb:126 nack pli\r\na=rtcp-fb:126 ccm fir\r\na=setup:passive\r\na=rtcp-mux\r\na=sendonly\r\na=candidate:828646434 1 udp 2130706431 10.224.196.188 9000 typ host generation 0\r\na=candidate:828646434 2 udp 2130706431 10.224.196.188 9000 typ host generation 0\r\na=candidate:828646435 1 tcp 2113937151 10.224.196.188 80 typ host generation 0\r\na=candidate:828646435 2 tcp 2113937151 10.224.196.188 80 typ host generation 0\r\na=ice-ufrag:807410045#4286640190#0\r\na=ice-pwd:xDfGAlWZJq7vnqPkzoP2NAMY\r\na=fmtp:126 profile-level-id=42e01f;level-asymmetry-allowed=1;packetization-mode=1\r\na=ssrc:807410045 cname:/pTzPEmQ7zEousgf\r\n","seq":1,"sessionToken":"0:0:4286640190:0

6. Got connection failed from onsignalingstatechange event callback: 

RTCPeerConnection.gotSignalingStateChange: peerId 0 signalingState: stable iceConnectionState= failed iceGatheringState= gathering


Actual results:

Firefox webrtc always meet ICE failed error on one pc


Expected results:

1. Got ICE success from oniceconnectionstatechange event callback
2. Got connection success from onsignalingstatechange event callback

Comment 1

[yanfzhen]

Attachment: aboutWebrtc.html

Comment 2

[Nils Ohlmeier [:drno]]

Thank you for attaching the log file. Unfortunately it seems like it contains a lot of attempts to get a connection working with lots of different error messages. Could you please restart your Firefox, try to connect to your service and then attach the log file from that one single test?

My current guess is that on the ICE layer we don't get proper responses from your implementation and therefore the connection simply times out.

Two hints:
- Connection state is never going to reach "success". It only reaches "stable" after offer answer has been completed. That is only based on calling setLocalDescription() and setRemoteDescription(). It does not reflect any network state.
- It looks like your implementation is not doing trickle ICE, it includes its ICE candidates in its SDP answer. Are you trickling the Firefox ICE candidates to your implementation? Because the offer you include here does not contain any ICE candidates, probably because it is the initial offer. If you pass that offer on to your implementation you need to trickle the ICE candidates to your implementation. Or you need to wait for gathering on the Firefox side to finish and then take the updated version of the offer and send that to your implementation.

Comment 3

[yanfzhen]

Attachment: aboutWebrtc-new.html

Comment 4

[Nils Ohlmeier [:drno]]

Sorry is that new log file really from a single call attempt?
Because it seems to contain again a lot of PeerConnections. When I said "restart Firefox" before I was hoping that you would start with an empty 'about:webrtc' page, make a single call attempt and then attach that result here. I'm not sure if the restart did not work, or if Firefox re-opened a lot of tabs with PeerConnections in it. Can you please make sure you have only a single Firefox window/tab open which is trying to connect to your service when creating a log file?

Comment 5

[yanfzhen]

Attachment: aboutWebrtc_single.html

We are from cisco webex team, we are developing the webex html5 client, so
this is a client to server webrtc application, our audio video server has
a public IP, so we use lite ICE instead of trickle ICE, we put the ICE
candidates into answer SDP

Comment 6

[Nils Ohlmeier [:drno]]

Thanks for the new log file that helps.

Sure I get that your implementation is an ICE lite. And putting the ICE candidates into the SDP works fine. My point is that hopefully you are aware that Firefox will not put its ICE candidates into its SDP. So you need to take care of transporting the ICE candidates from Firefox to your server.

A couple of observations from the log:
- Firefox fails to parse your ICE TCP candidates, I think because they are missing the 'tcptype' parameter.
- If you do ICE lite it would/could help to advertise that in the SDP.
- Why are you sending back an ICE candidate for a second component (with an identical port number?), although there is only a single video m-line, and you agree on doing rtcp-mux? This should result in only one component being used.
- It looks to me like the real problem seems to be that Firefox fails to create any candidates on your system according to these log lines:
  failed to create UDP candidates with error 6
  failed to create TCP candidates with error 6
Just from that message I can't say why it fails. To better understand what is going wrong here I would like to ask you to create a more detailed log file for ICE like described here https://wiki.mozilla.org/Media/WebRTC/Logging#ICE.2FSTUN.2FTURN and attach it here or send to me via email.
My only two ideas without a log file would be:
A) do you provide a STUN and/or TURN server? If so what and how do you set that server?
B) does your network require a HTTPS proxy?

Comment 7

[Nils Ohlmeier [:drno]]

Another idea to find out what is going wrong here is: could you please go to https://mozilla.github.io/webrtc-landing/pc_test.html and check if you can get a call working on your machine?
That should tell us if this is a problem specific to your machine or to your service.

Comment 8

[Nils Ohlmeier [:drno]]

We actually see the same error in Loop client error logs. It would be super helpful to get more debug logs for this case.

Comment 9

[yanfzhen]

Attachment: aboutWebrtc_for your test page.html

The attachment is the log that we try your:https://mozilla.github.io/webrtc-landing/pc_test.html on the PC, pls check.

Comment 10

[yanfzhen]

We open AppRTC in this machine and cannot get local media stream, here is
the link we use:
https://apprtc.appspot.com/?r=82313387&hd=true&stereo=true&debug=loopback

Seems like getUserMedia doesn¹t work on this machine, it is not a 100%
issue, but is 100% on this machine.

Comment 11

[Nils Ohlmeier [:drno]]

The fact that is also does not work with pc_test.html tells me that no STUN or TURN server is involved. And as ICE TCP is still disabled on your machine according to the log this leaves us in case of the nr_ice_component_initialize_tcp() https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_component.c#394 only with nICEr registry calls, which kind of makes sense as it would fail UDP and TCP.
The strange part is that according to the log setting the registry works. But then it fails on one of the get calls.

Comment 12

[Nils Ohlmeier [:drno]]

If my guess is right that this error happens when reading from the nICEr registry, then this seems to be the most plausible point of failure: https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nrappkit/src/registry/registry_local.c#777
Basically the only place where lots of R_BAD_ARGS get returned when trying to read from the registry.

Comment 13

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: Bug 1229633: hash interface names on Windows. r=ekr

Bug 1229633: turn on ABORT logging for debugging

Comment 14

[Byron Campen [:bwc]]

(In reply to Nils Ohlmeier [:drno] from comment #13)
> Created attachment 8696987 [details]
> MozReview Request: Bug 1229633: turn on ABORT logging for debugging
> 
> Bug 1229633: turn on ABORT logging for debugging

   Just note that the extra logging that this gives us is a straight-up fprintf, so it won't show up in about:webrtc. You will need to run the browser from the command line.

Comment 15

[Nils Ohlmeier [:drno]]

I created a special build which has logging to the console enabled to allow us to understand what is failing here. As Byron pointed out that you need to start that Firefox from the command line to capture the new extra output.

This is the link to the Windows installer: http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-7151f4c320df95921792fe4d074cc41928094f73/try-win32/firefox-45.0a1.en-US.win32.installer.exe
But I think the installer would overwrite your default Firefox on that machine. In this directory you can also find a ZIP file instead, which should allow you to just run the Firefox binary from that ZIP without having to install it first: http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-7151f4c320df95921792fe4d074cc41928094f73/try-win32/

It would be really great if you could get the ZIP, start Firefox from the command line, open the pc_test.html from above and send us or attach the output from that Firefox.

Thanks in advance!

Comment 16

[yanfzhen]

I cannot see any log output on the cmd output. Did I need input special command?

Comment 17

[Byron Campen [:bwc]]

You should not need anything special at all, since this logging just uses fprintf(stderr,...) Are you using the windows command prompt, or cygwin? Are you redirecting to a file?

Comment 18

[yanfzhen]

Attachment: output in zip.txt

Comment 19

[yanfzhen]

Attachment: aboutWebrtczip.html

Comment 20

[Nils Ohlmeier [:drno]]

So when I run the MacOS version of this build and start it manually from the command line like this:
  ./firefox
and then navigate to the pc_test.html page I see on the command line after starting the call lots if lines like this:

ABORT: error 2 at /builds/slave/try-m64-d-00000000000000000000/build/src/media/mtransport/third_party/nrappkit/src/registry/registry_local.c:345 (function nr_reg_fetch_node)
ABORT: error 2 at /builds/slave/try-m64-d-00000000000000000000/build/src/media/mtransport/third_party/nrappkit/src/registry/registry_local.c:345 (function nr_reg_fetch_node)
ABORT: error 2 at /builds/slave/try-m64-d-00000000000000000000/build/src/media/mtransport/third_party/nrappkit/src/registry/registry_local.c:592 (function nr_reg_get_array)
ABORT: error 2 at /builds/slave/try-m64-d-00000000000000000000/build/src/media/mtransport/third_party/nrappkit/src/log/r_log.c:249 (function r_log_get_reg_level)

That is what we are looking for, except that on your machine you should have a line with 'ABORT: error 6 at....' somewhere.

I'll double check that Windows build works on one of my machines some time this week.

Comment 21

[Nils Ohlmeier [:drno]]

Interesting... or outstanding how the author of this code would probably put it: the windows build in fact does not print anything to stdout.

I'll have to investigate how to get it to write something onto the command line.

Comment 22

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

(In reply to Nils Ohlmeier [:drno] from comment #21)
> Interesting... or outstanding how the author of this code would probably put
> it: the windows build in fact does not print anything to stdout.
> 
> I'll have to investigate how to get it to write something onto the command
> line.

e10s profiles/builds can't write to the cmd window from Content processes IIRC; write to a file (NSPR_LOG_FILE/etc)

Comment 23

[Nils Ohlmeier [:drno]]

Luckily there is an easy solution to the missing output on Windows problem: the windows debug build actually prints messages to the console.

Could you please try again with this debug build: http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-7151f4c320df95921792fe4d074cc41928094f73/try-win32-debug/firefox-45.0a1.en-US.win32.zip

Sorry for not testing it myself before hand. And thanks again for the support.

Comment 24

[yanfzhen]

Attachment: firefox dev cmd output.txt

Pls check the output which print in cmd, thanks.

Comment 25

[Nils Ohlmeier [:drno]]

Great that is the output we are looking for. Unfortunately it is missing the expect error message. I'm guessing it scrolled out of view. Could you please re-try with something like:
  firefox > log.txt 2>&1
and attach the resulting log.txt file here?

Comment 26

[yanfzhen]

Attachment: log.txt

use write to log.txt to save cmd console log, pls check.

Comment 27

[Byron Campen [:bwc]]

Not seeing any ABORT with error 6 in there. Maybe we did not hit the bug on that run?

Comment 28

[Nils Ohlmeier [:drno]]

My understanding is that ICE gathering always fails on the machine yanfzhen is using, correct?

So if the log was actually taken from a failed run, then I guess the next attempt is to ask for full nICEr log files. Could you please set the environment variables listed under "ICE/STUN/TURN" on this page https://wiki.mozilla.org/Media/WebRTC/Logging#ICE.2FSTUN.2FTURN and try again?
Please R_LOG_LEVEL to 9 instead of 3!

Ideally you should use the Firefox binary you were using last, and the same way to start it from command line and redirect the output to a log file like you did last time.

Comment 29

[yanfzhen]

Attachment: log1217.txt

change the R_LOG_LEVEL to 9, pls check the log

Comment 30

[Nils Ohlmeier [:drno]]

Byron was right this time the log actually contains the expected 6.

So from looking around in the code it looks to me like what is triggering this are these calls here:
  https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_component.c#213
and here:
  https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_component.c#429

Now the ABORT seems to suggest that some network interface name comes up as empty. Could you attach the output of 'ipconfig' on your system? Feel free to send it via email if you prefer to not attach it publicly here.

Comment 31

[Byron Campen [:bwc]]

So the error is in here:

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nrappkit/src/registry/registry.c#561

There's a couple things that could cause this:

- Interface names that are too long
- Interface names that start or end with '.' (we sanitize that here: https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/addrs.c?case=true&from=addrs.c#199)

I suspect that one of your interfaces has a longer name than nICEr can deal with.

Comment 32

[Nils Ohlmeier [:drno]]

To me it looks like systems which runs into this error have network interfaces with an empty friendly name. Which makes me wonder if we should check for the length of friendly name when we are going through the list, and try ask for another identifier in case the friendly name is empty.
The other possibility I see is that this is somehow related to GetAdaptersInfo bug, but instead of failing it just reports empty values.

Comment 33

[Nils Ohlmeier [:drno]]

Hmm nICEr's interface name length should be identical to the length of the temporary buffer on Windows:
https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/net/transport_addr.h#68
And if that is longer then what the registry can handle it should have bailed here:
https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nrappkit/src/registry/registry.c#578

Comment 34

[Nils Ohlmeier [:drno]]

I was able to reproduce the problem on my Win 7 laptop by giving the active network connection a really long name.

Comment 35

[Nils Ohlmeier [:drno]]

Darn it: I got the same error message different error though. By using a super long network interface name I hit the about in line 578 as expected: https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nrappkit/src/registry/registry.c#578

If I reduce the interface name to 100 characters everything just works fine. I'll look into fixing that interface name length issue.

But it would be still super helpful to get your 'ipconfig' output to help us understand why your machine is failing at a different place.

Comment 36

[Nils Ohlmeier [:drno]]

I was able to hit the same error, when I queried the |AdapterName| instead of the |FriendlyName| which turned out to be empty.
It appears that we could use |Description| in case |FriendlyName| should be empty. As we seem to use the name only for prioritizing and checking a bunch of registry values which Firefox never sets I assume we don't really have to use |FriendlyName|.

Reference for the different values I mentioned here: https://msdn.microsoft.com/en-us/library/windows/desktop/aa366058%28v=vs.85%29.aspx

Regarding limiting the length: it look we would want to query for a maximum of max size if the registry minus the length of the prefix. Now we could simply hard code that remaining length after manually calculating it once. Because at run time when we query the interfaces we don't know yet about the prefixes used in the registry.
I guess the other option is to just play safe and limit to 16 characters like it is limited on all other platforms. I assume the only problem with that is that the first 16 characters of |FriendlyName| might not be unique. So in that case we might have to add an index at the end to make them unique for us internally.

Comment 37

[yanfzhen]

Attachment: ipconfig.txt

Comment 38

[Nils Ohlmeier [:drno]]

Thank you, that was helpful.

Looks like we are using an ASCII function (snprintf) on a UTF-8 string (PWCHAR) here: https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/addrs.c#192
Not sure what the result will be, but I guess something strange. I'm not quite sure I get why this results in hitting:
https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nrappkit/src/registry/registry.c#595

But I think we have now some ideas how to improve this code.

Comment 39

[Nils Ohlmeier [:drno]]

Comment on attachment 8696987 [details]
MozReview Request: Bug 1229633: hash interface names on Windows. r=ekr

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

Comment 40

[Nils Ohlmeier [:drno]]

yanfzhen could you please try with this build: http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-9bc017becd72af5f282b5ef54465ba1845a5ce13/try-win32-debug/firefox-46.0a1.en-US.win32.zip

and let me know the result? Thanks in advance

Comment 41

[yanfzhen]

Hi,Nils

I check the new build 46.0a1.en on my PC, it works normally. Thanks for your quick responding.

Comment 42

[Nils Ohlmeier [:drno]]

Comment on attachment 8696987 [details]
MozReview Request: Bug 1229633: hash interface names on Windows. r=ekr

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

Comment 43

[Nils Ohlmeier [:drno]]

(In reply to yanfzhen from comment #41)
> Hi,Nils
> 
> I check the new build 46.0a1.en on my PC, it works normally. Thanks for your
> quick responding.

Awesome. Thank you very much for verifying it so quickly! Highly appreciated.

Comment 44

[Nils Ohlmeier [:drno]]

Comment on attachment 8696987 [details]
MozReview Request: Bug 1229633: hash interface names on Windows. r=ekr

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

Comment 45

[Eric Rescorla (:ekr)]

Comment on attachment 8696987 [details]
MozReview Request: Bug 1229633: hash interface names on Windows. r=ekr

https://reviewboard.mozilla.org/r/27479/#review25701

::: media/mtransport/third_party/nICEr/src/net/transport_addr.h:48
(Diff revision 3)
> -#define MAXIFNAME IFNAMSIZ
> +/* Length of astring  hex representation of a MD5 hash */
> +#define MAXIFNAME 2 * 16 + 1
>  #else
>  #define MAXIFNAME 16
>  #endif
>  

Is there a reason to have this not be 33 always? I have not checked.

This comment is kind of borked. s/astring /a/

::: media/mtransport/third_party/nICEr/src/stun/addrs.c:81
(Diff revision 3)
>  #if defined(WIN32)
>  
>  #define WIN32_MAX_NUM_INTERFACES  20
>  
> +#define _NR_MD5_HASH_LENGTH 16
> +/* String holding the HEX representation of the MD5 hash */
> +#define _NR_HEX_IF_LENGTH 2 * 16 + 1

Why not just reuse MAXIFNAME instead of NR_HEX_IF_LENGTH...

Also, why the leading underscores.

::: media/mtransport/third_party/nICEr/src/stun/addrs.c:200
(Diff revision 3)
> +        if(r=nr_crypto_md5((UCHAR *)tmpAddress->FriendlyName,
> +                           wcslen(tmpAddress->FriendlyName) * sizeof(wchar_t),
> +                           bin_hashed_ifname))
> +          ABORT(r);
> +        if(r=nr_bin2hex(bin_hashed_ifname, _NR_MD5_HASH_LENGTH,
> +          hex_hashed_ifname))
> +          ABORT(r);
> +        hex_hashed_ifname[_NR_HEX_IF_LENGTH - 1] = '\0';
> +

1. Use sizeof() not _NR_MD5...
2. nr_bin2hex already null terminates

::: media/mtransport/third_party/nICEr/src/stun/addrs.c:215
(Diff revision 3)
>                  ABORT(r);
>            }
>            else {
> -            r_log(NR_LOG_STUN, LOG_DEBUG, "Unrecognized sa_family for adapteraddress %s",munged_ifname);
> +            r_log(NR_LOG_STUN, LOG_DEBUG, "Unrecognized sa_family for adapteraddress %s",hex_hashed_ifname);
>              continue;
>            }
>  

It would be better if you were printing the actual name, not the hashed name

::: media/mtransport/third_party/nrappkit/nrappkit.gyp:26
(Diff revision 3)
>  
>            'sources' : [
>                # Shared
> -#              './src/share/nr_api.h',
> +              #'./src/share/nr_api.h',
>                './src/share/nr_common.h',
> -#              './src/share/nr_dynlib.h',
> +              #'./src/share/nr_dynlib.h',
>                './src/share/nr_reg_keys.h',
> -#              './src/share/nr_startup.c',
> -#              './src/share/nr_startup.h',
> -#              './src/share/nrappkit_static_plugins.c',
> +              #'./src/share/nr_startup.c',
> +              #'./src/share/nr_startup.h',
> +              #'./src/share/nrappkit_static_plugins.c',
>                 './src/port/generic/include'
>  
>                # libekr
>                './src/util/libekr/assoc.h',
>                './src/util/libekr/debug.c',
>                './src/util/libekr/debug.h',
>                './src/util/libekr/r_assoc.c',

This all seems unnecessary.

::: media/mtransport/third_party/nrappkit/nrappkit.gyp:124
(Diff revision 3)
>  
>                # Statistics
>                #'./src/stats/nrstats.c',
>                #'./src/stats/nrstats.h',
>                #'./src/stats/nrstats_app.c',
>                #'./src/stats/nrstats_int.h',
>                #'./src/stats/nrstats_memory.c',
>            ],
> -          
> +
>            'defines' : [
>                'SANITY_CHECKS',
>                'R_PLATFORM_INT_TYPES=<stdint.h>',
>                'R_DEFINED_INT2=int16_t',
>                'R_DEFINED_UINT2=uint16_t',
>                'R_DEFINED_INT4=int32_t',
> -              'R_DEFINED_UINT4=uint32_t',
> +             'R_DEFINED_UINT4=uint32_t',
> -              'R_DEFINED_INT8=int64_t',
> +             'R_DEFINED_INT8=int64_t',
> -              'R_DEFINED_UINT8=uint64_t',
> +             'R_DEFINED_UINT8=uint64_t',
>            ],
> -          
> +
>            'conditions' : [
>                ## Mac and BSDs
>                [ 'OS == "mac"', {
>                  'defines' : [
>                      'DARWIN',
>                  ],
>                }],
>                [ 'os_bsd == 1', {
>                  'defines' : [
>                      'BSD',
>                  ],
>                }],
>                [ 'OS == "mac" or OS == "ios" or os_bsd == 1', {
>                  'cflags_mozilla': [
>                      '-Wall',
>                      '-Wno-parentheses',
>                      '-Wno-strict-prototypes',
>                      '-Wmissing-prototypes',
>                   ],
>                   'defines' : [
>                       'HAVE_LIBM=1',
>                       'HAVE_STRDUP=1',
>                       'HAVE_STRLCPY=1',
>                       'HAVE_SYS_TIME_H=1',
>                       'HAVE_VFPRINTF=1',
>                       'NEW_STDIO'
>                       'RETSIGTYPE=void',
>                       'TIME_WITH_SYS_TIME_H=1',
>                       '__UNUSED__=__attribute__((unused))',
>                   ],
>  
>                   'include_dirs': [
>                       'src/port/darwin/include'
>                   ],
> -		 
> +
>                   'sources': [
> -                      './src/port/darwin/include/csi_platform.h',
> +                     './src/port/darwin/include/csi_platform.h',
>                   ],
>                }],
> -              
> +
>                ## Win
>                [ 'OS == "win"', {
>                   'defines' : [
>                       'WIN',
>                       '__UNUSED__=',
>                       'HAVE_STRDUP=1',

All this too. Please keep whitespace changes to a minimum

::: media/mtransport/third_party/nrappkit/src/registry/registry.c:576
(Diff revision 3)
>      clen = strlen(child);
>      if ((plen + clen + 2) > sizeof(NR_registry))
>          ABORT(R_BAD_ARGS);
> +    if (clen == 0)
> +        ABORT(R_BAD_ARGS);
>  

This test should come first. Also, !clen

::: media/mtransport/third_party/nrappkit/src/registry/registry.c:592
(Diff revision 3)
>      for (i = 0; i < clen; ++i, ++c) {
>          *c = child[i];
>          if (isspace(*c) || *c == '.' || *c == '/' || ! isprint(*c))
>              *c = '_';
>      }
> -    if (i == 0 || child[i-1] == '.')
> -        ABORT(R_BAD_ARGS);
>  
>      *c = '\0';
>  

Uh, why did you remove the trailing dot test?

Is it no longer possible to have it invoked?

Comment 46

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/27479/#review25701

> Why not just reuse MAXIFNAME instead of NR_HEX_IF_LENGTH...
> 
> Also, why the leading underscores.

I was just following the examples two lines below which start (for unknown reasons) with underscores.

> This all seems unnecessary.

When I enabled the RTRACE option in this file I discovered that indentation in this file is a complete mess. So I thought I better clean it up to leave the place in better condition then I found it. But it is not needed for this bug.

> Uh, why did you remove the trailing dot test?
> 
> Is it no longer possible to have it invoked?

The test is and was kind of pointless IMHO, because if you look in lines 592 and 593 (original file) the if condition checks for any dot in the string and replaces it with and underscore. Only to then later return a failure if the child ends with a dot (even though that got replaced with an underscore already).
So as we do this replacement I think we can remove that test.

Comment 47

[Nils Ohlmeier [:drno]]

Comment on attachment 8696987 [details]
MozReview Request: Bug 1229633: hash interface names on Windows. r=ekr

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

Comment 48

[Eric Rescorla (:ekr)]

Comment on attachment 8696987 [details]
MozReview Request: Bug 1229633: hash interface names on Windows. r=ekr

https://reviewboard.mozilla.org/r/27479/#review25791

::: media/mtransport/third_party/nICEr/src/stun/addrs.c:212
(Diff revisions 3 - 4)
>                  ABORT(r);
>            }
>            else {
> -            r_log(NR_LOG_STUN, LOG_DEBUG, "Unrecognized sa_family for adapteraddress %s",hex_hashed_ifname);
> +            r_log(NR_LOG_STUN, LOG_DEBUG, "Unrecognized sa_family for adapteraddress %s", tmpAddress->FriendlyName);
>              continue;
>            }

Don't you need a string modifier since this isn't a cstring?

::: media/mtransport/third_party/nrappkit/nrappkit.gyp:26
(Diff revision 3)
>  
>            'sources' : [
>                # Shared
> -#              './src/share/nr_api.h',
> +              #'./src/share/nr_api.h',
>                './src/share/nr_common.h',
> -#              './src/share/nr_dynlib.h',
> +              #'./src/share/nr_dynlib.h',
>                './src/share/nr_reg_keys.h',
> -#              './src/share/nr_startup.c',
> -#              './src/share/nr_startup.h',
> -#              './src/share/nrappkit_static_plugins.c',
> +              #'./src/share/nr_startup.c',
> +              #'./src/share/nr_startup.h',
> +              #'./src/share/nrappkit_static_plugins.c',
>                 './src/port/generic/include'
>  
>                # libekr
>                './src/util/libekr/assoc.h',
>                './src/util/libekr/debug.c',
>                './src/util/libekr/debug.h',
>                './src/util/libekr/r_assoc.c',

Please file a new bug then.

Comment 49

[Nils Ohlmeier [:drno]]

Comment on attachment 8696987 [details]
MozReview Request: Bug 1229633: hash interface names on Windows. r=ekr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27479/diff/4-5/

Comment 50

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/27479/#review25791

> Don't you need a string modifier since this isn't a cstring?

As this gets passed to snprintf() eventually I think using %S for a wide string should be enough.

Comment 51

[Nils Ohlmeier [:drno]]

Comment on attachment 8696987 [details]
MozReview Request: Bug 1229633: hash interface names on Windows. r=ekr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27479/diff/5-6/

Comment 52

[Nils Ohlmeier [:drno]]

For the record the try run with tests looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd4090ff801e (I only changed an error message after that which is not by any of our tests).

Comment 53

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c98d100b4ca

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c98d100b4ca