Status

defect
--
major
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: seinlin, Assigned: seinlin)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Resolver on gonk is not necessary now. It also causes gonk-kk build failed.
Posted patch bug-958010-fix.patch (obsolete) — Splinter Review
Hi Michael, Can you help me to have a look on this patch. I try on my build and it does work.
Assignee: nobody → kli
Attachment #8357837 - Flags: review?(mwu)
Blocks: gonk-kk
Seems to work on try. Maybe you need to clobber after making the change.
Comment on attachment 8357837 [details] [diff] [review]
bug-958010-fix.patch

Review of attachment 8357837 [details] [diff] [review]:
-----------------------------------------------------------------

::: moz.build
@@ -16,5 @@
>  
>  if not CONFIG['LIBXUL_SDK']:
>      add_tier_dir('base', ['mfbt'])
>  
> -    if CONFIG['MOZ_WIDGET_TOOLKIT'] in ('android', 'gonk'):

This can't be turned off on android - we're still supporting old android versions there, AFAIK.
Depends on: 694325
Posted patch bug-958010-fix-2.patch (obsolete) — Splinter Review
Hi Michael, I am not sure if it is reasonable? I try on nexus 5 it does work.
Attachment #8357837 - Attachment is obsolete: true
Attachment #8357837 - Flags: review?(mwu)
Attachment #8358176 - Flags: review?(mwu)
Comment on attachment 8358176 [details] [diff] [review]
bug-958010-fix-2.patch

I'll update a new patch later.
Attachment #8358176 - Flags: review?(mwu)
Hi Michael, 

I verified on gonk-jb build. After resolver is disabled DNS will not work.

I can ping hinet DNS server, but can't resolve any host.

---
root@mako:/ # ping www.google.com.tw
ping: unknown host www.google.com.tw
root@mako:/ # ping 168.95.1.1
PING 168.95.1.1 (168.95.1.1) 56(84) bytes of data.
64 bytes from 168.95.1.1: icmp_seq=1 ttl=248 time=23.2 ms
64 bytes from 168.95.1.1: icmp_seq=2 ttl=248 time=21.4 ms
---
Hi Michael,

As we discussed, the resolver can be disabled to wrap on gonk-kk version. After leaving the issue on gonk-jb to another bug if we want to disable on gonk-jb too.

Thanks.
Attachment #8359145 - Flags: review?(mwu)
s|After|Then|
Flags: needinfo?
Hi Mike,

In Gonk-KK version, FxOS can directly to use DNS resolver and no necessary to wrap it for using our internal one and actually this leads to build break.

So this patch will leave FxOS on Gonk-kk version to use resolver from external library.
Attachment #8359145 - Attachment is obsolete: true
Attachment #8359145 - Flags: review?(mwu)
Attachment #8359151 - Flags: review?(mh+mozilla)
Flags: needinfo?
Replace to a correct patch without missing "fi".
Attachment #8359151 - Attachment is obsolete: true
Attachment #8359151 - Flags: review?(mh+mozilla)
Attachment #8359152 - Flags: review?(mh+mozilla)
Attachment #8358176 - Attachment is obsolete: true
Attachment #8359152 - Attachment is obsolete: true
Attachment #8359152 - Flags: review?(mh+mozilla)
Attachment #8359212 - Flags: review?(mh+mozilla)
Looks like fennec will break on 4.4 since it will have this enabled. Probably worth filing a bug for it to fix that.
mwu can you steal this review?
Flags: needinfo?(mwu)
Comment on attachment 8359212 [details] [diff] [review]
Disable wrap of DNS resolver on Gonk-KK only

Review of attachment 8359212 [details] [diff] [review]:
-----------------------------------------------------------------

::: moz.build
@@ +17,5 @@
>  if not CONFIG['LIBXUL_SDK']:
>      add_tier_dir('base', ['mfbt'])
>  
>      if CONFIG['MOZ_WIDGET_TOOLKIT'] in ('android', 'gonk'):
> +        if CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk' or CONFIG['ANDROID_VERSION'] <= '18':

it would be clearer if it was widget==android or (widget==gonk and version <= 18)
Attachment #8359212 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/8af7fbd99315
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Blocks: 961598
Flags: needinfo?(mwu)
What landed is neither what i reviewed or what i suggested. Which effectively disabled this dns code on android.
(In reply to Mike Hommey [:glandium] from comment #19)
> What landed is neither what i reviewed or what i suggested. Which
> effectively disabled this dns code on android.

Mike, John will include what you suggested in bug 961598.
You need to log in before you can comment on or make changes to this bug.