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

Status: RESOLVED FIXED

(Core :: Networking: DNS, defect)

Description

[Dragana Damjanovic [:dragana]]

Add per network interface dns query.
The dns cache needs to be extended as well.
Per network interface dns queries are possible with b2g.

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: fix v1

Comment 2

[Dragana Damjanovic [:dragana]]

Attachment: fix v1

A test added.

Comment 3

[Henry Chang [:hchang]]

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

Comment 4

[Henry Chang [:hchang]]

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

Comment 5

[Henry Chang [:hchang]]

Attachment: Bug1108957.patch

Rebased and ignore ANDRIOD_VERSION check in prnetdb for testing.

Comment 6

[Dragana Damjanovic [:dragana]]

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)

Comment 7

[Henry Chang [: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

Comment 8

[Dragana Damjanovic [:dragana]]

Attachment: fix v2

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.

Comment 9

[Dragana Damjanovic [:dragana]]

May be you can give me feedback on my question from  comment 8.

Comment 10

[(not currently active) Ted Mielczarek]

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.

Comment 11

[Dragana Damjanovic [:dragana]]

Attachment: fix v3

Comment 12

[Dragana Damjanovic [:dragana]]

Can you try this one.
this is without changing nspr.
So if it is working I can mark it for review.
Thank you.

Comment 13

[Henry Chang [: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!

Comment 14

[Henry Chang [: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!

Comment 15

[Dragana Damjanovic [:dragana]]

Thanks for the test!

Comment 16

[Dragana Damjanovic [:dragana]]

Attachment: v4

Comment 17

[Honza Bambas (:mayhemer)]

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 18

[Honza Bambas (:mayhemer)]

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

Comment 19

[Dragana Damjanovic [:dragana]]

> 
> @@ +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

Comment 20

[Dragana Damjanovic [:dragana]]

Attachment: bug_1108957_v5.patch

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to Dragana Damjanovic [:dragana] from comment #19)
> Is nsPromiseFlatCString(aNetworkInterface).get() the right think here?

Yup.

Comment 22

[Honza Bambas (:mayhemer)]

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?

Comment 23

[Honza Bambas (:mayhemer)]

@@ +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?

Comment 24

[Dragana Damjanovic [:dragana]]

(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

[Jason Duell]

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.

Comment 26

[Dragana Damjanovic [:dragana]]

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

[cmtalbert]

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

Comment 28

[Dragana Damjanovic [:dragana]]

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

Comment 29

[Dragana Damjanovic [:dragana]]

Attachment: bug_1108957_v5.patch

Comment 30

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ed5f6b21256

Comment 31

[Carsten Book [:Tomcat]]

Hi, comment #22 was marked as feedback + and changed from review to feedback. Was this also a review + ?

Comment 32

[Dragana Damjanovic [:dragana]]

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.

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d0e23391d53

Comment 34

[Carsten Book [:Tomcat]]

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

Comment 35

[Dragana Damjanovic [:dragana]]

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

Comment 36

[Henry Chang [: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

Comment 38

[Dragana Damjanovic [:dragana]]

Attachment: bug_1108957_v6.patch

Added missing dependencies.

Michael, can you please check a change that I made in congifure.in

Comment 39

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

Comment 40

[Dragana Damjanovic [:dragana]]

Attachment: bug_1108957_v7.patch

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.

Comment 41

[Honza Bambas (:mayhemer)]

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

Comment 42

[Dragana Damjanovic [:dragana]]

Attachment: bug_1108957_v8.patch

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c26eb16bc31

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0c26eb16bc31

Comment 45

[Henry Chang [:hchang]]

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!

Comment 46

[Julien Wajsberg [:julienw]]

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.

Comment 47

[Dragana Damjanovic [:dragana]]

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.

Comment 48

[bhavana bajaj [:bajaj]]

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

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/54d09b376d9c

Comment 50

[Henry Chang [:hchang]]

Thank you, Ryan and Dragana :)

Comment 51

[Henry Chang [:hchang]]

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

Comment 52

[Eric Chang [:ericcc] [:echang]]

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

Comment 53

[Eric Chang [:ericcc] [: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)?