Add per network interface dns queries and change dns cache; Needed for b2g

RESOLVED FIXED in Firefox 38

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dragana, Assigned: dragana)

Tracking

unspecified
mozilla38
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(1 attachment, 10 obsolete attachments)

(Assignee)

Description

4 years ago
Add per network interface dns query.
The dns cache needs to be extended as well.
Per network interface dns queries are possible with b2g.
(Assignee)

Updated

4 years ago
Assignee: nobody → dd.mozilla
(Assignee)

Updated

4 years ago
Blocks: 1053650
(Assignee)

Updated

4 years ago
No longer blocks: 1053650
(Assignee)

Comment 1

4 years ago
Posted patch fix v1 (obsolete) — Splinter Review
(Assignee)

Comment 2

4 years ago
Posted patch fix v1 (obsolete) — Splinter Review
A test added.
Attachment #8538793 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
Blocks: 1053650
Hi Dragana,

Good job! I tested the patch and it works nearly perfect unless
ANDROID_VERSION is not visible in prnetdb.c. Other than that,
the interface name could be carried to PR_Android_GetAddrInfoForNetInterface
to resolve. I think it's already good enough to me to do
the rest of work which is out of the scope of this bug.

Thanks :)
By the way, AOSP replaces android_getaddrinfoforiface with android_getaddrinfofornet
in 5.0 [1]. The network interface is described by a network id (integer) but not a string.
Work regarding new routing rule and network id creation is being addressed 
in bug 1104664. For AOSP 5.0 we can convert the string to an integer and
delegate to different function.

[1] http://androidxref.com/5.0.0_r2/xref/bionic/libc/dns/net/getaddrinfo.c#592
Flags: needinfo?(dd.mozilla)
Posted patch Bug1108957.patch (obsolete) — Splinter Review
Rebased and ignore ANDRIOD_VERSION check in prnetdb for testing.
(Assignee)

Comment 6

4 years ago
in which android version android_getaddrinfofornet will be available?

You probably know in what form is netID argument for this function? it is integer but what does it represent and how it is obtained? (I need this to decide how to store/transform it from string to integer and to decide if we need some checks)
Flags: needinfo?(dd.mozilla) → needinfo?(hchang)
Some information for Dragana:

1) The implementation of android_getaddrinfoforiface is in [1]
2) NetId is an integer obtained and maintained by gecko. It'll
   typically be between 100~65535. [2]

[1] http://androidxref.com/5.0.0_r2/xref/bionic/libc/dns/net/getaddrinfo.c#592
[2] http://androidxref.com/5.0.0_r2/xref/system/netd/server/NetworkController.cpp#49
Flags: needinfo?(hchang)
(Assignee)

Comment 8

4 years ago
Posted patch fix v2 (obsolete) — Splinter Review
Hi,

May I ask you to take a look at the nspr part of this patch (fix v2)? I have added the PR_Android_GetAddrInfoForNetInterface function that is wrapping android_getaddrinfoforiface and android_getaddrinfofornet. Is this something that can be a part of nspr?

If you do not want this to be a part of nspr, I could implement it in necko.
Attachment #8539116 - Attachment is obsolete: true
Attachment #8548072 - Flags: feedback?(wtc)
(Assignee)

Comment 9

4 years ago
May be you can give me feedback on my question from  comment 8.
Flags: needinfo?(ted)
I don't see a lot of value in putting this function in NSPR, really. Since this is an Android-only function and not a cross-platform function I think you'd be better served just putting that in necko. Let me know if there's some reason to prefer putting it in NSPR.
Flags: needinfo?(ted)
(Assignee)

Comment 11

4 years ago
Posted patch fix v3 (obsolete) — Splinter Review
Attachment #8548072 - Attachment is obsolete: true
Attachment #8548072 - Flags: feedback?(wtc)
(Assignee)

Comment 12

