1053650 - [B2G][MMS] Host Resolving with specified DNS in XMLHttpRequest

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

Bug 1032097 is to address the problem of updating routing during mms connection.

File this new bug to follow up the discussion in comment 22 of Bug 1032097 regarding the host resolving issue with specified DNS in XMLHttpRequest:
https://bugzilla.mozilla.org/show_bug.cgi?id=1032097

In addition to the host in the url, proxy is also needed to take into consideration, because for some carriers, a named mms proxy is also provided for mms transactions.

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Patrick McManus [:mcmanus] from comment #21)
> I'm coming late to this bug - but it looks like you want to use XHR and have
> the app do DNS resolution itself, right?
> 
> Can you help me understand why? 


Because we need a specific name server for mms XMLHttpRequest 
transaction while a general purpose name server is still required
at the same time to resolve other domain names.

> And why not just hook the DNS Service to do the override?
> 


That's exactly what we need! Could you please point the 
where the hook is? I checked nsXMLHttpRequest but didn't find.
The place that DNS is resolved through all the XMLHttpRequest
seems here [1].

[1] http://hg.mozilla.org/mozilla-central/file/d7e78f0c1465/netwerk/base/src/nsSocketTransport2.cpp#l1043

> The way you have it written will probably work for that single transaction,
> but the transaction's concept of origin and everything that flows from that
> (e.g. SSL certificate auth, hsts, tracking protection, etc) will likely be
> mapped to the dotted decimal version which is not what you want.


If we only use the response of the XHR [2], will it be fine?
The response isn't even parsed to a DOM tree and run client side
code.

[2] http://hg.mozilla.org/mozilla-central/file/d7e78f0c1465/dom/mobilemessage/src/gonk/MmsService.js#l704

Thanks!

Comment 2

[Patrick McManus [:mcmanus]]

It looks like this implements SMS/MMS over HTTP, is that right? You'll need to forgive me that I don't know any of the details of that space so I might ask some weird questions.

First, I would expect that to be over https. Why isn't it (or is it?)? If it isn't today, let's presume it will be tomorrow.

Your origin, which is determined by your URL, is critical to doing https authentication. The server has to have a certificate signed for that origin - and they aren't going to have a signature for a dotted decimal IP address.

So that problem, and others like it (HSTS comes to mind), is why I'm uncomfortable with the host header game you have implemented to this point.

I'm trying to figure out if you just need to resolve www.some-domain.com globally across gecko as if it were a static host entry (i.e. have a value for it they all users see - even though that might not be the value from the public DNS) *OR* whether this resolution needs to be scoped to a particular XHR/nsIChannel (i.e. that channel gets the static host resolution, but other channels in other apps use the public DNS).

You can do the gecko-wide one with existing hooks, we don't have an answer for the channel specific one. Just implement the nsIDNSService service (@mozilla.org/network/dns-service;1) - respond with the right answer for AsyncResolve or Resolve calls that contain the hostname you are interested in, and pass everything else through to the original service.

You can set a proxy on a per transaction basis a couple of different ways. The most common is to implement a http-on-modify-request handler where the proxy information could be updated if necessary on the callback.

Comment 3

[Henry Chang [:hchang]]

(In reply to Patrick McManus [:mcmanus] from comment #2)
> It looks like this implements SMS/MMS over HTTP, is that right? You'll need
> to forgive me that I don't know any of the details of that space so I might
> ask some weird questions.
> 

One step of mms transaction requires HTTP and it wouldn't be https.

> First, I would expect that to be over https. Why isn't it (or is it?)? If it
> isn't today, let's presume it will be tomorrow.
> 
> Your origin, which is determined by your URL, is critical to doing https
> authentication. The server has to have a certificate signed for that origin
> - and they aren't going to have a signature for a dotted decimal IP address.
> 
> So that problem, and others like it (HSTS comes to mind), is why I'm
> uncomfortable with the host header game you have implemented to this point.
> 

Got it and thanks for teaching us :) So helpful. But it sounds like 
http-only transaction would be safe even it's definitely a hack, is it?

> I'm trying to figure out if you just need to resolve www.some-domain.com
> globally across gecko as if it were a static host entry (i.e. have a value
> for it they all users see - even though that might not be the value from the
> public DNS) *OR* whether this resolution needs to be scoped to a particular
> XHR/nsIChannel (i.e. that channel gets the static host resolution, but other
> channels in other apps use the public DNS).
> 

The special domain we want to resolve is not globally applicable.
In other words, this domain may also be resolvable by other servers
but they may not be the right answer.

How android solves this problem is there exists some API to bind
a process to a specific name server, which is not suitable for gecko
because the transaction always occurs on chrome process.

> You can do the gecko-wide one with existing hooks, we don't have an answer
> for the channel specific one. Just implement the nsIDNSService service
> (@mozilla.org/network/dns-service;1) - respond with the right answer for
> AsyncResolve or Resolve calls that contain the hostname you are interested
> in, and pass everything else through to the original service.
> 

I think we do need a hook for specific channel. I will be investigating
this. Can we have your comment on this?

> You can set a proxy on a per transaction basis a couple of different ways.
> The most common is to implement a http-on-modify-request handler where the
> proxy information could be updated if necessary on the callback.

I am not sure I got all of this but will study further on proxy things.

The only problem left here is if it's appropriate to add a
DNS hook to XMLHttpRequest. Thanks!

Comment 4

[Patrick McManus [:mcmanus]]

(In reply to Henry Chang [:henry] from comment #3)
> (In reply to Patrick McManus [:mcmanus] from comment #2)

> 
> One step of mms transaction requires HTTP and it wouldn't be https.

can you provide me more information (pointers on standards to read?) on this. It sounds like it could be really bad for our user's privacy, right?

> 
> Got it and thanks for teaching us :) So helpful. But it sounds like 
> http-only transaction would be safe even it's definitely a hack, is it?

there are http:// things that use origin rules as well. CORS is based on it and is tightly coupled to XHR. You could also conceivably have trouble with some kinds of captive portal situations which actually dns resolve everything to the same IP and then proxy your traffic (united inflight wi-fi has done things like that to me).

In general, what you are doing is a little odd. I would expect mms to maybe allow you to choose from 1 for N aliases for the service name, each of which uses DNS in the usual way. But if that's not what happens, that's not what happens :)

> I think we do need a hook for specific channel. I will be investigating
> this. Can we have your comment on this?

its something we could plausibly add to the chrome interface.

> 
> The only problem left here is if it's appropriate to add a
> DNS hook to XMLHttpRequest. Thanks!

xhr is a web facing api, not a chrome one (it wraps the chrome channels for the most part).. so you would need to talk to jonas or bz for that part.

Comment 5

[Henry Chang [:hchang]]

