Bug 958010 decided to disable wrapper of DNS resolver from bionic to gecko's own version. And Bionic's version on gonk-kk can't work yet.

This is a workaround for do not block other testing on gonk-kk porting.

Fix error hit on try server. Use strict condition check could be better.

Try result: https://tbpl.mozilla.org/?tree=Try&rev=a400cbb84324

Hi Michael, I heard that you suggest we should use bionic's getaddrinfo after android 3.0. When I was using bionic's getaddrinfo, I met connection refused error from getsockopt. I will look into this, but do you remember is this the reason we use our wrapped version before? > (gdb) bt > #0 retrying_select (sock=sock@entry=112, readset=readset@entry=0xae2eeeb0, writeset=writeset@entry=0x0, finish=finish@entry=0xae2eee18) at bionic/libc/netbsd/resolv/res_send.c:1028 > #1 0xb6ec4134 in send_dg (gotsomewhere=<synthetic pointer>, v_circuit=<synthetic pointer>, ns=0, terrno=<synthetic pointer>, anssiz=<optimized out>, ans=<optimized out>, buflen=32, > buf=0xae2ef61c "t\350\001", statp=0x1cc0534) at bionic/libc/netbsd/resolv/res_send.c:1145 > #2 __res_nsend (statp=statp@entry=0x1cc0534, buf=buf@entry=0xae2ef61c "t\350\001", buflen=32, ans=ans@entry=0xb0a9d008 "", anssiz=anssiz@entry=65536) > at bionic/libc/netbsd/resolv/res_send.c:573 > #3 0xb6ec5b6c in res_queryN (name=0xaaba45a0 "3.pool.ntp.org", target=target@entry=0xae2ffaa4, res=res@entry=0x1cc0534) at bionic/libc/netbsd/net/getaddrinfo.c:2206 > #4 0xb6ec5eea in res_querydomainN (name=name@entry=0xaaba45a0 "3.pool.ntp.org", domain=domain@entry=0x0, target=target@entry=0xae2ffaa4, res=res@entry=0x1cc0534) > at bionic/libc/netbsd/net/getaddrinfo.c:2450 > #5 0xb6ec6464 in res_searchN (res=0x1cc0534, target=0xae2ffaa4, name=0xaaba45a0 "3.pool.ntp.org") at bionic/libc/netbsd/net/getaddrinfo.c:2302 > #6 _dns_getaddrinfo (rv=0xae2ffbf0, cb_data=<optimized out>, ap=...) at bionic/libc/netbsd/net/getaddrinfo.c:1989 > #7 0xb6ec5990 in nsdispatch (retval=retval@entry=0xae2ffbf0, disp_tab=disp_tab@entry=0xb6ed9c58 <dtab.8449>, database=database@entry=0xb6ed270e "hosts", > method=method@entry=0xb6ed3735 "getaddrinfo", defaults=0xb6ed9c7c <default_dns_files>) at bionic/libc/netbsd/net/nsdispatch.c:142 > #8 0xb6ec6fe8 in explore_fqdn (mark=<optimized out>, iface=<optimized out>, res=0xae2ffc10, servname=0x0, hostname=<optimized out>, pai=0xae2ffc14) > at bionic/libc/netbsd/net/getaddrinfo.c:823 > #9 android_getaddrinfoforiface (hostname=hostname@entry=0xaaba45a0 "3.pool.ntp.org", servname=servname@entry=0x0, hints=hints@entry=0xae2ffd08, iface=iface@entry=0x0, mark=mark@entry=0, > res=res@entry=0xae2ffd04) at bionic/libc/netbsd/net/getaddrinfo.c:764 > #10 0xb6ec711a in getaddrinfo (hostname=hostname@entry=0xaaba45a0 "3.pool.ntp.org", servname=servname@entry=0x0, hints=hints@entry=0xae2ffd08, res=res@entry=0xae2ffd04) > at bionic/libc/netbsd/net/getaddrinfo.c:581 > #11 0xb6659c4a in PR_GetAddrInfoByName (hostname=0xaaba45a0 "3.pool.ntp.org", af=<optimized out>, flags=<optimized out>) at ../../../../../gecko/nsprpub/pr/src/misc/prnetdb.c:2047 > #12 0xb4ea0b6e in nsHostResolver::ThreadFunc (arg=0xb6a36ce0) at ../../../gecko/netwerk/dns/nsHostResolver.cpp:1163 > #13 0xb6662fd6 in _pt_root (arg=0xaabed600) at ../../../../../gecko/nsprpub/pr/src/pthreads/ptthread.c:205 > #14 0xb6e9d174 in __thread_entry (func=0xb6662f41 <_pt_root>, arg=0xaabed600, tls=0xae2ffdd0) at bionic/libc/bionic/pthread_create.cpp:105 > #15 0xb6e9d30c in pthread_create (thread_out=0xb3089a04, attr=<optimized out>, start_routine=0xb6662f41 <_pt_root>, arg=0x78) at bionic/libc/bionic/pthread_create.cpp:224 > #16 0x00000000 in ?? () > (gdb) list > 1023 return n; > 1024 } > 1025 if ((readset && FD_ISSET(sock, readset)) || (writeset && FD_ISSET(sock, writeset))) { > 1026 len = sizeof(error); > 1027 if (getsockopt(sock, SOL_SOCKET, SO_ERROR, &error, &len) < 0 || error) { > 1028 errno = error; > 1029 if (DBG) { > 1030 __libc_format_log(ANDROID_LOG_DEBUG, "libc", > 1031 " %d retrying_select dot error2 %d\n", sock, errno); > 1032 } > (gdb) p error > $1 = 111 (which means connection refused.)