4 years ago
Can you try this one.
this is without changing nspr.
So if it is working I can mark it for review.
Thank you.
Flags: needinfo?(hchang)
(In reply to Dragana Damjanovic from comment #12)
> Can you try this one.
> this is without changing nspr.
> So if it is working I can mark it for review.
> Thank you.

No problem! I would give it a try when I get a chance.
Thanks!
Flags: needinfo?(hchang)
(In reply to Henry Chang [:henry] from comment #13)
> (In reply to Dragana Damjanovic from comment #12)
> > Can you try this one.
> > this is without changing nspr.
> > So if it is working I can mark it for review.
> > Thank you.
> 
> No problem! I would give it a try when I get a chance.
> Thanks!

The new patch is as good as before! Could you please mark it as review?
Thanks!
(Assignee)

Comment 15

4 years ago
Thanks for the test!
(Assignee)

Comment 16

4 years ago
Posted patch v4 (obsolete) — Splinter Review
Attachment #8552602 - Attachment is obsolete: true
Attachment #8555759 - Flags: review?(honzab.moz)
Dragana, can you update the patch to the latest m-c, it doesn't apply anymore.  I'd like to manually test the patch.
Comment on attachment 8555759 [details] [diff] [review]
v4

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

::: netwerk/dns/ChildDNSService.cpp
@@ +61,5 @@
>                                       nsACString &aHashKey)
>  {
>    aHashKey.Assign(aHost);
>    aHashKey.AppendInt(aFlags);
> +  aHashKey.Append(aNetworkInterface);

maybe only if (!aNetIface.Empty()) ?

@@ +76,5 @@
>                                nsIDNSListener    *listener,
>                                nsIEventTarget    *target_,
>                                nsICancelable    **result)
>  {
> +  return AsyncResolveExtended(hostname, flags, nsAutoCString(), listener,

EmptyCString()

@@ +152,5 @@
>                                      uint32_t           aFlags,
>                                      nsIDNSListener    *aListener,
>                                      nsresult           aReason)
>  {
> +  return CancelAsyncResolveExtended(aHostname, aFlags, nsAutoCString(),

EmptyCString()

::: netwerk/dns/GetAddrInfo.cpp
@@ +311,5 @@
>    }
>  
> +  PRAddrInfo* prai;
> +#if defined(ANDROID) && ANDROID_VERSION >= 19
> +  if (aNetworkInterface[0] != '\0') {

you should check |aNetworkInterface| itself too..

@@ +321,5 @@
> +    prai = PR_GetAddrInfoByName(aCanonHost, aAddressFamily, prFlags);
> +  }
> +#else
> +  prai = PR_GetAddrInfoByName(aCanonHost, aAddressFamily, prFlags);
> +#endif

can you somehow merge these two into just one line (if I don't miss anything)?

::: netwerk/dns/nsDNSService2.cpp
@@ +345,5 @@
>  nsDNSAsyncRequest::Cancel(nsresult reason)
>  {
>      NS_ENSURE_ARG(NS_FAILED(reason));
> +    mResolver->DetachCallback(mHost.get(), mFlags, mAF, mNetworkInterface.get(),
> +                              this, reason);

I can see the mHost is already passed down as zstring.  Do we have any clear/simple mechanisms that ensure this class (keeping the string alive) does overlive existing references to the string(s) from the nsHostResolver code?

if hard to answer, maybe file a bug we should do the audit.  This is critical.

@@ +673,5 @@
>                             nsIDNSListener    *listener,
>                             nsIEventTarget    *target_,
>                             nsICancelable    **result)
>  {
> +    return AsyncResolveExtended(aHostname, flags, nsAutoCString(), listener, target_,

EmptyCString()

@@ +795,5 @@
>  
>      uint16_t af = GetAFForLookup(hostname, aFlags);
>  
> +    res->CancelAsyncRequest(hostname.get(), aFlags, af,
> +                            nsCString(aNetworkInterface).get(), aListener,

what's this?  you force a copy of the string (not sure this is the way if that's what you want)?  why?

@@ +858,5 @@
>  
>      uint16_t af = GetAFForLookup(hostname, flags);
>  
> +    nsresult rv = res->ResolveHost(hostname.get(), flags, af,
> +                                   nsCString(aNetworkInterface).get(),

here as well.

::: netwerk/dns/nsHostResolver.cpp
@@ +178,5 @@
>      host = ((char *) this) + sizeof(nsHostRecord);
>      memcpy((char *) host, key->host, strlen(key->host) + 1);
>      flags = key->flags;
>      af = key->af;
> +    netInterface = ((char *) this) + sizeof(nsHostRecord) +

maybe use |host| here?

@@ +226,5 @@
>      // must call locked
> +    LOG(("Checking blacklist for host [%s]%s%s%s, host record [%p].\n", host,
> +         (netInterface[0] != '\0') ? " on interface [" : "",
> +         (netInterface[0] != '\0') ? netInterface : "",
> +         (netInterface[0] != '\0') ? "]" : "", this));

some #define LOG_NET_INTERFACE(ni) or even LOG_HOST(host, interface) might be useful instead (as you can see bellow..)

@@ +245,5 @@
> +            LOG(("Address [%s] is blacklisted for host [%s]%s%s%s.\n", buf,
> +                 (netInterface[0] != '\0') ? " on interface [" : "",
> +                 (netInterface[0] != '\0') ? netInterface : "",
> +                 (netInterface[0] != '\0') ? "]" : "",
> +                 host));

Args in bad order?

@@ +275,5 @@
> +             "[%s]%s%s%s.\n", buf,
> +             (netInterface[0] != '\0') ? " on interface [" : "",
> +             (netInterface[0] != '\0') ? netInterface : "",
> +             (netInterface[0] != '\0') ? "]" : "",
> +             host));

bad order

@@ +881,5 @@
> +                     IsMediumPriority(flags) ? "medium" : "low",
> +                     (netInterface[0] != '\0') ? " on interface [" : "",
> +                     (netInterface[0] != '\0') ? netInterface : "",
> +                     (netInterface[0] != '\0') ? "]" : "",
> +                     host));

bad order

@@ +1373,5 @@
>                  NS_RELEASE(head);
>              }
>  #if TTL_AVAILABLE
>              if (!rec->mGetTtl && !rec->resolving && sGetTtlEnabled) {
> +                LOG(("Issuing second async lookup for TTL for %s%s%s%s.",

[%s]%s%s%s ?

::: netwerk/dns/nsIDNSService.idl
@@ +86,5 @@
>      /**
> +     * kicks off an asynchronous host lookup.
> +     *
> +     * This function is identical to asyncResolve except an additional
> +     * parameter aNetwortInterface. If parameter aNetworkInterface is not set

maybe instead "is not set" better say "is empty string" or so.. ?

@@ +118,5 @@
> +     * way as resolve function.
> +     */
> +    nsIDNSRecord resolveExtended(in AUTF8String   aHostName,
> +                                 in unsigned long aFlags,
> +                                 in AUTF8String   aNetworkInterface);

if there is no consumer right now, we should not add any new sync APIs.

::: netwerk/test/unit/test_dns_per_interface.js
@@ +1,1 @@
> +var dns = Cc["@mozilla.org/network/dns-service;1"].getService(Ci.nsIDNSService);

please add a comment what this test is doing (step by step).  it's useful when the test has to be fixed/updated/just understood by someone else.

@@ +4,5 @@
> +var netInterface2 = "interface2";
> +var hostname = "";
> +var possible = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";
> +
> +for( var i=0; i < 20; i++ ) {

for (var a = 0; ...) {

@@ +49,5 @@
> +                                                          netInterface1,
> +                                                          listener,
> +                                                          mainThread);
> +  dns.cancelAsyncResolveExtended(hostname, flags, netInterface1,
> +                                 listener, Cr.NS_ERROR_ABORT);

this one doesn't do any notification?  I would be surprised.

@@ +62,5 @@
> +                                                             listener,
> +                                                             mainThread);
> +
> +  do_test_pending();
> +  do_test_pending();

comment why twice and why do you expect the notification come after this point (that onLookupComplete is always posted?).
Attachment #8555759 - Flags: review?(honzab.moz) → feedback+
(Assignee)

Comment 19

4 years ago
> 
> @@ +76,5 @@
> >                                nsIDNSListener    *listener,
> >                                nsIEventTarget    *target_,
> >                                nsICancelable    **result)
> >  {
> > +  return AsyncResolveExtended(hostname, flags, nsAutoCString(), listener,
> 
> EmptyCString()

There is too many ..CStrings

> ::: netwerk/dns/nsDNSService2.cpp
> @@ +345,5 @@
> >  nsDNSAsyncRequest::Cancel(nsresult reason)
> >  {
> >      NS_ENSURE_ARG(NS_FAILED(reason));
> > +    mResolver->DetachCallback(mHost.get(), mFlags, mAF, mNetworkInterface.get(),
> > +                              this, reason);
> 
> I can see the mHost is already passed down as zstring.  Do we have any
> clear/simple mechanisms that ensure this class (keeping the string alive)
> does overlive existing references to the string(s) from the nsHostResolver
> code?
> 
> if hard to answer, maybe file a bug we should do the audit.  This is
> critical.

This reference to string is only use during this function call DetachCallback(...) to create hashkey and query table.



> 
> @@ +795,5 @@
> >  
> >      uint16_t af = GetAFForLookup(hostname, aFlags);
> >  
> > +    res->CancelAsyncRequest(hostname.get(), aFlags, af,
> > +                            nsCString(aNetworkInterface).get(), aListener,
> 
> what's this?  you force a copy of the string (not sure this is the way if
> that's what you want)?  why?
> 

Again i am mixing ..CStrings.
Is nsPromiseFlatCString(aNetworkInterface).get() the right think here?


> 
> @@ +49,5 @@
> > +                                                          netInterface1,
> > +                                                          listener,
> > +                                                          mainThread);
> > +  dns.cancelAsyncResolveExtended(hostname, flags, netInterface1,
> > +                                 listener, Cr.NS_ERROR_ABORT);
> 
> this one doesn't do any notification?  I would be surprised.

it does, but I have change it
(Assignee)

Comment 20

4 years ago
Posted patch bug_1108957_v5.patch (obsolete) — Splinter Review
Attachment #8555759 - Attachment is obsolete: true
Attachment #8557943 - Flags: review?(honzab.moz)
(In reply to Dragana Damjanovic [:dragana] from comment #19)
> Is nsPromiseFlatCString(aNetworkInterface).get() the right think here?

Yup.
Comment on attachment 8557943 [details] [diff] [review]
bug_1108957_v5.patch

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

still few tweaks, but nothing really serious.

r+ when Jason checks the host names you use in the test are OK.

::: netwerk/dns/GetAddrInfo.cpp
@@ +321,5 @@
> +#else
> +  {
> +#endif
> +    prai = PR_GetAddrInfoByName(aCanonHost, aAddressFamily, prFlags);
> +  }

maybe:

#if ...
  if (...) {
  } else
#endif
  {
    ...
  }

so it doesn't confuse at all.

::: netwerk/dns/nsHostResolver.cpp
@@ +73,5 @@
>  static PRLogModuleInfo *gHostResolverLog = nullptr;
>  #define LOG(args) PR_LOG(gHostResolverLog, PR_LOG_DEBUG, args)
> +#define LOG_HOST(host, interface) nsPrintfCString("host [%s%s%s]", host,       \
> +                 (interface && interface[0] != '\0') ? " on interface " : "",  \
> +                 (interface && interface[0] != '\0') ? interface : "").get()

ah, ok, I would be alright with not doing printf's inside printf's.  having [%s%s%s] in the LOG string and let this macro put for you the 3 strings arguments would be enough and a bit more sufficient.  Now that we have logging forced everywhere it's important to optimize the logs.

But that's kinda work to do... up to you to fix or leave as this...


and define this all the time, not when just PR_LOGGIN is on.

::: netwerk/test/unit/test_dns_per_interface.js
@@ +2,5 @@
> +
> +// This test checks DNSService host resolver when a network interface is supplied
> +// as well. In the test 3 request are sent: two with a network interface set 
> +// and one without a network interface.
> +// All request have the same host to be resolved and the same flags.

All requests

@@ +14,5 @@
> +var possible = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";
> +
> +// Create random host name to be resolved.
> +for( var i = 0; i < 20; i++ ) {
> +  hostname += possible.charAt(Math.floor(Math.random() * possible.length));

hmm.. now I've spot this.  I think we should consult people from infra on what to use as a host name.  Sending somewhere some random string is probably not a very deterministic test.

@Jason, can you help here please?

@@ +33,5 @@
> +      // This request should not be canceled.
> +      do_check_neq(inStatus, Cr.NS_ERROR_ABORT);
> +
> +      do_test_finished();
> +    }

so, you are getting a notification from the canceled one or not here?  if not, please express that in a comment.  if yes, then this test is wrong and should be fixed so that is expects it and checks the error is the expected one for a canceled lookup request.

@@ +54,5 @@
> +
> +  // We wait for notification for two request that are not canceled.
> +  // For each request onLookupComplete will be called.
> +  do_test_pending();
> +  do_test_pending();

please leave those at the bottom.  or are you getting notifications synchronously?
Attachment #8557943 - Flags: review?(honzab.moz) → feedback+
@@ +14,5 @@
> +var possible = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";
> +
> +// Create random host name to be resolved.
> +for( var i = 0; i < 20; i++ ) {
> +  hostname += possible.charAt(Math.floor(Math.random() * possible.length));

hmm.. now I've spot this.  I think we should consult people from infra on what to use as a host name.  Sending somewhere some random string is probably not a very deterministic test.

@Jason, can you help here please?
Flags: needinfo?(jduell.mcbugs)
(Assignee)

Comment 24

4 years ago
(In reply to Honza Bambas (:mayhemer) from comment #22)
> Comment on attachment 8557943 [details] [diff] [review]
> bug_1108957_v5.patch
> 
> Review of attachment 8557943 [details] [diff] [review]:
> -----------------------------------------------------------------

> 
> @@ +14,5 @@
> > +var possible = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";
> > +
> > +// Create random host name to be resolved.
> > +for( var i = 0; i < 20; i++ ) {
> > +  hostname += possible.charAt(Math.floor(Math.random() * possible.length));
> 
> hmm.. now I've spot this.  I think we should consult people from infra on
> what to use as a host name.  Sending somewhere some random string is
> probably not a very deterministic test.
> 
> @Jason, can you help here please?


Once before, for another test, I was asking about this, and it was fine because I did not care about the outcome.
But for this test we can really put anything.


> @@ +33,5 @@
> > +      // This request should not be canceled.
> > +      do_check_neq(inStatus, Cr.NS_ERROR_ABORT);
> > +
> > +      do_test_finished();
> > +    }
> 
> so, you are getting a notification from the canceled one or not here?  if
> not, please express that in a comment.  if yes, then this test is wrong and
> should be fixed so that is expects it and checks the error is the expected
> one for a canceled lookup request.
> 

I will put in a check that we are getting a notification, but we cannot check the expected error because it is racy. There is a comment about this but i will write it differently.

Comment 25

4 years ago
re: the fake DNS name.  I assume the idea here is that by issuing something random, we're guaranteed that the host OS resolver will have to hit the network, which makes it very likely that our cancel() call will not race (i.e. the request won't have been completed before we cancel it).  But I'd guess that would work better if we had a ".com" at the end of the string, so we know DNS will make a network request to a top-level name server.

I'm not sure of the rules (if any) we have for what's permitted in our test infrastructure here.  Generally of course we don't allow any outside network access, but doing a DNS request for a bogus hostname might still be ok?  Maybe Clint knows.
Flags: needinfo?(jduell.mcbugs) → needinfo?(ctalbert)
(Assignee)

Comment 26

4 years ago
Actually since I have change the test, I can also put localhost.

I have put random because of the race with the cancel operation, but because of that race, now I am not checking the responses for the canceled requests only for not canceled requests, so it will be ok.
Probably most of the time cancel will be faster, so we are testing that the right requests got canceled and the other did not get canceled, so the test is still useful.

Comment 27

4 years ago
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #25)
> re: the fake DNS name.  I assume the idea here is that by issuing something
> random, we're guaranteed that the host OS resolver will have to hit the
> network, which makes it very likely that our cancel() call will not race
> (i.e. the request won't have been completed before we cancel it).  But I'd
> guess that would work better if we had a ".com" at the end of the string, so
> we know DNS will make a network request to a top-level name server.
> 
> I'm not sure of the rules (if any) we have for what's permitted in our test
> infrastructure here.  Generally of course we don't allow any outside network
> access, but doing a DNS request for a bogus hostname might still be ok? 
> Maybe Clint knows.

In general, I want to shy away from anything random in the tests. For something like this, I would use something like:thisshouldnotexist.mozilla.com
Then you would hit the network, but stay inside our walls, and it would be guaranteed to fail.

The reason we like things to not be random is that we do periodically review all the network requests coming from the tests, and try to understand if there are any problems from those network requests. So having deterministic names (even for things that don't exist) makes our lives easier.

That said, if you can use localhost (I'm not sure how you can test DNS with localhost only, so I'd like to learn that if you can explain it further) that's even better.
Flags: needinfo?(ctalbert)
(Assignee)

Comment 28

4 years ago
(In reply to Clint Talbert ( :ctalbert ) from comment #27)

> In general, I want to shy away from anything random in the tests. For
> something like this, I would use something
> like:thisshouldnotexist.mozilla.com
> Then you would hit the network, but stay inside our walls, and it would be
> guaranteed to fail.
> 
> The reason we like things to not be random is that we do periodically review
> all the network requests coming from the tests, and try to understand if
> there are any problems from those network requests. So having deterministic
> names (even for things that don't exist) makes our lives easier.
> 
> That said, if you can use localhost (I'm not sure how you can test DNS with
> localhost only, so I'd like to learn that if you can explain it further)
> that's even better.

This test is not about thoroughly testing our host resolver meaning all possibilities for different host names, ip addresses literals, etc. It is testing our DNS service from submitting an async request, our dns request queues, till getting the result back (what ever that result is).

I tested the test with localhost and without e10s a request is canceled fast enough (before the results get back), but with e10s it is better to use something else.
I will use thisshouldnotexist.mozilla.com

thanks Clint.
(Assignee)

Comment 29

4 years ago
Posted patch bug_1108957_v5.patch (obsolete) — Splinter Review
Attachment #8545599 - Attachment is obsolete: true
Attachment #8557943 - Attachment is obsolete: true
Attachment #8561300 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Hi, comment #22 was marked as feedback + and changed from review to feedback. Was this also a review + ?
Flags: needinfo?(dd.mozilla)
Keywords: checkin-needed
(Assignee)

Comment 32

4 years ago
If you look at comment #22 about 10th line:

"r+ when Jason checks the host names you use in the test are OK."

and according to comment #27 the host name that i use in the final version is allowed.
Flags: needinfo?(dd.mozilla)
sorry had to back this out for bustage in nexus builds like in https://treeherder.mozilla.org/logviewer.html#?job_id=6461644&repo=mozilla-inbound

05:24:28 ERROR - ../../../gecko/netwerk/dns/GetAddrInfo.cpp:261:72: error: 'android_getaddrinfofornet' was not declared in this scope
05:24:28 INFO - rv = android_getaddrinfofornet(hostname, NULL, &hints, netId, 0, &res);
05:24:28 INFO - ^
05:24:28 ERROR - make[6]: *** [Unified_cpp_netwerk_dns0.o] Error 1
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 35

4 years ago
Thanks Carsten.

Henry, in comment #4 you wrote that android_getaddrinfoforiface will be replace with android_getaddrinfofornet.
In an e-mail you wrote that the new interface is for ANDRIOD_VERSION >= 20.
Have I miss something (see comment 34)?
Flags: needinfo?(dd.mozilla) → needinfo?(hchang)
I guess it may be required to include #include <resolv_netid.h> like [1]. Are you able to get a chance to have a nexus 5 lollipop build from your side? I am also willing to get it a try but probably tomorrow or later since I am in a business trip in Paris.

Thanks!

http://androidxref.com/5.0.0_r2/xref/system/netd/server/DnsProxyListener.cpp#28
Flags: needinfo?(hchang)
Duplicate of this bug: 1131591
(Assignee)

Comment 38

4 years ago
Posted patch bug_1108957_v6.patch (obsolete) — Splinter Review
Added missing dependencies.

Michael, can you please check a change that I made in congifure.in
Attachment #8561300 - Attachment is obsolete: true
Attachment #8565085 - Flags: review?(mwu)
Comment on attachment 8565085 [details] [diff] [review]
bug_1108957_v6.patch

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

Add that include path to the moz.build of the source directories that need it.
Attachment #8565085 - Flags: review?(mwu) → review-
(Assignee)

Comment 40

4 years ago
Posted patch bug_1108957_v7.patch (obsolete) — Splinter Review
The only thing that have changed is added a header. It is only for gonk version 21. I forgot to test it and it its not on try.
Attachment #8565085 - Attachment is obsolete: true
Attachment #8565979 - Flags: review?(honzab.moz)
Comment on attachment 8565979 [details] [diff] [review]
bug_1108957_v7.patch

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

Thanks, r=honzab

::: netwerk/test/unit/test_dns_per_interface.js
@@ +70,5 @@
> +  requestWithInterfaceNotCanceled = dns.asyncResolveExtended(hostname, flags,
> +                                                             netInterface2,
> +                                                             listener,
> +                                                             mainThread);
> +  // We wait for notification for two request that are not canceled.

nit: update the comment
Attachment #8565979 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 42

4 years ago
Attachment #8565979 - Attachment is obsolete: true
Attachment #8566428 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0c26eb16bc31
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Hi Dragana,

Thanks for your hard work! I am working based on your patch right now.
However, because of certain project which will use 2.2 and requires
Bug 992772 to be solved, we need this patch also to be uplifted to 2.2.
Could you please help also get it land on 2.2? 

Thanks!
Flags: needinfo?(dd.mozilla)
Please request an approval b2g37 :)

Note to release managers: without this and bug 992772 (see bug 1053650), MMS don't work if the phone is connected on Wi-Fi (for some carriers only, IIRC). We lived with this for some time now but this is extremely frustrating.
(Assignee)

Comment 47

4 years ago
Comment on attachment 8566428 [details] [diff] [review]
bug_1108957_v8.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): A new feature.
User impact if declined: Without this MMS do not work when a phone is connected to Wi-Fi. Also see comment #46
Testing completed: There is xpcshell test witch test part of it. The rest was tested manually because it is not possible to have automatic test.
Risk to taking this patch (and alternatives if risky): Low risk. it is already almost 2 weeks.
String or UUID changes made by this patch: yes, one.
Flags: needinfo?(dd.mozilla)
Attachment #8566428 - Flags: approval-mozilla-b2g37?
Flags: needinfo?(echang)
Keywords: verifyme
Comment on attachment 8566428 [details] [diff] [review]
bug_1108957_v8.patch

Eric, can you pleae add this to the QA verification queue once this lands on 2.2 (testing MMs etc per bug comments)?
Attachment #8566428 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/54d09b376d9c
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
status-firefox36: --- → wontfix
status-firefox37: --- → wontfix
Thank you, Ryan and Dragana :)
(In reply to bhavana bajaj [:bajaj] from comment #48)
> Comment on attachment 8566428 [details] [diff] [review]
> bug_1108957_v8.patch
> 
> Eric, can you pleae add this to the QA verification queue once this lands on
> 2.2 (testing MMs etc per bug comments)?

Bug 1108957 doesn't solve MMS bug actually. However, this bug is the
prerequisite function for Bug 992772 and Bug 1118272 which actually
solves the MMS bug.
Sure we will verify this, thanks.
(In reply to bhavana bajaj [:bajaj] from comment #48)
> Comment on attachment 8566428 [details] [diff] [review]
> bug_1108957_v8.patch
> 
> Eric, can you pleae add this to the QA verification queue once this lands on
> 2.2 (testing MMs etc per bug comments)?
Flags: needinfo?(echang)
Just checked with RD, we are not able to verify this at this moment.
1. There are dependency bugs which are not yet resolved. https://bugzil.la/1046517
2. This case could be verified with Wifi on, and APN is a domain name, not an IP.
We can verify after https://bugzil.la/1046517 is resolved, remove verifyme for now.

(In reply to Eric Chang [:ericcc] [:echang] from comment #52)
> Sure we will verify this, thanks.
> (In reply to bhavana bajaj [:bajaj] from comment #48)
> > Comment on attachment 8566428 [details] [diff] [review]
> > bug_1108957_v8.patch
> > 
> > Eric, can you pleae add this to the QA verification queue once this lands on
> > 2.2 (testing MMs etc per bug comments)?
Keywords: verifyme
Depends on: 1152122
Depends on: 1152329
No longer depends on: 1152122
You need to log in before you can comment on or make changes to this bug.