(In reply to Patrick McManus [:mcmanus] from comment #4)
> (In reply to Henry Chang [:henry] from comment #3)
> > (In reply to Patrick McManus [:mcmanus] from comment #2)
> 
> > 
> > One step of mms transaction requires HTTP and it wouldn't be https.
> 
> can you provide me more information (pointers on standards to read?) on
> this. It sounds like it could be really bad for our user's privacy, right?
> 
> > 
> > Got it and thanks for teaching us :) So helpful. But it sounds like 
> > http-only transaction would be safe even it's definitely a hack, is it?
> 
> there are http:// things that use origin rules as well. CORS is based on it
> and is tightly coupled to XHR. You could also conceivably have trouble with
> some kinds of captive portal situations which actually dns resolve
> everything to the same IP and then proxy your traffic (united inflight wi-fi
> has done things like that to me).
> 

The use of HTTP in mms scenario is a very limited application.
HTTP is just used to download data like this [1]. But I don't
know if it's insecure since the transaction occurs in a private
channel. 

[1] http://dxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/MmsService.js?from=mmsservice.js#704

> In general, what you are doing is a little odd. I would expect mms to maybe
> allow you to choose from 1 for N aliases for the service name, each of which
> uses DNS in the usual way. But if that's not what happens, that's not what
> happens :)
> 
> > I think we do need a hook for specific channel. I will be investigating
> > this. Can we have your comment on this?
> 
> its something we could plausibly add to the chrome interface.
> 
> > 
> > The only problem left here is if it's appropriate to add a
> > DNS hook to XMLHttpRequest. Thanks!
> 
> xhr is a web facing api, not a chrome one (it wraps the chrome channels for
> the most part).. so you would need to talk to jonas or bz for that part.

We only need the DNS hook for chrome interface (nsIXMLHttpRequest).
Whenever I have a proposal for such a hook, I will update here soon.

Thanks!

Comment 6

[Patrick McManus [:mcmanus]]

(In reply to Henry Chang [:henry] from comment #5)
> (In reply to Patrick McManus [:mcmanus] from comment #4)
> > (In reply to Henry Chang [:henry] from comment #3)
> > > (In reply to Patrick McManus [:mcmanus] from comment #2)
> > 
> > > 
> > > One step of mms transaction requires HTTP and it wouldn't be https.
> > 
> > can you provide me more information (pointers on standards to read?) on
> > this. It sounds like it could be really bad for our user's privacy, right?
> > 
> > > 
> > > Got it and thanks for teaching us :) So helpful. But it sounds like 
> > > http-only transaction would be safe even it's definitely a hack, is it?
> > 
> > there are http:// things that use origin rules as well. CORS is based on it
> > and is tightly coupled to XHR. You could also conceivably have trouble with
> > some kinds of captive portal situations which actually dns resolve
> > everything to the same IP and then proxy your traffic (united inflight wi-fi
> > has done things like that to me).
> > 
> 
> The use of HTTP in mms scenario is a very limited application.
> HTTP is just used to download data like this [1]. But I don't
> know if it's insecure since the transaction occurs in a private
> channel. 
> 
> [1]
> http://dxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/
> MmsService.js?from=mmsservice.js#704
> 

Thanks for the reply. Your link doesn't really help me understand semantically what that data is. can you help my understanding? If it isn't over https, its not secure and its probably a problem waiting to be exploited.

> > xhr is a web facing api, not a chrome one (it wraps the chrome channels for
> > the most part).. so you would need to talk to jonas or bz for that part.
> 
> We only need the DNS hook for chrome interface (nsIXMLHttpRequest).
> Whenever I have a proposal for such a hook, I will update here soon.

:bz really ought to have a look at all things xhr, including that.

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Patrick McManus [:mcmanus] from comment #6)
> 
> Thanks for the reply. Your link doesn't really help me understand
> semantically what that data is. can you help my understanding? If it isn't
> over https, its not secure and its probably a problem waiting to be
> exploited.
> 

Here are some information regarding MMS binding HTTP protocol stack.
Hope this is helpful to clarify your concern:
1. Unlike other HTTP service in public internet, MMS service will only be applied in internal network
   provided by carrier. 
   In this case, carrier will provide the APN for mobile device to setup this specific data connection 
   and provide the mmsc/mms proxy address for MMS transaction.
2. For the MMS implementation in gecko, all the transactions are handled internally in gecko
   (MmsService.js) instead of in the app.
3. HTTP protocol is used by MMS as a bearer of MMS PDUs defined in [1] and possible binding to HTTP 
   stack is also mentioned in Chapter 8.2. Binding When MMS Client Uses HTTP Based Stack of [2].
   Hence, what MMS client to do is to encode/decode the binary stream of the MMS PDUs in the HTTP body.

[1] http://technical.openmobilealliance.org/Technical/release_program/docs/MMS/V1_3-20110913-A/OMA-TS-MMS_ENC-V1_3-20110913-A.pdf
[2] http://technical.openmobilealliance.org/Technical/release_program/docs/MMS/V1_3-20110913-A/OMA-TS-MMS_CTR-V1_3-20110913-A.pdf

Comment 8

[Patrick McManus [:mcmanus]]

Thanks for the pointers - there is a lot of info there!

One thing I'll note is that the first document doesn't seem to talk about http vs https, it just uses uris. So either seems a plausible deployment and I hope we encourage our partners to use https.

The second document definitely discusses https.. and it even goes so far as to say

"Security Considerations
At present, the end-to-end security aspects of the PDUs are dependent upon the security provided by the transport service(s) utilized.
[..]
When the MMS Proxy-Relay in an M-Notification.ind PDU identifies a new MM to the MMS Client using the https URI
scheme, the MMS Client SHALL use a secure transport"

The host hack you're using won't be compatible with that.

We want to encourage https:// in this context imo. it would allow b2g to assert that it really is talking to the appropriate end point and the communication is private and untampered. Otherwise you're just trusting in a VPN that gecko has no insight into.

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Patrick McManus [:mcmanus] from comment #8)
> "Security Considerations
> At present, the end-to-end security aspects of the PDUs are dependent upon
> the security provided by the transport service(s) utilized.
> [..]
> When the MMS Proxy-Relay in an M-Notification.ind PDU identifies a new MM to
> the MMS Client using the https URI
> scheme, the MMS Client SHALL use a secure transport"
> 
> The host hack you're using won't be compatible with that.
> 
Thanks for pointing out this.
We didn't take https into consideration because we haven't seen any request from carrier-side to support this yet but it will become a problem when it comes. :(

> We want to encourage https:// in this context imo. it would allow b2g to
> assert that it really is talking to the appropriate end point and the
> communication is private and untampered. Otherwise you're just trusting in a
> VPN that gecko has no insight into.

Comment 10

[Henry Chang [:hchang]]

This bug will be the last mile to solving Bug 992772. 

I am going to add a DNS service hook to nsIXMLHttpRequest [1]. 
I was hoping to append another optional parameter to |open| 
like the following

[optional_argc] void open(in ACString method, in AUTF8String url,
                          [optional] in boolean async,
                          [optional,Undefined(Empty)] in DOMString user,
                          [optional,Undefined(Empty)] in DOMString password,
                          [optional] in nsIDNSService userDnsService);

but it makes the parameter list more like a beast. So I intend to
add a mozilla and chrome only attribute "userDnsService" and populate
all the way down to [2]. 

Other than introducing a DNS hook, I am wondering if we can instead
store an additional |mIpBasedHost| and avoid resolving host if it's
non-empty. This approach seems to require less effort than a DNS hook
but potentially store inconsistent domain name and ip.

:bz, what do you think of adding such a DNS service hook to XMLHttpRequest
chrome interface? We certainly need a "http specific" or "channel specific"
DNS server for MMS. Thanks!

[1] http://hg.mozilla.org/mozilla-central/file/e7806c9c83f3/content/base/public/nsIXMLHttpRequest.idl
[2] http://hg.mozilla.org/mozilla-central/file/e7806c9c83f3/netwerk/base/src/nsSocketTransport2.cpp#l1025
[3] http://hg.mozilla.org/mozilla-central/file/e7806c9c83f3/netwerk/base/src/nsSocketTransport2.h#l267

Comment 11

[Henry Chang [:hchang]]

Attachment: Bug1053650.patch

I got a new idea to solve this issue. This patch adds a new XMLHttpRequest internal API |openOnPreResolvedHost|. Everything remains the same after nsHttpConnectionMgr creates a SocketTransport [1] but a pre-resolved ip-based host will be passed along nsHttpChannel and nsHttpConnectionInfo for creating SocketTransport.

[1] http://hg.mozilla.org/mozilla-central/file/dac8b4a0bd7c/netwerk/protocol/http/nsHttpConnectionMgr.cpp#l2913

Comment 12

[Patrick McManus [:mcmanus]]

Comment on attachment 8476594 [details] [diff] [review]
Bug1053650.patch

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

::: netwerk/protocol/http/nsHttpConnectionInfo.h
@@ +52,5 @@
>      {
>          SetOriginServer(nsDependentCString(host), port);
>      }
>  
> +    void SetPreResolvedHost(const nsACString& aPreResolvedHost) { mPreResolvedHost = aPreResolvedHost; }

this is going to need to carefully annotate mHashKey as well - otherwise you run the risk of connections with different "pre-resolution" information re-using the same persistent connections.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +2917,2 @@
>      rv = sts->CreateTransport(socketTypes, typeCount,
> +                              host,

you're still going to have https problems here with authentication because psm uses this host param for authentication

There are two approaches - 

1] bubble this logic down into nsSocketTransport2.cpp and catch it right before resolution

2] coordinate with me on the "opportunistic security" patches I'm doing for h2. Those will probably land in the next gecko cycle and they basically split host into "host to route to" and "hostname to authenticate"

::: netwerk/protocol/http/nsIHttpChannelInternal.idl
@@ +201,5 @@
>      void addRedirect(in nsIPrincipal aPrincipal);
>  
>      readonly attribute PRTime lastModifiedTime;
> +
> +    attribute ACString preResolvedHost;

I'm basically ok with this - but I think I'd like to call it "explicitRoute".. and they way you have it written it can be pre-resolved (i.e. dotted decimal) or not without a problem.

Comment 13

[Patrick McManus [:mcmanus]]

I omitted comments on the xhr portions.

Comment 14

[Henry Chang [:hchang]]

Thanks for your comments!

(In reply to AFK Aug 22- Sep 01 Patrick McManus [:mcmanus] from comment #12)
> Comment on attachment 8476594 [details] [diff] [review]
> Bug1053650.patch
> 
> Review of attachment 8476594 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpConnectionInfo.h
> @@ +52,5 @@
> >      {
> >          SetOriginServer(nsDependentCString(host), port);
> >      }
> >  
> > +    void SetPreResolvedHost(const nsACString& aPreResolvedHost) { mPreResolvedHost = aPreResolvedHost; }
> 
> this is going to need to carefully annotate mHashKey as well - otherwise you
> run the risk of connections with different "pre-resolution" information
> re-using the same persistent connections.
> 

Thanks for the reminding! I'll look into persisten connections things.

> ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
> @@ +2917,2 @@
> >      rv = sts->CreateTransport(socketTypes, typeCount,
> > +                              host,
> 
> you're still going to have https problems here with authentication because
> psm uses this host param for authentication
> 
> There are two approaches - 
> 
> 1] bubble this logic down into nsSocketTransport2.cpp and catch it right
> before resolution
> 

This is what I most originally though: store both host and "resolve host"
in nsSocketTransport2. 

> 2] coordinate with me on the "opportunistic security" patches I'm doing for
> h2. Those will probably land in the next gecko cycle and they basically
> split host into "host to route to" and "hostname to authenticate"
> 

May I know the bug number? I am interested in the patches.

> ::: netwerk/protocol/http/nsIHttpChannelInternal.idl
> @@ +201,5 @@
> >      void addRedirect(in nsIPrincipal aPrincipal);
> >  
> >      readonly attribute PRTime lastModifiedTime;
> > +
> > +    attribute ACString preResolvedHost;
> 
> I'm basically ok with this - but I think I'd like to call it
> "explicitRoute".. and they way you have it written it can be pre-resolved
> (i.e. dotted decimal) or not without a problem.

One more thing I forgot to address: the proxy. Since the proxyInfo
will be propagated to nsSocketTransport2 as well, I have to check
if we can pass ip-based URL in the beginning or we're required to
do something like what I do in the patch.

Comment 15

[Patrick McManus [:mcmanus]]

(In reply to Henry Chang [:henry] from comment #14)
> in nsSocketTransport2. 
> 
> > 2] coordinate with me on the "opportunistic security" patches I'm doing for
> > h2. Those will probably land in the next gecko cycle and they basically
> > split host into "host to route to" and "hostname to authenticate"
> > 
> 
> May I know the bug number? I am interested in the patches.
> 

1003448

Comment 16

[Boris Zbarsky [:bzbarsky]]

Ignoring the necko parts for the moment...

Was this actually tested?  Is this new API meant to be used from script?  Because I don't think nsIXMLHttpRequest affects what script sees nowadays.

Comment 17

[Henry Chang [:hchang]]

(In reply to Boris Zbarsky [:bz] from comment #16)
> Ignoring the necko parts for the moment...
> 
> Was this actually tested?  Is this new API meant to be used from script? 
> Because I don't think nsIXMLHttpRequest affects what script sees nowadays.

This wasn't tested yet and the new API will be used by [1].
Thanks!

[1] http://hg.mozilla.org/mozilla-central/file/daa84204a11a/dom/mobilemessage/src/gonk/MmsService.js#l751

Comment 18

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry] from comment #17)
> (In reply to Boris Zbarsky [:bz] from comment #16)
> > Ignoring the necko parts for the moment...
> > 
> > Was this actually tested?  Is this new API meant to be used from script? 
> > Because I don't think nsIXMLHttpRequest affects what script sees nowadays.
> 
> This wasn't tested yet and the new API will be used by [1].
> Thanks!
> 
> [1]
> http://hg.mozilla.org/mozilla-central/file/daa84204a11a/dom/mobilemessage/
> src/gonk/MmsService.js#l751

:bz

nsIXMLHttpRequest does not affect what scripts sees indeed...
So, I modify XMLHttpRequest.webidl to add a new interface 
"openViaExplicitRoute". Thanks for reminding.

Patrick,

I changed all 'preresolvedHost' to 'explictRoute',
bubble it to SocketTransport and make use of it before
resolving host. Besides, I also take the proxy info
into consideration: I add an attribute 'explicitRoute'
to nsIProxyInfo and also bubble it to SocketTransport.
When resolving host, mProxyExplictRoute and mExplicitRoute
would be checked first.

I tested openViaExplicitRoute and the passed 'explictRoute'
will actually be used to resolve. May I ask for your comments
again? Thanks!

Comment 19

[Henry Chang [:hchang]]

Attachment: Bug1053650_V2.patch

Comment 20

[Boris Zbarsky [:bzbarsky]]

You don't need to change nsIXMLHttpRequest.idl at all anymore, right?

>+nsXMLHttpRequest::SetExplicitRoute(const nsACString& aExplicitRoute) {

Please follow standard DOM style: curly on next line.

>+  // Called by XMLHttpRequestBinding.

Please don't add those comments all over.  Nothing prevents other code from calling these methods, and it shouldn't matter who calls them.

>+  void openViaExplicitRoute(ByteString method, DOMString url, boolean async,

This is exposing the API to the web.  Shouldn't it be [ChromeOnly]?  I thought that was the idea...

Does explicitRoute really need to be nullable?  I don't think it does.  Furthermore, does it need to be optional?  Are there cases in which you would call openViaExplicitRoute but NOT pass an explicitRoute?

>+    OpenInternal(aMethod, aUrl, true, Optional<nsAString>(),
>+         Optional<nsAString>(), Optional<nsAString>(), aRv);

Please fix the indentation.

That said, do we actually need this API on workers?  Or can we mark it as [Exposed=Window]?

I didn't look at the necko end of things, just the XHR stuff.

Comment 21

[Henry Chang [:hchang]]

Attachment: Bug1053650_V3.patch

Attachment V3 addresses bz's feedback and add hash key rebuild to nsHttpConnectionInfo.

Comment 22

[Henry Chang [:hchang]]

Patrick,

Bevis and I just came out another idea and we'd like to
know your thought. Instead of carrying ip or DNS server
all the way to SocketTransport, we'd like to generalize the
auxiliary data to 'NetworkInterface', which can be used and 
interpreted differently on different platform. 

For B2G, the opaque data would be nsINetworkInterface [1].
The name of nsINetworkInterface would be used to resolve
DNS on specific name server and it's gonk only implementation.
For now 'NetworkInterface' only makes difference to DNS lookup
but it may make difference to other behavior in the future.

The implementation will look the way similar to the 'explicit route'
version except a section of gonk-only code will be added
to SocketTransport (since nsINetworkInterface is gonk-only interface
and the per-interface DNS lookup relies on AOSP.)

We appreciate your feedback. Thanks!

[1] http://hg.mozilla.org/mozilla-central/file/dc352a7bf234/dom/system/gonk/nsINetworkManager.idl#l13

Comment 23

[Henry Chang [:hchang]]

Attachment: ByNetworkInterfaceId.patch

A patch to demonstrate my idea at comment 22:

1) Use a string as an identifier for certain network interface, which
   is different from what I proposed in the beginning because the life
   cycle of the actual network interface object may be difficult to control.
   Use a key/id to associate the network interface.

2) I only try to change the ends of the entire implementation for demo.

Comment 24

[Henry Chang [:hchang]]

Attachment: Part 1: Add attribute 'networkInterfaceId' to XMLHttpRequest

Comment 25

[Henry Chang [:hchang]]

Attachment: Part 2: Populate networkInterfaceId to SocketTransport.

Comment 26

[Henry Chang [:hchang]]

Attachment: Part 3: Make use of gonk-specific per-interface DNS lookup.

Comment 27

[Henry Chang [:hchang]]

Attachment: Part 4: Implement nsINetworkService.resolveHostForInterface

Comment 28

[Henry Chang [:hchang]]

Attachment: Example

Comment 29

[Henry Chang [:hchang]]

Comment on attachment 8482141 [details] [diff] [review]
Part 1: Add attribute 'networkInterfaceId' to XMLHttpRequest

bz,

My new approach passes 'networkInterfaceId' to XMLHttpRequest
and it will be used to do per-interface DNS lookup if it's
not empty. Could you give me some comments regarding the DOM part?

Thanks!

Comment 30

[Henry Chang [:hchang]]

Comment on attachment 8482144 [details] [diff] [review]
Part 3: Make use of gonk-specific per-interface DNS lookup.

Hi Patrick,

I implement my idea mentioned above:

1) Populate 'networkInterfaceId 'rather than ip to SocketTransport
2) For gonk, 'networkInterfaceId' will be used to do a per-interface
   DNS lookup with nsINetworkService.resolveHostForInterface()
3) 'networkInterfaceId' can be interpreted differently on other platform.

What do you think of this approach? Thanks!

Comment 31

[Patrick McManus [:mcmanus]]

Comment on attachment 8482144 [details] [diff] [review]
Part 3: Make use of gonk-specific per-interface DNS lookup.

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

It seems you have re-invented your own dns resolution interface to go from name to ip? We've already got one of those :)

Your prior patch passed in an explicit route for the transaction that worked cross platform. That was reasonable enough.. this one passes in an alternate name that is looked up via an alternate resolution system that only works on gonk.. I don't like that as much :)

If you need to map from names to IPs at a later stage, like DNS does, then use DNS - change things to pass in your own instance of a nsIDNSService rather than creating this new gonk only interface.

Either way is going to involve a round trip through the main thread which is going to slow things down considerably for your use case unless you do your dns service in c++. If possible I would go back to the prior approach of an explicit route that is either a dotted decimal or a normal dns name.

Comment 32

[Henry Chang [:hchang]]

Thanks for the feedback!

(In reply to Patrick McManus [:mcmanus] from comment #31)
> Comment on attachment 8482144 [details] [diff] [review]
> Part 3: Make use of gonk-specific per-interface DNS lookup.
> 
> Review of attachment 8482144 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It seems you have re-invented your own dns resolution interface to go from
> name to ip? We've already got one of those :)
> 

Yes and no. The new interface I re-invented requires additional
information to do the DNS lookup, which is pretty gonk specific.
That's why I didn't modify the original nsIDNSService since
it would be only meaningful for gonk.

> Your prior patch passed in an explicit route for the transaction that worked
> cross platform. That was reasonable enough.. this one passes in an alternate
> name that is looked up via an alternate resolution system that only works on
> gonk.. I don't like that as much :)
> 
> If you need to map from names to IPs at a later stage, like DNS does, then
> use DNS - change things to pass in your own instance of a nsIDNSService
> rather than creating this new gonk only interface.
> 
> Either way is going to involve a round trip through the main thread which is
> going to slow things down considerably for your use case unless you do your
> dns service in c++. If possible I would go back to the prior approach of an
> explicit route that is either a dotted decimal or a normal dns name.

Implementing it in c++ is not difficult since the original implementation
is already native. Suppose now my native DNS service is created then we 
have two approaches based on the assumption:

1) Use this DNS service as a hook through entire call path:
   (XMLHttpRequest->nsHttpChannel->nsHttpConnectionInfo->nsHttpConnectionMgr
    ->nsSocketTransport)

2) Still pass 'networkInterfaceId' but use this id to obtain
   the customized and gonk-only DNS service which inherits nsIDNSService.
   For example:

nsCOMPtr<nsIDNSService> dns;
if (mNetworkInterfaceId.IsEmpty()) {
    do_GetService(kDNSServiceCID, &rv);
} else {
    do_GetService(kMyDNSServiceCID, &rv);
}

By the way, I am also good for the explicit route approach.
ni bevis to see if he has any concern regarding this two approaches.

Thanks!

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8482141 [details] [diff] [review]
Part 1: Add attribute 'networkInterfaceId' to XMLHttpRequest

This seems fine if you document in the IDL what this thing does.

Comment 34

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Henry Chang [:henry] from comment #32)
> Thanks for the feedback!
> 
> (In reply to Patrick McManus [:mcmanus] from comment #31)
> > Comment on attachment 8482144 [details] [diff] [review]
> > Part 3: Make use of gonk-specific per-interface DNS lookup.
> > 
> > Review of attachment 8482144 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > It seems you have re-invented your own dns resolution interface to go from
> > name to ip? We've already got one of those :)
> > 
> 
> Yes and no. The new interface I re-invented requires additional
> information to do the DNS lookup, which is pretty gonk specific.
> That's why I didn't modify the original nsIDNSService since
> it would be only meaningful for gonk.
> 
> > Your prior patch passed in an explicit route for the transaction that worked
> > cross platform. That was reasonable enough.. this one passes in an alternate
> > name that is looked up via an alternate resolution system that only works on
> > gonk.. I don't like that as much :)
> > 
> > If you need to map from names to IPs at a later stage, like DNS does, then
> > use DNS - change things to pass in your own instance of a nsIDNSService
> > rather than creating this new gonk only interface.
> > 
> > Either way is going to involve a round trip through the main thread which is
> > going to slow things down considerably for your use case unless you do your
> > dns service in c++. If possible I would go back to the prior approach of an
> > explicit route that is either a dotted decimal or a normal dns name.
> 
> Implementing it in c++ is not difficult since the original implementation
> is already native. Suppose now my native DNS service is created then we 
> have two approaches based on the assumption:
> 
> 1) Use this DNS service as a hook through entire call path:
>    (XMLHttpRequest->nsHttpChannel->nsHttpConnectionInfo->nsHttpConnectionMgr
>     ->nsSocketTransport)
> 
> 2) Still pass 'networkInterfaceId' but use this id to obtain
>    the customized and gonk-only DNS service which inherits nsIDNSService.
>    For example:
> 

