On my Linux box prnetdb.c was not correctly detecting that getaddrinfo was available and was always using pr_GetAddrInfoByNameFB. I did a bit of digging and found that the ifdefs (and comments about _PR_HAVE_GETADDRINFO) were out of sync with reality. Also, the use of _pr_ipv6_is_present() was used when what really needed to be tested was whether or not getaddrinfo was available. The attached patch fixes the ifdefs used to determine whether or not to include the _pr_find_getaddrinfo() probe. They now match (though not in the same order), the ifdefs around the _pr_find_getipnodebyname() probe. It also adds a static boolean, _pr_use_fallback, which is independent of whether or not ipv6 is enabled/available. Finally, the code/ifdefs which determine whether or not to use the fallback code in various support routines is updated to use _pr_use_fallback, and simplified to reduce duplicated code. This patch is against mozilla-central as of today.

updated patch after testing on mingw32 and OS X. - simplify ifdefs around getaddrinfo detection. - only define/check _pr_use_fallback if we might need it - add _pr_netdb_init for one-time probe of ipv6/getaddrinfo

Hi Robert! Thanks for contributing your first patch! I took a look at this today since I've been working on cleaning up some DNS bugs, and I have some comments: -- As you say, the code comment for _PR_HAVE_GETADDRINFO doesn't seem to be in sync with the code - from what I see, it is defined if getaddrinfo IS or MIGHT BE available, i.e. it should not be defined if it is known at build time that a probe will fail. Thus, it makes sense to that this code comment should be updated, and the rest of the code verified based on that. -- Just to make sure the basic case is covered ... Can you confirm if this was a prebuilt binary from Mozilla or your Linux distro, or did you build it yourself? If it was a prebuilt binary, can you try building it locally and determine what prnetdb.c does, please? It may be that the prebuilt binary uses the fallback option as a catch-all default (though I'd need to confirm that). -- Re the patch, I am concerned that the complexity of variables is increased by adding #define PRNETDB_CHECK_FALLBACK and _pr_use_fallback. Have you tried just replacing the calls to _pr_ipv6_is_present() with a call to a new function for getaddrinfo, for example? Something like: int _pr_getaddrinfo_present = PR_FALSE; int _pr_getaddrinfo_probed = PR_FALSE; int _pr_is_getaddrinfo_present() { if (!_pr_getaddrinfo_probed) _pr_getaddrinfo_present = ... // carry out probe return _pr_getaddrinfo_present } It's also possible that there may have been reasons in the past to include all the checks in _pr_ipv6_is_present(), but I'm not sure about those. -- Finally, apart from not calling a function that is already there, do you observe any missing functionality or performance issues using the fallback? Since this is older, relatively stable code, which could affect all platforms, we would need to confirm the patch on all supported platforms. Thanks again for contributing!

Hi Steve, thanks for looking at my patch. -- This was/is based on mozilla-central. I will work on a small tweak to add a self-test and print the results. I can test on Linux (F16), Mac OS X (10.umm, 8, i think), cygwin and mingw. > -- Just to make sure the basic case is covered ... Can you confirm if this was a prebuilt binary from Mozilla or your Linux distro, or did you build it yourself? If it was a prebuilt binary, can you try building it locally and determine what prnetdb.c does, please? It may be that the prebuilt binary uses the fallback option as a catch-all default (though I'd need to confirm that). Obviously I'll do what you guys want, but there are a few reasons I added PRNETDB_CHECK_FALLBACK and _pr_use_fallback. * The macro lets us completely exclude the code for plaforms that don't need it, and IMHO it is much easier to look at the code and understand what is going on. When I started looking at it, I had no idea why fallback was related to _pr_ipv6_is_present(). It wasn't until I checked the code carefully that I could reassure myself that it was just because _pr_ipv6_is_present() is the code that initiates the probe. So I think that the macro actually *reduces* complexity. * The static bool has two benefits.. ** it serves as a marker/documentation in the code for what we expect to be the case wrt probing. Trying to follow all the ifdefs is a major headache, and it helped me remember what was going on when I had to look at the code again after I'd not been working with it for a while. ** it just doesn't make sense to me to continually have the overhead of a function call for a check that we essentially only need to perform once. This is the same is _pr_netdb_initialized for _pr_netdb_init(). > It's also possible that there may have been reasons in the past to include all the checks in _pr_ipv6_is_present() I'm not sure what your concern is here. At any rate, _pr_ipv6_is_present uses PR_CallOnce to ensure it is only called once, so clearly we shouldn't need to call it more than once either. > -- Finally, apart from not calling a function that is already there, do you observe any missing functionality or performance issues using the fallback? No, but I wasn't looking at performance. The browser seemed to function normaly in my simple tests (visit a few web sites). > Since this is older, relatively stable code, which could affect all platforms, we would need to confirm the patch on all supported platforms. Yep. I'll test it on the 4 platforms I mentioned earlier. I'm glad to help with any issues that arise on other platforms.

> Yep. I'll test it on the 4 platforms I mentioned earlier. I'm glad to help with any issues that arise on other platforms. I added this simple patch to log results: diff --git a/nsprpub/pr/src/misc/prnetdb.c b/nsprpub/pr/src/misc/prnetdb.c index 2a22ec9..66fd26f 100644 --- a/nsprpub/pr/src/misc/prnetdb.c +++ b/nsprpub/pr/src/misc/prnetdb.c @@ -2423,5 +2423,11 @@ static void _pr_netdb_init() /* make sure we've had a chance to probe for ipv6/getaddrinfo */ _pr_ipv6_is_present(); /* this will call _pr_find_getaddrinfo() */ _pr_use_fallback = (!_pr_getaddrinfo); + PR_LOG(_pr_io_lm, PR_LOG_DEBUG, + ("_pr_netdb_initialized, _pr_use_fallback=%d, _pr_ipv6_is_present=%d", + _pr_use_fallback, _pr_ipv6_is_present())); +#else + PR_LOG(_pr_io_lm, PR_LOG_DEBUG, + ("_pr_netdb_initialized, no fallback probes")); #endif } Then ran on a few platforms: * Linux F16 64bit: $ uname -a Linux f16 3.2.7-1.fc16.x86_64 #1 SMP Tue Feb 21 01:40:47 UTC 2012 x86_64 x86_64 x86_64 GNU/Linux $ NSPR_LOG_MODULES=io:5 ./pr/tests/getai www.mozilla.org -144095552[9a2f548]: _pr_netdb_initialized, no fallback probes * OS X: $ uname -a Darwin osx 10.8.0 Darwin Kernel Version 10.8.0: Tue Jun 7 16:33:36 PDT 2011; root:xnu-1504.15.3~1/RELEASE_I386 i386 $ NSPR_LOG_MODULES=io:5 ./getai www.mozilla.org -1597311680[104680]: _pr_netdb_initialized, no fallback probes * Fedora 16 mingw32 cross-compile, copied to XP box and run under cygwin $ uname -a CYGWIN_NT-5.1 xp 1.7.11(0.260/5/3) 2012-02-24 14:05 i686 Cygwin $ NSPR_LOG_MODULES=io:5 ./getai.exe www.cnn.com 0[3e4008]: _pr_netdb_initialized, _pr_use_fallback=0, _pr_ipv6_is_present=0 * MozBuild environment: $ uname -a MINGW32_NT-5.1 XP 1.0.11(0.46/3/2) 2009-05-23 19:33 i686 Msys $ NSPR_LOG_MODULES=io:5 ./getai.exe www.cnn.com 0[3b3668]: _pr_netdb_initialized, _pr_use_fallback=0, _pr_ipv6_is_present=0

Went back and tested the fallback case (by tweaking the names which are probed), and found a missing return in PR_FreeAddrInfo that was causing a double free. changes from v2: add missing return in fallback case log initialization results to _pr_io_lm