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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect)

Description

[Marco Chen [:mchen]]

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.

Comment 1

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: DNS-work-around-for-gonk-kk.patch

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

Comment 2

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: Fix DNS doesn't work in gonk-kk

Comment 3

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: Fix DNS doesn't work in gonk-kk

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

Comment 4

[Kai-Zhen Li [:seinlin][:kli]]

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

Comment 5

[Alan Huang]

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.)

Comment 6

[Michael Wu [: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.

Comment 7

[Michael Wu [: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.

Comment 8

[Marco Chen [:mchen]]

Looking for network team's help.

Hi Ken,

As we discussed, thanks for your help here in advance.

Comment 9

[Vincent Chang[:vchang][changyihsin]]

Hi John, can you help to check this issue ?

Comment 10

[Diego Wilson [:diego]]

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

Comment 11

[Diego Wilson [:diego]]

Hello jshih,

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

Comment 12

[Diego Wilson [:diego]]

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

Comment 13

[John Shih]

(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.

Comment 14

[Ken Chang[:kenkai]]

Set the target date.

Comment 15

[John Shih]

Attachment: Bug 961598 - Support DNS resolver on kk. r=vchang

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.

Comment 16

[John Shih]

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.

Comment 17

[Michael Wu [:mwu]]

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

Comment 18

[Diego Wilson [:diego]]

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 20

[Vincent Chang[:vchang][changyihsin]]

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.

Comment 21

[John Shih]

Attachment: Bug 961598 - Part1: Support DNS resolver on kk. r=vchang

patch revised for Comment 20.

Comment 22

[John Shih]

Attachment: Bug 961598 - Part2: Remove wrapped DNS resolver. r=mwu

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 23

[Mike Hommey [:glandium]]

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.

Comment 24

[Mike Hommey [:glandium]]

(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.

Comment 25

[John Shih]

(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

Comment 26

[John Shih]

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

Comment 27

[Michael Wu [:mwu]]

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

Comment 28

[John Shih]

(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.

Comment 29

[Mike Hommey [:glandium]]

Please fix the mess from bug 958010, too.

Comment 30

[John Shih]

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

Comment 31

[John Shih]

Attachment: Bug 961598 - Part1: Support DNS resolver on kk. r=vchang

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.

Comment 32

[John Shih]

Attachment: Bug 961598 - Part2: Disable wrapped DNS resolver from ICS. r=mwu, mh

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 33

[Mike Hommey [:glandium]]

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.

Comment 34

[John Shih]

Attachment: Bug 961598 - Part2: Disable wrapped DNS resolver from ICS. r=mwu, mh

Fix nits.

Comment 35

[Michael Wu [: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.

Comment 36

[John Shih]

Attachment: Bug 961598 - Part2: Disable wrapped DNS resolver from ICS. r=mwu, mh

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

Comment 37

[[:fabrice] Fabrice Desré]

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

Comment 38

[Kai-Chih Hu [:khu]]

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

Comment 39

[[:fabrice] Fabrice Desré]

(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

Comment 40

[Carsten Book [:Tomcat]]

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