2 approaches are acceptable to me.
I prefer 2) because it's is a light-way approach compared to 1).

> nsCOMPtr<nsIDNSService> dns;
> if (mNetworkInterfaceId.IsEmpty()) {
>     do_GetService(kDNSServiceCID, &rv);
> } else {
>     do_GetService(kMyDNSServiceCID, &rv);
> }
> 
> By the way, I am also good for the explicit route approach.
> ni bevis to see if he has any concern regarding this two approaches.
> 
My concern of explicit route approach is that it will more complicated to the caller when proxy is taken into account. :(

> Thanks!

Comment 35

[Henry Chang [:hchang]]

Thanks! Bevis!

Patrick,

What do you think of my two approaches at comment 32?
I would go for explicit route at this time if both of 
them (comment 32) are not good to you.

Comment 36

[Patrick McManus [:mcmanus]]

(In reply to Henry Chang [:henry] from comment #35)
> Thanks! Bevis!
> 
> Patrick,
> 
> What do you think of my two approaches at comment 32?
> I would go for explicit route at this time if both of 
> them (comment 32) are not good to you.

I actually would prefer the explict route approach because I think that's a concept we need independent of this work.. so the dovetail is strong.

Comment 37

[Henry Chang [:hchang]]

(In reply to Patrick McManus [:mcmanus] from comment #36)
> (In reply to Henry Chang [:henry] from comment #35)
> > Thanks! Bevis!
> > 
> > Patrick,
> > 
> > What do you think of my two approaches at comment 32?
> > I would go for explicit route at this time if both of 
> > them (comment 32) are not good to you.
> 
> I actually would prefer the explict route approach because I think that's a
> concept we need independent of this work.. so the dovetail is strong.

Okay I got it! I will go for explicit route approach. Thanks!

Comment 38

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry] from comment #37)
> (In reply to Patrick McManus [:mcmanus] from comment #36)
> > (In reply to Henry Chang [:henry] from comment #35)
> > > Thanks! Bevis!
> > > 
> > > Patrick,
> > > 
> > > What do you think of my two approaches at comment 32?
> > > I would go for explicit route at this time if both of 
> > > them (comment 32) are not good to you.
> > 
> > I actually would prefer the explict route approach because I think that's a
> > concept we need independent of this work.. so the dovetail is [e-mail]
> 
> Okay I got it! I will go for explicit route approach. Thanks!

By the way, since DNS lookup may have multiple answers, what if
we use explicit route as a nsIDNSRecord to be able get recovered from
error [1] ?

[1] http://hg.mozilla.org/mozilla-central/file/3b7921328fc1/netwerk/base/src/nsSocketTransport2.cpp#l1462

Comment 39

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Henry Chang [:henry] from comment #38)
> 
> By the way, since DNS lookup may have multiple answers, what if
> we use explicit route as a nsIDNSRecord to be able get recovered from
> error [1] ?

This makes sense to the caller.
MmsService can provide the DNSRecord queried from DNSService with specified networkInterface to request  explicit route in the upcoming HTTPRequest.

> 
> [1]
> http://hg.mozilla.org/mozilla-central/file/3b7921328fc1/netwerk/base/src/
> nsSocketTransport2.cpp#l1462

Comment 40

[Patrick McManus [:mcmanus]]

(In reply to Henry Chang [:henry] from comment #38)
> (In reply to Henry Chang [:henry] from comment #37)
> > (In reply to Patrick McManus [:mcmanus] from comment #36)
> > > (In reply to Henry Chang [:henry] from comment #35)
> > > > Thanks! Bevis!
> > > > 
> > > > Patrick,
> > > > 
> > > > What do you think of my two approaches at comment 32?
> > > > I would go for explicit route at this time if both of 
> > > > them (comment 32) are not good to you.
> > > 
> > > I actually would prefer the explict route approach because I think that's a
> > > concept we need independent of this work.. so the dovetail is [e-mail]
> > 
> > Okay I got it! I will go for explicit route approach. Thanks!
> 
> By the way, since DNS lookup may have multiple answers, what if
> we use explicit route as a nsIDNSRecord to be able get recovered from
> error [1] ?
> 
> [1]
> http://hg.mozilla.org/mozilla-central/file/3b7921328fc1/netwerk/base/src/
> nsSocketTransport2.cpp#l1462

I'm a little confused - you're saying if the default route fails then use the explicit route as the backup (and just use the existing dns record as the interface somehow?)

maybe you're saying something else.. I'd rather not overload the concepts.

Comment 42

[Henry Chang [:hchang]]

(In reply to Patrick McManus [:mcmanus] from comment #40)
> (In reply to Henry Chang [:henry] from comment #38)
> > (In reply to Henry Chang [:henry] from comment #37)
> > > (In reply to Patrick McManus [:mcmanus] from comment #36)
> > > > (In reply to Henry Chang [:henry] from comment #35)
> > > > > Thanks! Bevis!
> > > > > 
> > > > > Patrick,
> > > > > 
> > > > > What do you think of my two approaches at comment 32?
> > > > > I would go for explicit route at this time if both of 
> > > > > them (comment 32) are not good to you.
> > > > 
> > > > I actually would prefer the explict route approach because I think that's a
> > > > concept we need independent of this work.. so the dovetail is [e-mail]
> > > 
> > > Okay I got it! I will go for explicit route approach. Thanks!
> > 
> > By the way, since DNS lookup may have multiple answers, what if
> > we use explicit route as a nsIDNSRecord to be able get recovered from
> > error [1] ?
> > 
> > [1]
> > http://hg.mozilla.org/mozilla-central/file/3b7921328fc1/netwerk/base/src/
> > nsSocketTransport2.cpp#l1462
> 
> I'm a little confused - you're saying if the default route fails then use
> the explicit route as the backup (and just use the existing dns record as
> the interface somehow?)
> 
> maybe you're saying something else.. I'd rather not overload the concepts.

Sorry for the confusion. I am saying the use of explicit route.
When user is going to use explicit route, he will do a DNS query 
on his own prior to using XMLHttpRequest. The DNS query will be done
by calling a gonk-specific function |android_getaddrinfoforiface|.
However, the queried result may contain multiple ips. 

Looking at the use of DNS query result, nsSocketTransport2.cpp
will iteratively try each result until success. (If I understand correctly)

We are wondering if we need to extend the concept of 'explicit route'
to 'available explicit routes' so that the user doesn't need to
try each ip on his own.

Thanks!

Comment 43

[Patrick McManus [:mcmanus]]

This starts to get complicated, right? The existing code not only tries the alternate names, but it marks the failed ones as bad so they aren't used again (until they are all marked bad, and then it starts over)

if you wanted to provide a nsidnsrecrord interface with the explicit route to accomodate that it makes sense.. I think you should probably provide it in addition to a string which is resolved in the usual way. (i.e. it could just be a dotted address pre-resolved, or it could be some other user of an explicit route that has a hostname in mind it wants resolved with normal dns)

Comment 44

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Patrick McManus [:mcmanus] from comment #43)
> This starts to get complicated, right? The existing code not only tries the
> alternate names, but it marks the failed ones as bad so they aren't used
> again (until they are all marked bad, and then it starts over)
> 
> if you wanted to provide a nsidnsrecrord interface with the explicit route
> to accomodate that it makes sense.. I think you should probably provide it
> in addition to a string which is resolved in the usual way. (i.e. it could
> just be a dotted address pre-resolved, or it could be some other user of an
> explicit route that has a hostname in mind it wants resolved with normal dns)

If this becomes complicated from both of yours viewpoints,
will you re-consider the 2) approach in comment 32?
"
    2) pass 'networkInterfaceId' but use this id to obtain
       the customized and gonk-only DNS service which inherits nsIDNSService.
       nsCOMPtr<nsIDNSService> dns;
       if (mNetworkInterfaceId.IsEmpty()) {
           do_GetService(kDNSServiceCID, &rv);
       } else {
           do_GetService(kMyDNSServiceCID, &rv);
       }
"
Then, all this will be handled between the enhanced DNSService and SocketTransport. :)

Comment 45

[Patrick McManus [:mcmanus]]

sorry this dropped off my radar - i just found the ni in a bugzilla report.

so the proposal is to pass an explicit route and if the explicit route is found in the socket transport code to resolve the explicit route with an alternative nsidnsservice?

can we do that in a way that isn't gonk specific (even if only gonk provides the alternative nsidnssservice)

Comment 46

[Henry Chang [:hchang]]

Hi Patrick,

Thanks for the response!

(In reply to Patrick McManus [:mcmanus] from comment #45)
> sorry this dropped off my radar - i just found the ni in a bugzilla report.
> 
> so the proposal is to pass an explicit route and if the explicit route is
> found in the socket transport code to resolve the explicit route with an
> alternative nsidnsservice?
> 

Interpreting the "explicit route" above as a "network interface identifier"
and then it's the proposal! So the user can issue a XMLHttpRequest with or
without a network interface id. While issuing with a network interface id,
SocketTransport will to use a "special" DNS service to resolve.

> can we do that in a way that isn't gonk specific (even if only gonk provides
> the alternative nsidnssservice)

I cannot come up with a way to do this not gonk specifically ...
Bevis, do you have any idea?

Comment 47

[Henry Chang [:hchang]]

Per discussion in the Portland work week, I will be going for annotating 
something like "network identifier" to SocketTransport and doing the 
per-interface DNS query.

Comment 48

[Dragana Damjanovic [:dragana]]

I have opened a new bug (bug 1108957) to extend dns host resolver to hand the case specific to gonk.

Comment 49

[Henry Chang [:hchang]]

(In reply to Dragana Damjanovic from comment #48)
> I have opened a new bug (bug 1108957) to extend dns host resolver to hand
> the case specific to gonk.

Hi Dragana,

Do you have any doc or pesudo code to demonstrate what is intended
to implement in bug 1108957? My current implementation won't explicitly
create a dns host resolver. Instead, it will directly use existing
android function call.

Thanks :)

Comment 50

[Dragana Damjanovic [:dragana]]

I want to make dns requests for gonk be as any other dns request, so that they are cached as any other. This is all in DNSService and nsHostResolver.

So you will need to change just one line:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsSocketTransport2.cpp#1049

I have not decides yet, but this will probably look like:
rv = dns->AsyncResolve2(SocketHost(), dnsFlags, networkInterface, this, nullptr, getter_AddRefs(mDNSRequest));

I will decide this today or tomorrow.

So you do not have to take care of calling android function and parsing results and in this way the dns request will behave as any other dns request.

Comment 51

[Henry Chang [:hchang]]

Attachment: Part 1: Add attribute 'networkInterfaceId' to XMLHttpRequest

Comment 52

[Henry Chang [:hchang]]

Attachment: Part 2: Populate networkInterfaceId to SocketTransport.

Comment 53

[Henry Chang [:hchang]]

Attachment: Part 3: Make use of gonk-specific per-interface DNS lookup.

Comment 54

[Henry Chang [:hchang]]

Attachment: Part 3: Make use of gonk-specific per-interface DNS lookup.

Comment 55

[Henry Chang [:hchang]]

Attachment: Part 3: Make use of gonk-specific per-interface DNS lookup.

Comment 56

[Henry Chang [:hchang]]

Attachment: Example.patch

Comment 57

[Henry Chang [:hchang]]

Comment on attachment 8534216 [details] [diff] [review]
Part 2: Populate networkInterfaceId to SocketTransport.

Hi Patrick,

Per discussion in Portland, I will go for the 'netowkr interface id' approach.

Part 2 has nothing dependent on gonk. It simply annotates a general 'network interface id' to SocketTransport. 

Part 3 will be the actual patch to make use of the id. The only gonk-specific
section is nsINetworkService. nsINetworkService is now gonk only interface
and offers a couple of networking utilities on gonk. In gonk, many networking
utilities are delegated to netd, which is a native process. The implementation
of nsINetworkService.resolveHostForInterface will be in bug 1109452. 

Thanks1

Comment 58

[Patrick McManus [:mcmanus]]

Comment on attachment 8534306 [details] [diff] [review]
Part 3: Make use of gonk-specific per-interface DNS lookup.

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

I think we just need to cleanup the threading and remove the gonk specific ifdefs

::: netwerk/base/public/nsISocketTransport.idl
@@ +43,5 @@
>       * For Unix domain sockets, this is zero.
>       */
>      readonly attribute long port;
>  
> +    attribute ACString networkInterfaceId;

comment

::: netwerk/base/src/nsSocketTransport2.cpp
@@ +1027,5 @@
>              return PostEvent(MSG_DNS_LOOKUP_COMPLETE, NS_OK, nullptr);
>          }
>      }
>  
> +#ifdef MOZ_WIDGET_GONK

I don't see any reason that any of this should be ifdef GONK. What am I missing?

@@ +1063,5 @@
>  
> +#ifdef MOZ_WIDGET_GONK
> +
> +nsresult
> +nsSocketTransport::ResolveHostForInterface(const nsACString& aNetworkInterfaceId)

this module uses type &var and type *var instead of type* var.. please use that throughout the patch - thanks

@@ +1075,5 @@
> +        {
> +        }
> +
> +        NS_IMETHODIMP Run()
> +        {

assert main thread

@@ +1130,5 @@
> +    nsCOMPtr<nsINetworkService> networkService =
> +        do_GetService("@mozilla.org/network/service;1", &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    mResolving = true;

so this is a problem.

the socket transport is normally a socket thread object.. and now you're running methods and accessing members on the main thread - that's a whole bunch of new race conditions to think about that haven't been an issue before.

I think its better if your new class just does the lookup and posts the result back to the socket thread where it can be integrated into the socket transport object.

@@ +1149,5 @@
> +nsSocketTransport::ResolveHostForInterfaceResult(bool success,
> +                                                 const char ** aIps,
> +                                                 uint32_t aNumOfIps)
> +{
> +    // flag host lookup complete for the benefit of the ResolveHost method.

I'm trying to figure out what thread this is.. please assert it. main thread I think?

It should really just an event to the socket thread before doing anything with the member object - which might simultaneously be in use on the socket thread. (so even mResolving change is not a great idea).

Comment 59

[Patrick McManus [:mcmanus]]

Comment on attachment 8534216 [details] [diff] [review]
Part 2: Populate networkInterfaceId to SocketTransport.

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

r+ if you'll address the comments below

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +415,5 @@
>  
>    bool                              mForcePending;
>    nsCOMPtr<nsIURI>                  mTopWindowURI;
> +
> +  nsCString                         mNetworkInterfaceId;

comment what this is

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +3107,5 @@
>      socketTransport->SetConnectionFlags(tmpFlags);
>  
>      socketTransport->SetQoSBits(gHttpHandler->GetQoSBits());
>  
> +    if (mEnt->mConnInfo->NetworkInterfaceId()) {

I would have to check, but I think that statement is always true. i.e. "" is a the initialized value.

@@ +3108,5 @@
>  
>      socketTransport->SetQoSBits(gHttpHandler->GetQoSBits());
>  
> +    if (mEnt->mConnInfo->NetworkInterfaceId()) {
> +        socketTransport->SetNetworkInterfaceId(nsDependentCString(mEnt->mConnInfo->NetworkInterfaceId()));

and likewise the ctor is kind of odd.

just expose the nsACString from conninfo so you can test .IsEmpty() and pass it directly.

::: netwerk/protocol/http/nsIHttpChannelInternal.idl
@@ +240,5 @@
>       * The URI of the top-level window that's associated with this channel.
>       */
>      readonly attribute nsIURI topWindowURI;
> +
> +    attribute ACString networkInterfaceId;

comment

Comment 60

[Henry Chang [:hchang]]

Thanks Patrick!

The reason of gonk specific ifdefs is 'nsINetworkService' is gonk only.
nsINetworkService provides a couple of gonk specific commands used by
B2G.

Regarding other comments, I will have it a look and fix it in the next
patch. Thanks!

(In reply to Patrick McManus [:mcmanus] from comment #58)
> Comment on attachment 8534306 [details] [diff] [review]
> Part 3: Make use of gonk-specific per-interface DNS lookup.
> 
> Review of attachment 8534306 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we just need to cleanup the threading and remove the gonk specific
> ifdefs
> 
> ::: netwerk/base/public/nsISocketTransport.idl
> @@ +43,5 @@
> >       * For Unix domain sockets, this is zero.
> >       */
> >      readonly attribute long port;
> >  
> > +    attribute ACString networkInterfaceId;
> 
> comment
> 
> ::: netwerk/base/src/nsSocketTransport2.cpp
> @@ +1027,5 @@
> >              return PostEvent(MSG_DNS_LOOKUP_COMPLETE, NS_OK, nullptr);
> >          }
> >      }
> >  
> > +#ifdef MOZ_WIDGET_GONK
> 
> I don't see any reason that any of this should be ifdef GONK. What am I
> missing?
> 
> @@ +1063,5 @@
> >  
> > +#ifdef MOZ_WIDGET_GONK
> > +
> > +nsresult
> > +nsSocketTransport::ResolveHostForInterface(const nsACString& aNetworkInterfaceId)
> 
> this module uses type &var and type *var instead of type* var.. please use
> that throughout the patch - thanks
> 
> @@ +1075,5 @@
> > +        {
> > +        }
> > +
> > +        NS_IMETHODIMP Run()
> > +        {
> 
> assert main thread
> 
> @@ +1130,5 @@
> > +    nsCOMPtr<nsINetworkService> networkService =
> > +        do_GetService("@mozilla.org/network/service;1", &rv);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +    mResolving = true;
> 
> so this is a problem.
> 
> the socket transport is normally a socket thread object.. and now you're
> running methods and accessing members on the main thread - that's a whole
> bunch of new race conditions to think about that haven't been an issue
> before.
> 
> I think its better if your new class just does the lookup and posts the
> result back to the socket thread where it can be integrated into the socket
> transport object.
> 
> @@ +1149,5 @@
> > +nsSocketTransport::ResolveHostForInterfaceResult(bool success,
> > +                                                 const char ** aIps,
> > +                                                 uint32_t aNumOfIps)
> > +{
> > +    // flag host lookup complete for the benefit of the ResolveHost method.
> 
> I'm trying to figure out what thread this is.. please assert it. main thread
> I think?
> 
> It should really just an event to the socket thread before doing anything
> with the member object - which might simultaneously be in use on the socket
> thread. (so even mResolving change is not a great idea).

Comment 61

[Patrick McManus [:mcmanus]]

(In reply to Henry Chang [:henry] from comment #60)
> Thanks Patrick!
> 
> The reason of gonk specific ifdefs is 'nsINetworkService' is gonk only.
> nsINetworkService provides a couple of gonk specific commands used by
> B2G.
> 

there is no reason this has to use nsINetworkService - it can use a new cross platform service. Its ok if only gonk provides that service right now.

Comment 62

[Patrick McManus [:mcmanus]]

wrt comment 50 - dragana seems to have a more general answer to this problem.. using the original patches's infrastructure for identifying an interface name, but incorportating the lookup that uses that name directly into the platform dns code on android.

this is nice as it uses the existing caching code and doesn't require bouncing back and forth across threads.

Can we get a unified patch?

Comment 63

[Henry Chang [:hchang]]

(In reply to Patrick McManus [:mcmanus] from comment #62)
> wrt comment 50 - dragana seems to have a more general answer to this
> problem.. using the original patches's infrastructure for identifying an
> interface name, but incorportating the lookup that uses that name directly
> into the platform dns code on android.
> 
> this is nice as it uses the existing caching code and doesn't require
> bouncing back and forth across threads.
> 
> Can we get a unified patch?

Hi Patrick,

I am good to use the new nsIDNSService API that dragana is going to implement.
However, I also hope the counterpart API, "binding name server to a interface", 
to be added to nsIDNSService as well. (Right now it's in nsINetworkService.setDNS and
is a gonk specific implementation as well)

dragana,

Have you decided to go for this approach? Are you available to implement it?
I am very willing to help you complete it. Thanks!

Comment 64

[Dragana Damjanovic [:dragana]]

I will leave setDNS function in nsINetworkService , because I am considering it a part of device configuration. An DNS resolver is only responsible for dns queries, it only calls gethostbyname or if there is a network interface specified and it is the gonk it calls android_getaddrinfoforiface.
Is this ok with your configuration? (Looking at your function resolveHostForInterface it only calls nsINetworkService that is only calling android_getaddrinfoforiface and transform results)

I could finish my part soon, but I need some more time to test it. If you have a quick test for this it will be great.

Comment 65

[Henry Chang [:hchang]]

(In reply to Dragana Damjanovic from comment #64)
> I will leave setDNS function in nsINetworkService , because I am considering
> it a part of device configuration. An DNS resolver is only responsible for
> dns queries, it only calls gethostbyname or if there is a network interface
> specified and it is the gonk it calls android_getaddrinfoforiface.
> Is this ok with your configuration? (Looking at your function
> resolveHostForInterface it only calls nsINetworkService that is only calling
> android_getaddrinfoforiface and transform results)
> 
> I could finish my part soon, but I need some more time to test it. If you
> have a quick test for this it will be great.

Hi dragana,

It sounds good! I am glad to test whatever you do. So we can be tracking 
bug 1108957 regarding the new added API on nsIDNSService.

Thanks!

Comment 66

[Dragana Damjanovic [:dragana]]

Attachment: Part 3: SocketTransport part - patch from bug 1108957 is needed

May I ask you to test this. I hope it is working properly.
Thank you.

Comment 67

[Dragana Damjanovic [:dragana]]

Attachment: Part 3: SocketTransport part - patch from bug 1108957 is needed

Only nits changed.

Comment 68

[Dragana Damjanovic [:dragana]]

May I ask you to test this.
Thanks!

Comment 69

[Henry Chang [:hchang]]

(In reply to Dragana Damjanovic from comment #68)
> May I ask you to test this.
> Thanks!

Sure! I will definitely test it and keep you posted!
Thanks!

Comment 70

[Henry Chang [:hchang]]

Hi :dragana,

Sorry I am currently busy porting some feature to the latest AOSP
and have no spare time testing your patch. Just let you know :)
Thanks :)

Comment 71

[Dragana Damjanovic [:dragana]]

Thanks!

Comment 72

[Henry Chang [:hchang]]

Attachment: Part 1: Add attribute 'networkInterfaceId' to XMLHttpRequest

Comment 73

[Henry Chang [:hchang]]

Attachment: Part 2: Populate networkInterfaceId to SocketTransport.

Comment 74

[Henry Chang [:hchang]]

Attachment: Part 3: Make use of gonk-specific per-interface DNS lookup.

Comment 75

[Henry Chang [:hchang]]

Attachment: Part 2: Populate networkInterfaceId to SocketTransport.

Comment 76

[Henry Chang [:hchang]]

Attachment: Part 3: Make use of gonk-specific per-interface DNS lookup.

Comment 77

[Henry Chang [:hchang]]

Attachment: Part 1: Add attribute 'networkInterfaceId' to XMLHttpRequest

Comment 78

[Henry Chang [:hchang]]

Attachment: Part 2: Populate networkInterfaceId to SocketTransport. (r+'d)

Comment 79

[Henry Chang [:hchang]]

Attachment: Part 3: Make use of gonk-specific per-interface DNS lookup.

Comment 80

[Henry Chang [:hchang]]

Comment on attachment 8571895 [details] [diff] [review]
Part 1: Add attribute 'networkInterfaceId' to XMLHttpRequest

Hi bz,

Could you please help this patch for enhancing XMLHttpRequest to have
a chrome-only attribute 'networkInterfaceId' to associate this request
with a specific network interface (based on the previous f+)?

Thanks!

Comment 81

[Henry Chang [:hchang]]

Hi Patrick,

Could you please have a review on attachment 8571897 [details] [diff] [review] which uses the new
interface 'nsIDNSService.asyncResolveExtended' to do the per-interface
DNS query feature? The real world use case would be in Bug 992772. Thanks!

Comment 82

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8571895 [details] [diff] [review]
Part 1: Add attribute 'networkInterfaceId' to XMLHttpRequest

>+nsXMLHttpRequest::PopulateNetworkInterfaceId(const nsACString& aNetworkInterfaceId)
>+{
>+  if (mNetworkInterfaceId.IsEmpty()) {

Why this mix of using the argument and the member?

Seems to me like this should just use the member and get rid of the argument.

>+  void PopulateNetworkInterfaceId(const nsACString& aNetworkInterfaceId);

Needs documentation.

>+  attribute ByteString? networkInterfaceId;

Likewise needs documentation.

Also, we're not going to need this in workers?

Comment 83

[Henry Chang [:hchang]]

(In reply to Not doing reviews right now from comment #82)
> Comment on attachment 8571895 [details] [diff] [review]
> Part 1: Add attribute 'networkInterfaceId' to XMLHttpRequest
> 
> >+nsXMLHttpRequest::PopulateNetworkInterfaceId(const nsACString& aNetworkInterfaceId)
> >+{
> >+  if (mNetworkInterfaceId.IsEmpty()) {
> 
> Why this mix of using the argument and the member?
> 
> Seems to me like this should just use the member and get rid of the argument.
> 

Yes you are right! I should use the member variable.

> >+  void PopulateNetworkInterfaceId(const nsACString& aNetworkInterfaceId);
> 
> Needs documentation.
> 
> >+  attribute ByteString? networkInterfaceId;
> 
> Likewise needs documentation.
> 
> Also, we're not going to need this in workers?

It would be only used by chrome process for now. Do you think that I should
consider the casein workers at this time?

Thanks!

Comment 84

[Boris Zbarsky [:bzbarsky]]

> Do you think that I should consider the casein workers at this time?

No, I just wanted to make sure we'd thought about the issue and were not implementing it there on purpose for now.  Sounds like we have and we are.

Comment 85

[Henry Chang [:hchang]]

Attachment: Part 1: Add attribute 'networkInterfaceId' to XMLHttpRequest

Comment 86

[Henry Chang [:hchang]]

Attachment: Part 1: Add attribute 'networkInterfaceId' to XMLHttpRequest

Comment 87

[Henry Chang [:hchang]]

Comment on attachment 8574986 [details] [diff] [review]
Part 1: Add attribute 'networkInterfaceId' to XMLHttpRequest

Hi bz,

I've addressed the comments in the previous review. Could you please review again? Thanks!

Comment 88

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8574986 [details] [diff] [review]
Part 1: Add attribute 'networkInterfaceId' to XMLHttpRequest

>+  // Populate the network interface id to the internal used HTTP channel.

How about:

  // Tell our channel what network interface ID we were told to use, if it's an
  // HTTP channel and we were told to use a non-default interface ID.

r=me

Comment 89

[Henry Chang [:hchang]]

Attachment: Part 1: Add attribute 'networkInterfaceId' to XMLHttpRequest (r+'d)

Comment 90

[Henry Chang [:hchang]]

Thanks bz! Followed you suggestion and re-attached the patch.

Comment 91

[Patrick McManus [:mcmanus]]

Comment on attachment 8571897 [details] [diff] [review]
Part 3: Make use of gonk-specific per-interface DNS lookup.

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

I'm sorry about the delay - I somehow lost track of this from my todo list in a merge conflict

::: netwerk/base/nsISocketTransport.idl
@@ +24,5 @@
>   * NOTE: Connection setup is triggered by opening an input or output stream,
>   * it does not start on its own. Completion of the connection setup is
>   * indicated by a STATUS_CONNECTED_TO notification to the event sink (if set).
>   *
>   * NOTE: This is a free-threaded interface, meaning that the methods on

note this comment

@@ +43,5 @@
>       * For Unix domain sockets, this is zero.
>       */
>      readonly attribute long port;
>  
> +    attribute ACString networkInterfaceId;

access to this has to be locked or at least documented when its ok to work read and write it

Comment 92

[Henry Chang [:hchang]]

Hi Patrick,

Thanks for the review!

The networkInterfaceId would only be accessed in the socket thread.
Can I add some assertion in the getter/setter just like what |keepaliveEnabled|
does?

Thanks!

(In reply to Patrick McManus [:mcmanus] from comment #91)
> Comment on attachment 8571897 [details] [diff] [review]
> Part 3: Make use of gonk-specific per-interface DNS lookup.
> 
> Review of attachment 8571897 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm sorry about the delay - I somehow lost track of this from my todo list
> in a merge conflict
> 
> ::: netwerk/base/nsISocketTransport.idl
> @@ +24,5 @@
> >   * NOTE: Connection setup is triggered by opening an input or output stream,
> >   * it does not start on its own. Completion of the connection setup is
> >   * indicated by a STATUS_CONNECTED_TO notification to the event sink (if set).
> >   *
> >   * NOTE: This is a free-threaded interface, meaning that the methods on
> 
> note this comment
> 
> @@ +43,5 @@
> >       * For Unix domain sockets, this is zero.
> >       */
> >      readonly attribute long port;
> >  
> > +    attribute ACString networkInterfaceId;
> 
> access to this has to be locked or at least documented when its ok to work
> read and write it

Comment 93

[Patrick McManus [:mcmanus]]

(In reply to Henry Chang [:henry] from comment #92)
> Hi Patrick,
> 
> Thanks for the review!
> 
> The networkInterfaceId would only be accessed in the socket thread.
> Can I add some assertion in the getter/setter just like what
> |keepaliveEnabled|
> does?
> 

yes.

please update the documentation to reflect the reality (both for your new member and KA). thanks!

Comment 94

[Henry Chang [:hchang]]

Attachment: Part 3: Make use of gonk-specific per-interface DNS lookup.

Comment 95

[Henry Chang [:hchang]]

Attachment: Part 3: Make use of gonk-specific per-interface DNS lookup.

Comment 96

[Henry Chang [:hchang]]

Comment on attachment 8585289 [details] [diff] [review]
Part 3: Make use of gonk-specific per-interface DNS lookup.

Hi Patrick,

Could you please review this patch again? I added documentation
to both KA and the new member I adds. Thanks!

Comment 97

[Henry Chang [:hchang]]

Attachment: Part 3: Make use of gonk-specific per-interface DNS lookup. (r+'d, rebased)

Comment 98

[Henry Chang [:hchang]]

Attachment: Part 2: Populate networkInterfaceId to SocketTransport. (r+'d, rebased)

Comment 99

[Henry Chang [:hchang]]

Attachment: Part 1: Add attribute 'networkInterfaceId' to XMLHttpRequest (r+'d, rebased)

Comment 100

[Henry Chang [:hchang]]

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

Comment 101

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/d7d77d26136d
https://hg.mozilla.org/integration/b2g-inbound/rev/15eb3e502463
https://hg.mozilla.org/integration/b2g-inbound/rev/0addaa4d5c19

Comment 102

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d7d77d26136d
https://hg.mozilla.org/mozilla-central/rev/15eb3e502463
https://hg.mozilla.org/mozilla-central/rev/0addaa4d5c19

Comment 103

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9193ddd0311f

Comment 104

[Henry Chang [:hchang]]

Attachment: Bug1053650_Part1_v2_2.patch

Comment 105

[Henry Chang [:hchang]]

Attachment: Bug1053650_Part2_v2_2.patch

Comment 106

[Henry Chang [:hchang]]

Attachment: Bug1053650_Part3_v2_2.patch

Comment 107

[Henry Chang [:hchang]]

Comment on attachment 8590245 [details] [diff] [review]
Bug1053650_Part1_v2_2.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 #): MMS
User impact if declined: MMS might not work in some cases
Testing completed: Yes
Risk to taking this patch (and alternatives if risky): No
String or UUID changes made by this patch: nsIHttpChannelInternal.idl, nsISocketTransport

Comment 108

[bhavana bajaj [:bajaj]]

(In reply to Henry Chang [:henry] from comment #107)
> Comment on attachment 8590245 [details] [diff] [review]
> Bug1053650_Part1_v2_2.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 #): MMS
> User impact if declined: MMS might not work in some cases
How often would a user hit this issue or can yo give more details here. This looks like a longstanding issue to me so I am not sure what the urgency is to uplift this on 2.2, so getting to understand the user impact and risk in detail will help here.
> Testing completed: Yes
> Risk to taking this patch (and alternatives if risky): No
> String or UUID changes made by this patch: nsIHttpChannelInternal.idl,
> nsISocketTransport

Comment 109

[Henry Chang [:hchang]]

(In reply to bhavana bajaj [:bajaj] from comment #108)
> (In reply to Henry Chang [:henry] from comment #107)
> > Comment on attachment 8590245 [details] [diff] [review]
> > Bug1053650_Part1_v2_2.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 #): MMS
> > User impact if declined: MMS might not work in some cases
> How often would a user hit this issue or can yo give more details here. This
> looks like a longstanding issue to me so I am not sure what the urgency is
> to uplift this on 2.2, so getting to understand the user impact and risk in
> detail will help here.
> > Testing completed: Yes
> > Risk to taking this patch (and alternatives if risky): No
> > String or UUID changes made by this patch: nsIHttpChannelInternal.idl,
> > nsISocketTransport

Since we have a 2.2 blocker (Bug 1152568) and this patch in conjunction with
Bug 992772 could fix Bug 1152568. They are all caused by the inability to do
per-interface DNS query.

Thanks!

Comment 110

[bhavana bajaj [:bajaj]]

Comment on attachment 8590245 [details] [diff] [review]
Bug1053650_Part1_v2_2.patch

approving for uplift as it helps a blocker.

Comment 111

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c09519f7f74c
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/4a91d189b58c
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/dc0fce2633dd