Closed
Bug 1229633
Opened 5 years ago
Closed 5 years ago
Firefox PeerConnection fails to gather on Win with "failed to create UDP candidates with error 6"
Categories
(Core :: WebRTC: Networking, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
Blocking Flags:
backlog | webrtc/webaudio+ |
People
(Reporter: yanfzhen, Assigned: drno)
References
Details
Attachments
(11 files)
109.21 KB,
text/html
|
Details | |
61.66 KB,
text/html
|
Details | |
11.56 KB,
text/html
|
Details | |
17.16 KB,
text/html
|
Details | |
40 bytes,
text/x-review-board-request
|
ekr
:
review+
|
Details |
4.20 KB,
text/plain
|
Details | |
14.47 KB,
text/html
|
Details | |
7.79 KB,
text/plain
|
Details | |
33.70 KB,
text/plain
|
Details | |
36.90 KB,
text/plain
|
Details | |
741 bytes,
text/plain
|
Details |
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
OS: Unspecified → Windows 7
Priority: -- → P2
Hardware: Unspecified → x86
Updated•5 years ago
|
Component: Untriaged → WebRTC: Networking
Priority: P2 → --
Product: Firefox → Core
Assignee | ||
Comment 2•5 years ago
|
||
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.
Assignee | ||
Comment 4•5 years ago
|
||
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?
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
Assignee | ||
Comment 6•5 years ago
|
||
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?
Assignee | ||
Comment 7•5 years ago
|
||
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.
Assignee | ||
Comment 8•5 years ago
|
||
We actually see the same error in Loop client error logs. It would be super helpful to get more debug logs for this case.
Status: UNCONFIRMED → NEW
backlog: --- → webrtc/webaudio+
Rank: 15
Ever confirmed: true
Priority: -- → P1
Summary: Firefox webrtc always meet ICE failed error on one pc → Firefox PeerConnection fails to gather on Win with "failed to create UDP candidates with error 6"
The attachment is the log that we try your:https://mozilla.github.io/webrtc-landing/pc_test.html on the PC, pls check.
Reporter | ||
Comment 10•5 years ago
|
||
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.
Assignee | ||
Comment 11•5 years ago
|
||
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.
Assignee | ||
Comment 12•5 years ago
|
||
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.
Assignee | ||
Comment 13•5 years ago
|
||
Bug 1229633: turn on ABORT logging for debugging
Comment 14•5 years ago
|
||
(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.
Assignee | ||
Comment 15•5 years ago
|
||
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/bind-autoland@mozilla.com-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/bind-autoland@mozilla.com-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!
Reporter | ||
Comment 16•5 years ago
|
||
I cannot see any log output on the cmd output. Did I need input special command?
Comment 17•5 years ago
|
||
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?
Reporter | ||
Comment 18•5 years ago
|
||
Reporter | ||
Comment 19•5 years ago
|
||
Assignee | ||
Comment 20•5 years ago
|
||
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.
Assignee | ||
Comment 21•5 years ago
|
||
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•5 years ago
|
||
(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)
Assignee | ||
Comment 23•5 years ago
|
||
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/bind-autoland@mozilla.com-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.
Reporter | ||
Comment 24•5 years ago
|
||
Pls check the output which print in cmd, thanks.
Assignee | ||
Comment 25•5 years ago
|
||
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?
Reporter | ||
Comment 26•5 years ago
|
||
use write to log.txt to save cmd console log, pls check.
Comment 27•5 years ago
|
||
Not seeing any ABORT with error 6 in there. Maybe we did not hit the bug on that run?
Assignee | ||
Comment 28•5 years ago
|
||
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.
Reporter | ||
Comment 29•5 years ago
|
||
change the R_LOG_LEVEL to 9, pls check the log
Assignee | ||
Comment 30•5 years ago
|
||
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•5 years ago
|
||
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.
Assignee | ||
Comment 32•5 years ago
|
||
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.
Assignee | ||
Comment 33•5 years ago
|
||
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
Assignee | ||
Comment 34•5 years ago
|
||
I was able to reproduce the problem on my Win 7 laptop by giving the active network connection a really long name.
Assignee | ||
Comment 35•5 years ago
|
||
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.
Flags: needinfo?(yanfzhen)
Assignee | ||
Comment 36•5 years ago
|
||
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.
Reporter | ||
Comment 37•5 years ago
|
||
Flags: needinfo?(yanfzhen)
Assignee | ||
Comment 38•5 years ago
|
||
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.
Assignee | ||
Comment 39•5 years ago
|
||
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/
Attachment #8696987 -
Attachment description: MozReview Request: Bug 1229633: turn on ABORT logging for debugging → MozReview Request: Bug 1229633: hash interface names on Windows
Assignee | ||
Comment 40•5 years ago
|
||
yanfzhen could you please try with this build: http://archive.mozilla.org/pub/firefox/try-builds/drno@ohlmeier.org-9bc017becd72af5f282b5ef54465ba1845a5ce13/try-win32-debug/firefox-46.0a1.en-US.win32.zip and let me know the result? Thanks in advance
Flags: needinfo?(yanfzhen)
Reporter | ||
Comment 41•5 years ago
|
||
Hi,Nils I check the new build 46.0a1.en on my PC, it works normally. Thanks for your quick responding.
Flags: needinfo?(yanfzhen)
Assignee | ||
Comment 42•5 years ago
|
||
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/
Assignee | ||
Comment 43•5 years ago
|
||
(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.
Updated•5 years ago
|
Assignee: nobody → drno
Updated•5 years ago
|
Rank: 15 → 12
Assignee | ||
Comment 44•5 years ago
|
||
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/
Attachment #8696987 -
Flags: review?(ekr)
Comment 45•5 years ago
|
||
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?
Attachment #8696987 -
Flags: review?(ekr)
Assignee | ||
Comment 46•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Attachment #8696987 -
Attachment description: MozReview Request: Bug 1229633: hash interface names on Windows → MozReview Request: Bug 1229633: hash interface names on Windows. r=ekr
Attachment #8696987 -
Flags: review?(ekr)
Assignee | ||
Comment 47•5 years ago
|
||
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/
Updated•5 years ago
|
Attachment #8696987 -
Flags: review?(ekr) → review+
Comment 48•5 years ago
|
||
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.
Assignee | ||
Comment 49•5 years ago
|
||
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/
Attachment #8696987 -
Attachment description: MozReview Request: Bug 1229633: hash interface names on Windows. r=ekr → MozReview Request: Bug 1229633: hash interface names on Windows. r?ekr
Assignee | ||
Comment 50•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Attachment #8696987 -
Attachment description: MozReview Request: Bug 1229633: hash interface names on Windows. r?ekr → MozReview Request: Bug 1229633: hash interface names on Windows. r=ekr
Assignee | ||
Comment 51•5 years ago
|
||
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/
Assignee | ||
Comment 52•5 years ago
|
||
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 54•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6c98d100b4ca
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•