The wrapped version was originally introduced to avoid multithreading problems in bionic's stdio implementation (which then caused issues in the resolver which used it). AFAIK, this was fixed before ICS, so we don't need the workaround.

Comment on attachment 8364845 [details] [diff] [review] Fix DNS doesn't work in gonk-kk We don't want to continue using our resolver fork.

Looking for network team's help. Hi Ken, As we discussed, thanks for your help here in advance.

Hi John, can you help to check this issue ?

On my QRD 8626 device I see this issue after applying the fix for 973832. FWIW attachment 8364845 [details] [diff] [review] doesn't work.

Hello jshih, This is a big blocker for the kitkat bring up activity. Are you the right person to be assigned here?

Change component to "Core -> Networking" to be inline with bug 694325

(In reply to Diego Wilson [:diego] from comment #11) > Hello jshih, > > This is a big blocker for the kitkat bring up activity. Are you the right > person to be assigned here? Yes, I'm working on it. It was blocked by a regression that the device cannot connect to wifi, which we finally solved yesterday. I'll fix this problem ASAP.

Set the target date.

Since JB, Android doesn't use properties anymore. The DNS is set through netd, which is also in charge of doing getaddrinfo. This patch supports the corresponding netd command (resolver setdefaultif, resolver setifdns) and send when setDNS() is called.

Hi Michael, After have a fix with this problem, it seems we don't need our wrapped version any more. Should we also remove all the wrapped code here or make it as a follow-up work? One for thing, do we still need to support gingerbread now? Since the wrapped version may be needed for it.

We don't need to support gingerbread, so I think we can get rid of the wrapping on gonk.

Comment on attachment 8379661 [details] [diff] [review] Bug 961598 - Support DNS resolver on kk. r=vchang Review of attachment 8379661 [details] [diff] [review]: ----------------------------------------------------------------- This patch works for me on QRD8x26

Comment on attachment 8379661 [details] [diff] [review] Bug 961598 - Support DNS resolver on kk. r=vchang Review of attachment 8379661 [details] [diff] [review]: ----------------------------------------------------------------- The patch looks good, thank you. But I think that you still need to remove the dns wrapper thing in the other patch and ask for review by mwu. Some notes for tracking purpose. It seems that Anroid trends to use netd to handle the DNS query request after Jellybean. Because the ANDROID_DNS_MODE environment variable is not set in calling process (B2G process), the dns query will be redirected to netd process. http://androidxref.com/4.4.2_r1/xref/bionic/libc/netbsd/net/getaddrinfo.c#735 The calling process has to wait there until getting the netd response. In netd process, android_getaddrinfoforiface() is used with ANDROID_DNS_MODE environment variable set to local. So the netd does the real dns query stuff.... and responses the query result via /dev/socket/dnsproxyd socket to calling process. We also need to use below two commands to set dns server ip. "resolver setdefaultif" "resolver setifdns" ::: dom/system/gonk/NetworkUtils.cpp @@ +902,5 @@ > + CommandCallback aCallback, > + NetworkResultOptions& aResult) > +{ > + postMessage(aChain->getParams(), aResult); > +} Please remove the dead code, if you are not using it.

patch revised for Comment 20.

Since we don't need the wrapped DNS resovler anymore, the code copied from bionic in Bug 694325 (which is placed in other-license/android) can thus be removed. The patch works on both ICS/KK by my testing locally. Also, have a push try (https://tbpl.mozilla.org/?tree=Try&rev=c085a78a51d9). mwu, please check if we really can remove these code. If there is any concern (such as causing regression on other platform), we can postpone this work and make it as a follow-up bug. Thanks

Comment on attachment 8380519 [details] [diff] [review] Bug 961598 - Part2: Remove wrapped DNS resolver. r=mwu This code is still used on android... well, it is supposed to still be used on android, but i just realized bug 958010 broke it.

(In reply to Mike Hommey [:glandium] from comment #23) > Comment on attachment 8380519 [details] [diff] [review] > Bug 961598 - Part2: Remove wrapped DNS resolver. r=mwu > > This code is still used on android... well, it is supposed to still be used > on android, but i just realized bug 958010 broke it. ... because what landed is not what i reviewed... sigh.

(In reply to Mike Hommey [:glandium] from comment #24) > (In reply to Mike Hommey [:glandium] from comment #23) > > Comment on attachment 8380519 [details] [diff] [review] > > Bug 961598 - Part2: Remove wrapped DNS resolver. r=mwu > > > > This code is still used on android... well, it is supposed to still be used > > on android, but i just realized bug 958010 broke it. > > ... because what landed is not what i reviewed... sigh. I'm guessing the behavior of calling getaddrinfo is same on both B2G and Fennec. So IMO, the main factor will be whether we need to support the version before ICS or not. As you mentioned, do you know the how Android use this code? or is there any specific reason that Fennec needs them? Thanks

mwu, Please see Comment 22. Do we still need our own wrapped DNS resolver? (B2G/Fennec) If so, I think we can just have the patch (the r+ one) landed directly without remove copied bionic code. Thanks

Yes. We need this as long as we support Gingerbread, which we still do.

(In reply to Michael Wu [:mwu] from comment #27) > Yes. We need this as long as we support Gingerbread, which we still do. Thanks for clarification. Let's have the patch (the r+ one) landed first to fix the KK blocker.

Please fix the mess from bug 958010, too.

Will re-ask for landing after fix the mess in bug 958010.

Note: the change actually started from JellyBean (4.3), which maps to SDK version 18. Previous JB version (4.1.1 ~ 4.2.2) still uses properties directly.

Since we don't need the wrapped DNS reslover in gonk anymore, disable it from now. In the same time, fennec will keep using the wrapped code regardless any version. Please let fennec guys to decide if they want to make any change.

Comment on attachment 8381178 [details] [diff] [review] Bug 961598 - Part2: Disable wrapped DNS resolver from ICS. r=mwu, mh Review of attachment 8381178 [details] [diff] [review]: ----------------------------------------------------------------- ::: moz.build @@ +25,5 @@ > add_tier_dir('base', ['mfbt']) > > if not CONFIG['JS_STANDALONE']: > if CONFIG['MOZ_WIDGET_TOOLKIT'] in ('android', 'gonk'): > + if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android' or (CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk' and CONFIG['ANDROID_VERSION'] <= '14:'): '14'. This test makes the if CONFIG['MOZ_WIDGET_TOOLKIT'] in ('android', 'gonk') test above useless. Please remove it.

Fix nits.

Comment on attachment 8381228 [details] [diff] [review] Bug 961598 - Part2: Disable wrapped DNS resolver from ICS. r=mwu, mh I think we should just turn this off for any version of gonk, since we're not supporting gingerbread.

Patch updated. Don't support wrapped DNS resolver anymore in gonk.

https://hg.mozilla.org/integration/b2g-inbound/rev/480d41979c72 https://hg.mozilla.org/integration/b2g-inbound/rev/62cef6077cb4

So, it looks like this bug could be marked as resolved fixed, right?

(In reply to Kevin Hu [:khu] from comment #38) > So, it looks like this bug could be marked as resolved fixed, right? No, only when this will be merged on mozilla-central

https://hg.mozilla.org/mozilla-central/rev/480d41979c72 https://hg.mozilla.org/mozilla-central/rev/62cef6077cb4