[Gonk-KK] The DNS Resolver from Bionic's netBSD is not workable on Nexus-5.

RESOLVED FIXED in Firefox 30

Status

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: mchen, Assigned: johnshih.bugs)

Tracking

unspecified
1.4 S2 (28feb)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.4+, firefox28 wontfix, firefox29 wontfix, firefox30 fixed, b2g-v1.4 fixed)

Details

(Whiteboard: [caf priority: p2][CR 613692])

Attachments

(3 attachments, 7 obsolete attachments)

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.
Depends on: 958010
Blocks: gonk-kk
Assignee: nobody → kli
This is a workaround for do not block other testing on gonk-kk porting.
Attachment #8362422 - Attachment is obsolete: true
Attachment #8363843 - Flags: review?(mh+mozilla)
Fix error hit on try server. Use strict condition check could be better.
Attachment #8363843 - Attachment is obsolete: true
Attachment #8363843 - Flags: review?(mh+mozilla)
Attachment #8364845 - Flags: review?(mh+mozilla)
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.)
Flags: needinfo?(mwu)
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.
Flags: needinfo?(mwu)
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.
Attachment #8364845 - Flags: review?(mh+mozilla) → review-
Looking for network team's help.

Hi Ken,

As we discussed, thanks for your help here in advance.
Assignee: kli → nobody
Hi John, can you help to check this issue ?
Assignee: nobody → jshih
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?
Flags: needinfo?(jshih)
Change component to "Core -> Networking" to be inline with bug 694325
Component: Wifi → Networking
Product: Firefox OS → Core
(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.
Flags: needinfo?(jshih)
Set the target date.
Component: Networking → GonkIntegration
Product: Core → Firefox OS
Target Milestone: --- → 1.4 S2 (28feb)
blocking-b2g: --- → 1.4+
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.
Attachment #8379661 - Flags: review?(vchang)
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.
Flags: needinfo?(mwu)
We don't need to support gingerbread, so I think we can get rid of the wrapping on gonk.
Flags: needinfo?(mwu)
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
Attachment #8379661 - Flags: feedback+
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.
Attachment #8379661 - Flags: review?(vchang) → review+
patch revised for Comment 20.
Attachment #8379661 - Attachment is obsolete: true
Attachment #8380477 - Flags: review+
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
Attachment #8380519 - Flags: review?(mwu)
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.
Attachment #8380519 - Flags: review?(mwu) → review-
(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
Flags: needinfo?(mwu)
Yes. We need this as long as we support Gingerbread, which we still do.
Flags: needinfo?(mwu)
(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.
Keywords: checkin-needed
Please fix the mess from bug 958010, too.
Will re-ask for landing after fix the mess in bug 958010.
Keywords: checkin-needed
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.
Attachment #8380477 - Attachment is obsolete: true
Attachment #8381065 - Flags: review+
Blocks: 976427
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.
Attachment #8380519 - Attachment is obsolete: true
Attachment #8381178 - Flags: review?(mwu)
Attachment #8381178 - Flags: review?(mh+mozilla)
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.
Attachment #8381178 - Flags: review?(mh+mozilla) → review+
Fix nits.
Attachment #8381178 - Attachment is obsolete: true
Attachment #8381178 - Flags: review?(mwu)
Attachment #8381228 - Flags: review?(mwu)
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.
Attachment #8381228 - Attachment is obsolete: true
Attachment #8381228 - Flags: review?(mwu)
Attachment #8381276 - Flags: review?(mwu)
Attachment #8381276 - Flags: review?(mwu) → review+
Keywords: checkin-needed
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
Whiteboard: [CR 613692]
Duplicate of this bug: 976973
Whiteboard: [CR 613692] → [caf priority: p2][CR 613692]
You need to log in before you can comment on or make changes to this bug.