787369 - XMPP: Support SRV records

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

[Sven Schwedas]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.1 (KHTML, like Gecko) Chrome/21.0.1180.81 Safari/537.1

Steps to reproduce:

Create a new account on a server that uses SRV records to point to the actual XMPP server.

E.g.: 
Domain foo.bar
Account [e-mail]
Actual XMPP server resides under asdf-chatserver.foo.bar 


Actual results:

Server is not found automatically, has to be configured manually


Expected results:

Thunderbird queries SRV records ( http://wiki.xmpp.org/web/SRV_Records ) and automatically uses the correct server

Comment 1

[Jo Hermans]

That's being done in bug 14328.

Comment 2

[Sven Schwedas]

Ok, I assumed "general support for SRV records" and "actually using them in this case" would be two different tasks. :)

Comment 3

[Patrick Cloke [:clokep]]

(In reply to Sven Schwedas from comment #2)
> Ok, I assumed "general support for SRV records" and "actually using them in
> this case" would be two different tasks. :)
> 
> *** This bug has been marked as a duplicate of bug 14328 ***

I think that's valid. I'm not positive it'll just work once Gecko supports it.

Comment 4

[Florian Quèze [:florian]]

Supporting DNS SRV for XMPP will in most cases also require a way to set the expected hostname for the certificate check (bug 741550).

Comment 5

[Mark Banner (:standard8)]

Not tracking at the moment, this isn't going to happen for 17. Whilst we recognise it prevents access to some servers, I don't think this is something that we need to push through significantly faster.

Comment 6

[Marcel Waldvogel]

Thunderbird (and Mozilla in general) is all about the user, including relieving them from knowing nitty-gritty technical detail. The mail autoconfiguration was a big step forward, but why does service autoconfiguration seem to be so much of a nightmare? Every XMPP client out there supports SRV records, why can't Thunderbird? Please, guys, get these few lines into your code.

Comment 7

[Ben Bucksch (:BenB)]

Please see dependencies listed above.
This is not just a matter of a few lines, but implementing new Mozilla core functionality. Mozilla currently can't query DNS at all, apart from A/AAAA records. Plus, there's no API to determine which SSL hostname to check again.

Comment 8

[Ben Bucksch (:BenB)]

Correction: Plus, there's no API to tell PSM/NSS which SSL hostname to check against (which in this case isn't the hostname we physically connect to, due to SRV).

Comment 9

[Landry Breuil (:gaston)]

I know it sucks to just add a 'me too' here, but i just stumbled upon this w/ tb 31.3.0 - *if* the xmpp server is not listening on the @ record for domain.tld, one *needs* to set account.options.server pointing to the actual xmpp.domain.tld fqdn, otherwise chat will try to connect to the @ record.

Comment 10

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: patch

For the case of aResult is -1, I reviewed the libresolv implementation (https://github.com/lattera/glibc/blob/a2f34833b1042d5d8eeb263b4cf4caaea138c4ad/resolv/res_query.c#L323) and it seems that there's no way to know the error exactly, but we only know there is an error has occurred.

Comment 11

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: DNS.jsm

Fallen's code that I edited and used in the previous patch.

Comment 12

[aleth [:aleth]]

Comment on attachment 8745413 [details] [diff] [review]
patch

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

I think clokep knows a lot about DNS, so adding a r?.

Did you modify DNS.jsm at all? Anyway, Fallen might have some interesting input here.

Comment 13

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #12)
> Did you modify DNS.jsm at all?

No

Comment 14

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #13)
> (In reply to aleth [:aleth] from comment #12)
> > Did you modify DNS.jsm at all?
> 
> No

but in the patch is modified, also the lookup method of load_dnsapi, the condition of dnsStatus to check if the query succeeds, should return -1 to be consistent with lookup in load_libresolv unless dnsStatus has negative values to indicate error exactly (may be Fallen knows the expected return of DnsQuery_W)

Comment 15

[aleth [:aleth]]

Comment on attachment 8745413 [details] [diff] [review]
patch

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

Great progress!

::: chat/locales/en-US/xmpp.properties
@@ +12,5 @@
>  connection.initializingEncryption=Initializing encryption
>  connection.authenticating=Authenticating
>  connection.gettingResource=Getting resource
>  connection.downloadingRoster=Downloading contact list
> +connection.SRVLookup=Lookup a server

"Looking up the SRV record"

@@ +33,5 @@
>  connection.error.notSendingPasswordInClear=The server only supports authentication by sending the password in cleartext
>  connection.error.authenticationFailure=Authentication failure
>  connection.error.notAuthorized=Not authorized (Did you enter the wrong password?)
>  connection.error.failedToGetAResource=Failed to get a resource
> +connection.error.XMPPNotSupported=The server does not support XMPP

"This server..."

@@ +34,5 @@
>  connection.error.authenticationFailure=Authentication failure
>  connection.error.notAuthorized=Not authorized (Did you enter the wrong password?)
>  connection.error.failedToGetAResource=Failed to get a resource
> +connection.error.XMPPNotSupported=The server does not support XMPP
> +connection.error.SRVLookupFailed=Unable to lookup a server

"Unable to look up the server's SRV record"

@@ +35,5 @@
>  connection.error.notAuthorized=Not authorized (Did you enter the wrong password?)
>  connection.error.failedToGetAResource=Failed to get a resource
> +connection.error.XMPPNotSupported=The server does not support XMPP
> +connection.error.SRVLookupFailed=Unable to lookup a server
> +connection.error.loadResolvFailed=Unable to load open resolv library

I am not sure this one should be displayed to the user. When can it happen?

::: chat/modules/DNS.jsm
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +if (typeof Components !== "undefined") {
> +    Components.utils.import("resource://gre/modules/ctypes.jsm");

Nit: If there is no upstream to this library, it would be good to adjust the indentation to 2 spaces.

@@ +32,5 @@
> +                    lastException = ex;
> +                }
> +            }
> +            dump("#### Could not find libresolv in any of " + libnames + " " +
> +                 "Exception: " + lastException + "\n");

This should be a Cu.reportError(), and similarly elsewhere in this file.

::: chat/protocols/xmpp/xmpp.jsm
@@ +1206,5 @@
>      //FIXME if we have changed this._jid.resource, then this._jid.jid
>      // needs to be updated. This value is however never used because
>      // while connected it's the jid of the session that's interesting.
>  
> +    let account = this;

You don't need this helper variable if you use an arrow function for doConnect.

@@ +1215,5 @@
> +                        account.getString("connection_security"), account._jid,
> +                        account.imAccount.password, account);
> +    };
> +
> +    // User has specified a certain server or port, so there is no need for DNS SRV lookup.

If you want to check if the user has picked a specific value for a pref, use hasUserValue(), not the value of the pref.

@@ +1219,5 @@
> +    // User has specified a certain server or port, so there is no need for DNS SRV lookup.
> +    let serverPref = this.getString("server");
> +    let portPref = account.getInt("port");
> +    if (serverPref || portPref != 5222) {
> +      doConnect(account.getString("server"), account.getInt("port"));

account.getString("server") is serverPref.

@@ +1272,5 @@
> +      let record = this._SRVRecords.shift();
> +      doConnect(record.host, record.port);
> +    }).catch(function() {
> +      account.reportDisconnecting(Ci.prplIAccount.ERROR_OTHER_ERROR,
> +                                  _("connection.error.loadResolvFailed"));

This seems too specific an error message to me. There could have been some other error in the code covered by this catch.

Comment 16

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8745413 [details] [diff] [review]
patch

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

::: chat/modules/DNS.jsm
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +if (typeof Components !== "undefined") {
> +    Components.utils.import("resource://gre/modules/ctypes.jsm");

The calendar code I wrote this for is the upstream, but given you are closer to review I guess you got there first :-P

In the end I don't mind about the indent, but if we can get this into toolkit at some point that would be great!

@@ +6,5 @@
> +    Components.utils.import("resource://gre/modules/ctypes.jsm");
> +    Components.utils.import("resource://gre/modules/Services.jsm");
> +}
> +
> +const LOCATION = "resource:///modules/DNS.jsm";

This needs to be set to whereever the module actually ends up being. Is this the uri where DNS.jsm is available?

@@ +32,5 @@
> +                    lastException = ex;
> +                }
> +            }
> +            dump("#### Could not find libresolv in any of " + libnames + " " +
> +                 "Exception: " + lastException + "\n");

Note this code also runs in a worker, so you can't use Components.utils. If you want, you could rename it to log and then bind that to Cu.reportError if Components is available, falling back to dump otherwise.

::: chat/protocols/xmpp/xmpp.jsm
@@ +1237,5 @@
> +
> +    this.reportConnecting(_("connection.SRVLookup"));
> +
> +    // RFC 6120 (Section 3.2.1): SRV lookup.
> +    let lookupResult = DNS.srv("_xmpp-client._tcp." + this._jid.domain);

Not sure how it is meant to be for xmpp-client, but maybe if this yields no results, you could use Services.eTLD.getBaseDomain() to try the lookup on the base domain instead.

Comment 17

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Ah splinter review does not like me replying to a previous review. If my comments seem disconnected, please check comment 15 for what I was replying to.

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #14)
> (In reply to Abdelrhman Ahmed [:abdelrhman] from comment #13)
> > (In reply to aleth [:aleth] from comment #12)
> > > Did you modify DNS.jsm at all?
> > 
> > No
> 
> but in the patch is modified, also the lookup method of load_dnsapi, the
> condition of dnsStatus to check if the query succeeds, should return -1 to
> be consistent with lookup in load_libresolv unless dnsStatus has negative
> values to indicate error exactly (may be Fallen knows the expected return of
> DnsQuery_W)

The function is documented here:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms682016(v=vs.85).aspx

The error codes are from winerror.h, here are the DNS related ones:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms681391(v=vs.85).aspx

and I believe ERROR_SUCCESS is 0L. It would certainly be good to be consistent here, I didn't try this extensively on windows.

Comment 19

[Florian Quèze [:florian]]

It looks like as part of bug 1153212 (which I don't understand), a new nsIRoutedSocketTransportService interface was added: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsISocketTransportService.idl?rev=2124a617b3bc#122

Currently in socket.jsm we do:
  this.transport = socketTS.createTransport(...
It seems instead for the case using DNS SRV we could do:
  this.transport = socketTS.QueryInterface(Ci.nsIRoutedSocketTransportService)
                           .createRoutedTransport(...)

Comment 20

[aleth [:aleth]]

Comment on attachment 8745413 [details] [diff] [review]
patch

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +1207,5 @@
>      // needs to be updated. This value is however never used because
>      // while connected it's the jid of the session that's interesting.
>  
> +    let account = this;
> +    function doConnect(aServer = null, aPort = null) {

I'm not a fan of this helper function. It seems that for each connect() call, doConnect gets called exactly once, just before the return. So move it to the end and inline it instead.

@@ +1225,5 @@
> +    }
> +    else if (this._SRVLookup) {
> +      // Try next record, if we failed to connect to the previous one or lost
> +      // connection of the current record (reconnecting) and if we do not have
> +      // any record left, just connect to the domain.

So this exploits the automatic reconnection mechanism to try the various records. How many records typically exist? If more than one is common, I'm not sure whether it wouldn't be better to handle this in the prpl.

Comment 21

[aleth [:aleth]]

Comment on attachment 8745413 [details] [diff] [review]
patch

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +1234,5 @@
> +        doConnect();
> +      return;
> +    }
> +
> +    this.reportConnecting(_("connection.SRVLookup"));

Have you considered moving all this added SRV code to XMPPSession?

Comment 22

[Patrick Cloke [:clokep]]

Comment on attachment 8745413 [details] [diff] [review]
patch

You mentioned on IRC that a lot of changes were made to handle the cert stuff. I'll look at the next version!

Comment 23

[Abdelrahman Ahmed [:abdelrahman]]

Thanks for your reviews, some of questions here are discussed on IRC.

(In reply to Philipp Kewisch [:Fallen] from comment #16)
> This needs to be set to whereever the module actually ends up being. Is this
> the uri where DNS.jsm is available?
Yes

> Note this code also runs in a worker, so you can't use Components.utils. If
> you want, you could rename it to log and then bind that to Cu.reportError if
> Components is available, falling back to dump otherwise.
This does not work with me, can you give an example / clarification for this as it seems that I applied it wrongly?

(In reply to Philipp Kewisch [:Fallen] from comment #18)
> The error codes are from winerror.h, here are the DNS related ones:
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms681391(v=vs.85).
> aspx
> 
> and I believe ERROR_SUCCESS is 0L. It would certainly be good to be
> consistent here, I didn't try this extensively on windows.
Yes, you are right, but the errors are a lot, so it will be difficult to get the exact error.

(In reply to Florian Quèze [:florian] [:flo] from comment #19)
> It looks like as part of bug 1153212 (which I don't understand), a new
> nsIRoutedSocketTransportService interface was added:
Here is a good explanation from Patrick McManus (when I pinged him) that would be helpful
> the routedHost concept. That's the hostname of the TCP connection - but it might differ from the Origin name. The Origin name is the conceptual one for the service, its not necessarily a DNS entry (though it might be) and it is the only one ever given to PSM - because that's what you want to check against. When I speak of a "route" that's the routed host - how you connect to the origin. But its never the name of the origin, its just a convenient identifier.. like a CNAME provides a level of indirection.
> A proxy is another way to look at it. The proxy name is a route to use to reach the origin - but you always authenticate against the origin. In the case of a proxy the origin name might not even be in the DNS - the client would never know.
> a SRV is just like a CNAME except it is service specific. So I would expect it to work the same way - it is a route to the origin, never the origin itself. So don't give it to PSM.

Comment 24

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: patch

I haven't finished this patch yet, but your review is essential for the recent changes according to our discussions on IRC.

Comment 25

[aleth [:aleth]]

Comment on attachment 8747368 [details] [diff] [review]
patch

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

I only looked at xmpp-session for now and the approach you are taking looks fine to me.

I didn't tackle nits etc in this review as it's a WIP.

::: chat/protocols/xmpp/xmpp-session.jsm
@@ +47,5 @@
>    this._handlers = new Map();
>  
> +  // The User has not specified a certain server or port, so we should do
> +  // DNS SRV lookup.
> +  if (this._host != this._domain || aPort != 5222) {

See previous review comment.

@@ +69,5 @@
> +      // No records are found, so we will try to login with this domain.
> +      this._connect(this._host, this._port);
> +      return;
> +    }
> +    else if (aResult.length == 1 && aResult[0].host == ".") {

Add a reference to the spec explaining "." in a comment here

@@ +87,5 @@
> +    this._SRVRecords = aResult;
> +    this._SRVTryToConnect = true;
> +    this._connectNextRecord();
> +  }).catch(() => {
> +    this.ERROR("Unable to load resolv library");

Make sure you fix the error reporting inside DNS.jsm.

Here, you should do something like 
.catch(e => { this.ERROR("Error during SRV: " + e); ...
so as not to hide the actual error message.

@@ +141,5 @@
>    _security: null,
>    _encrypted: false,
>  
> +  _connect: function(aHost, aPort) {
> +    this._account.reportConnecting();

Move this to the constructor to avoid repeat calls.

@@ +144,5 @@
> +  _connect: function(aHost, aPort) {
> +    this._account.reportConnecting();
> +    try {
> +      this.connect(this._domain, this._port, this._security, null, aHost, aPort);
> +    } catch (e) {

I don't understand the need for this try/catch. I don't think IRC or Yahoo have it. Do you have any examples for when it is used?

If we can get rid of it, we can get rid of _connect.

@@ +145,5 @@
> +    this._account.reportConnecting();
> +    try {
> +      this.connect(this._domain, this._port, this._security, null, aHost, aPort);
> +    } catch (e) {
> +      Cu.reportError(e);

If you do need it, use this.ERROR, so it shows up in debug logs

@@ +147,5 @@
> +      this.connect(this._domain, this._port, this._security, null, aHost, aPort);
> +    } catch (e) {
> +      Cu.reportError(e);
> +      // We can't use _networkError because this._account._connection
> +      // isn't set until we return from the XMPPSession constructor.

Looks like you need to check this._SRVTryToConnect here as well?

Comment 27

[aleth [:aleth]]

Comment on attachment 8747368 [details] [diff] [review]
patch

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

::: chat/protocols/xmpp/xmpp-session.jsm
@@ +48,5 @@
>  
> +  // The User has not specified a certain server or port, so we should do
> +  // DNS SRV lookup.
> +  if (this._host != this._domain || aPort != 5222) {
> +    this._connect(this._host, this._port);

If the user specifies a server that differs from the domain, do you need to use the routing to avoid cert errors?

Comment 28

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #23)
> (In reply to Philipp Kewisch [:Fallen] from comment #16)

You should probably needinfo Fallen so that he sees your questions as this is quite a noisy bug ;)

Comment 29

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #10)
> For the case of aResult is -1, I reviewed the libresolv implementation
> (https://github.com/lattera/glibc/blob/
> a2f34833b1042d5d8eeb263b4cf4caaea138c4ad/resolv/res_query.c#L323) and it
> seems that there's no way to know the error exactly, but we only know there
> is an error has occurred.

It looks like h_errno is set. Not sure if it's worth retrieving that.

Comment 30

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #23)
> > Note this code also runs in a worker, so you can't use Components.utils. If
> > you want, you could rename it to log and then bind that to Cu.reportError if
> > Components is available, falling back to dump otherwise.
> This does not work with me, can you give an example / clarification for this
> as it seems that I applied it wrongly?
You did it correctly, just aleth's comment about using Cu.reportError will not work, because the code is also run in a worker. You can do this if aleth insists on using Cu.reportError:

let log;
if (typeof Components == "undefined") {
  log = dump;
} else {
  log = Components.utils.reportError.bind(Components.utils);
}

> 
> (In reply to Philipp Kewisch [:Fallen] from comment #18)
> > The error codes are from winerror.h, here are the DNS related ones:
> > https://msdn.microsoft.com/en-us/library/windows/desktop/ms681391(v=vs.85).
> > aspx
> > 
> > and I believe ERROR_SUCCESS is 0L. It would certainly be good to be
> > consistent here, I didn't try this extensively on windows.
> Yes, you are right, but the errors are a lot, so it will be difficult to get
> the exact error.
I think the important thing is making sure the success case is consistent. I haven't checked this, but it sounded to me like in one case the "success" case is -1, in the other case we have ERROR_SUCCESS which is 0. I didn't check the error case at all.

Comment 31

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #25)
> I don't understand the need for this try/catch. I don't think IRC or Yahoo
> have it. Do you have any examples for when it is used?

> Looks like you need to check this._SRVTryToConnect here as well?
It for this error (https://dxr.mozilla.org/comm-central/source/chat/modules/socket.jsm#142).
We do not need to check SRVTryToConnect as this error means we are offline (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIIOService).

(In reply to Philipp Kewisch [:Fallen] from comment #30)
> let log;
> if (typeof Components == "undefined") {
>   log = dump;
> } else {
>   log = Components.utils.reportError.bind(Components.utils);
> }
> 
Thanks :)

> I think the important thing is making sure the success case is consistent. I
> haven't checked this, but it sounded to me like in one case the "success"
> case is -1, in the other case we have ERROR_SUCCESS which is 0. I didn't
> check the error case at all.
OK, I will check that.

Comment 32

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #31)
> (In reply to aleth [:aleth] from comment #25)
> > I don't understand the need for this try/catch. I don't think IRC or Yahoo
> > have it. Do you have any examples for when it is used?
> 
> > Looks like you need to check this._SRVTryToConnect here as well?
> It for this error
> (https://dxr.mozilla.org/comm-central/source/chat/modules/socket.jsm#142).

That should never happen unless there is a bad bug in the code - we don't try to connect accounts while offline.

Comment 33

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #27)
If the user specifies a server that differs from the domain, do you need to
> use the routing to avoid cert errors?
Yes, you are right.

(In reply to Philipp Kewisch [:Fallen] from comment #30)
> I think the important thing is making sure the success case is consistent. I
> haven't checked this, but it sounded to me like in one case the "success"
> case is -1, in the other case we have ERROR_SUCCESS which is 0. I didn't
> check the error case at all.

I've checked that and I think they are consistent.

BTW, these links are useful
https://msdn.microsoft.com/en-us/library/windows/desktop/ms681382%28v=vs.85%29.aspx
ERROR_SUCCESS is 0.

https://github.com/Microsoft/Windows-classic-samples/tree/master/Samples/DNSAsyncQuery/cpp
This is an example of using DnsQuery from Microsoft.

Comment 34

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: patch

Comment 35

[aleth [:aleth]]

Comment on attachment 8747538 [details] [diff] [review]
patch

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

Partial review.

::: chat/modules/socket.jsm
@@ +138,5 @@
>     *****************************************************************************
>     */
>    // Synchronously open a connection.
> +  connect: function(aHostOrigin, aPortOrigin, aSecurity, aProxy,
> +                    aHost = null, aPort = null) {

Does aHost = aHostOrigin, aPort = aPortOrigin help?

Please add/update the documentation in the comments.

@@ +590,5 @@
> +    this.transport =
> +      socketTS.createRoutedTransport(this.security, this.security.length,
> +                                     this.hostOrigin, this.portOrigin,
> +                                     this.host, this.port,
> +                                     this.proxy);

Is there a cost to using RoutedTransport when it's not needed?

::: chat/protocols/xmpp/xmpp-session.jsm
@@ +54,2 @@
>      this.connect(this._host, this._port, this._security);
> +    return;

Check with flo about the original reason for the server preference. Should we use 'server' or the jid domain as the origin for the cert?

@@ +105,5 @@
>    connectTimeout: 60,
>    readWriteTimeout: 300,
> +
> +  // Contains the remaining SRV records if we failed to connect the current one.
> +  _SRVRecords: null,

maybe []

@@ +109,5 @@
> +  _SRVRecords: null,
> +
> +  // Used to indicate that we are still trying to connect to DNS SRV records or
> +  // not and its value will be false in the last record.
> +  _SRVTryToConnect: false,

Can't you use if(_SRVRecords.length) for this?

Comment 36

[Patrick Cloke [:clokep]]

Comment on attachment 8747538 [details] [diff] [review]
patch

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

::: chat/modules/socket.jsm
@@ +138,5 @@
>     *****************************************************************************
>     */
>    // Synchronously open a connection.
> +  connect: function(aHostOrigin, aPortOrigin, aSecurity, aProxy,
> +                    aHost = null, aPort = null) {

Please update any documentation in this file.

@@ +148,5 @@
>  
>      this.LOG("Connecting to: " + aHost + ":" + aPort);
> +    this.hostOrigin = aHostOrigin;
> +    this.portOrigin = aPortOrigin;
> +    if (aHost) {

What if aHost is set, but not aPort?

@@ +585,5 @@
>      this._resetBuffers();
>  
>      // Create a socket transport
>      let socketTS = Cc["@mozilla.org/network/socket-transport-service;1"]
> +                      .getService(Ci.nsIRoutedSocketTransportService);

Please update the documentation at the top of this file about this.

::: chat/protocols/xmpp/xmpp-session.jsm
@@ +49,3 @@
>  
> +  // The User has specified a certain server or port, so we should do
> +  // DNS SRV lookup.

Shouldn't this comment says "we should *NOT* do a DNS SRV lookup."?

@@ +56,5 @@
> +  }
> +
> +  // RFC 6120 (Section 3.2.1): SRV lookup.
> +  this._account.reportConnecting(_("connection.SRVLookup"));
> +  let lookupResult = DNS.srv("_xmpp-client._tcp." + this._domain);

No reason to save this, just inline the DNS.srv call.

@@ +57,5 @@
> +
> +  // RFC 6120 (Section 3.2.1): SRV lookup.
> +  this._account.reportConnecting(_("connection.SRVLookup"));
> +  let lookupResult = DNS.srv("_xmpp-client._tcp." + this._domain);
> +  lookupResult.then((aResult) => {

We should likely log the result no matter what.

@@ +70,5 @@
> +
> +    if (aResult.length == 0) {
> +      // RFC 6120 (Section 3.2.1): No records are found, so we will try to
> +      // login with this domain.
> +      this.connect(this._host, this._port, this._security);

I don't understand this comment. Do you mean "No SRV records found, try to login with the given domain name."?

@@ +74,5 @@
> +      this.connect(this._host, this._port, this._security);
> +      return;
> +    }
> +    else if (aResult.length == 1 && aResult[0].host == ".") {
> +      // Abort as the service is decidedly not available at this domain.

This is a misconfiguration of the account, right?

@@ +85,5 @@
> +    // Sort results: Lower priority is more preferred and higher weight is
> +    // more preferred in equal priorities.
> +    aResult.sort(function(a, b) {
> +      return a.prio - b.prio || b.weight - a.weight;
> +    });

I'd love to see some tests for this.

@@ +90,5 @@
> +
> +    this._SRVRecords = aResult;
> +    this._SRVTryToConnect = true;
> +    this._connectNextRecord();
> +  }).catch((e) => {

Doesn't this catch imply a coding error on our part?

@@ +105,5 @@
>    connectTimeout: 60,
>    readWriteTimeout: 300,
> +
> +  // Contains the remaining SRV records if we failed to connect the current one.
> +  _SRVRecords: null,

I'm not a big fan of the styling of these variables, use something like _srvRecords please.

@@ +109,5 @@
> +  _SRVRecords: null,
> +
> +  // Used to indicate that we are still trying to connect to DNS SRV records or
> +  // not and its value will be false in the last record.
> +  _SRVTryToConnect: false,

Can't you just check whether _srvRecords is truth-y?

@@ +145,5 @@
>    _security: null,
>    _encrypted: false,
>  
> +  _connectNextRecord: function() {
> +    if (!this._SRVTryToConnect)

I'd like to see some logging in this function, especially if it's called with SRVTryToConnect being false, doesn't that imply a coding error?

@@ +170,5 @@
>    },
>  
>    /* Report errors to the account */
>    onError: function(aError, aException) {
> +    if (this._SRVTryToConnect) {

If I'm connected to a server for 2 hours and then get disconnected, will this code run? I suspect you'll want to somehow demark when a successful XMPP negotiation was started and clear our the SRV information.

Comment 37

[aleth [:aleth]]

Comment on attachment 8747538 [details] [diff] [review]
patch

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

::: chat/protocols/xmpp/xmpp-session.jsm
@@ +60,5 @@
> +  let lookupResult = DNS.srv("_xmpp-client._tcp." + this._domain);
> +  lookupResult.then((aResult) => {
> +    if ((typeof aResult) == "number" && aResult == -1) {
> +      // RFC 6120 (Section 3.2.1): Error occurred while DNS SRV lookup
> +      // (e.g. user is offline)

I'm confused by these if clauses, maybe the comment should explain what you are checking for better. The spec says

If the initiating entity receives a response to its SRV query but it is not able to establish an XMPP connection using the data received in the response, it SHOULD NOT attempt the fallback process described in the next section (this helps to prevent a state mismatch between inbound and outbound connections).

If the initiating entity does *not* receive a response to its SRV query, it SHOULD attempt the fallback process described in the next section.

@@ +93,5 @@
> +    this._connectNextRecord();
> +  }).catch((e) => {
> +    this.ERROR("Error during SRV: " + e);
> +    this._account.reportDisconnecting(Ci.prplIAccount.ERROR_OTHER_ERROR,
> +                                      _("connection.error.SRVLookupFailed"));

Do we land in this catch if the SRV lookup fails, or only if there is some bug?

If the lookup fails, shouldn't we fall back to a normal connect?

Comment 38

[Kent James (:rkent)]

Comment on attachment 8747538 [details] [diff] [review]
patch

Although I am excited that you would land code to do a DNS SRV lookup, if it was my decision I would not allow a new file with as little documentation as that has. At the very least, the top of the file should say what the heck the module is supposed to do, and give some hints how to use it.

Comment 39

[Florian Quèze [:florian]]

Comment on attachment 8747538 [details] [diff] [review]
patch

Removing the f- that doesn't seem intended, as you say you are excited to see this, and are just requesting an additional comment.

Comment 40

[Kent James (:rkent)]

I just wanted to get your attention, so feedback+ is also OK if you just add a few comments!

Comment 41

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #35)
> >      this.connect(this._host, this._port, this._security);
> > +    return;
> 
> Check with flo about the original reason for the server preference. Should
> we use 'server' or the jid domain as the origin for the cert?

RFC6120 3.2.3 probably covers this, but it depends on why that pref exists.

Comment 42

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: patch

(In reply to aleth [:aleth] from comment #35)
> Is there a cost to using RoutedTransport when it's not needed?

Check this ;)
(https://dxr.mozilla.org/comm-central/source/mozilla/netwerk/base/nsSocketTransportService2.cpp?from=nsSocketTransportService%3A%3ACreateTransport#706)

(In reply to Patrick Cloke [:clokep] from comment #36)
> I don't understand this comment. Do you mean "No SRV records found, try to
> login with the given domain name."?

Yes

> This is a misconfiguration of the account, right?

Maybe, but this indicates that this domain does not support XMPP or has service for XMPP.

Most of other questions/requests are done and answered as a comment in this patch.

Comment 43

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: patch

Fixing some mistakes in previous patch in testing file.

Comment 44

[Patrick Cloke [:clokep]]

Comment on attachment 8748338 [details] [diff] [review]
patch

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

::: chat/protocols/xmpp/test/test_dnsSrv.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +* http://creativecommons.org/publicdomain/zero/1.0/ */

Please reformat this file using 2 space indent.

::: chat/protocols/xmpp/xmpp-session.jsm
@@ +74,5 @@
> +      this.ERROR("Error during SRV: " + e);
> +
> +      // Since we are not able to perform SRV lookup, we will attempt the
> +      // fallback process (use normal connect without SRV lookup).
> +      this.connect(this._host, this._port, this._security);

Above you connect with _host, _port, _security, null, _domain, _port. How come we don't use the _domain here?

@@ +136,5 @@
> +      this._account.reportDisconnected();
> +
> +      // Since we don't receive a response to SRV query, we SHOULD attempt the
> +      // fallback process (use normal connect without SRV lookup).
> +      this.connect(this._host, this._port, this._security);

Why are we calling reportDisconnecting/reportDisconnected above? I don't think you can call those and then continue connecting!

On second thought, would it make more sense to throw here and let the catch do the fallback connection?

@@ +139,5 @@
> +      // fallback process (use normal connect without SRV lookup).
> +      this.connect(this._host, this._port, this._security);
> +
> +      this.WARN("Error during SRV: Lookup failed.");
> +      return SRV_ERROR_LOOKUP_FAILED;

Is this only used in tests? That seems a bit strange. Is there a better way we can do this?

@@ +200,5 @@
>    },
>  
>    /* Report errors to the account */
>    onError: function(aError, aException) {
> +    if (this.srvTryToConnect) {

Put a comment above this saying what's happening ("If we're trying to connect to SRV entries, then keep trying until a successful connection occurs or we run out of SRV entries to try.")

@@ +284,5 @@
>      else
>        this.onXmppStanza = this.stanzaListeners.initStream;
> +
> +    // Clear SRV results since we have connected.
> +    this._srvRecords = [];

So if we reconnect at some point, do we do a DNS SRV request again?

Comment 45

[aleth [:aleth]]

Comment on attachment 8748338 [details] [diff] [review]
patch

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

Almost finished...

::: chat/modules/DNS.jsm
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// This module is responsible for performing DNS queries using ctypes for
> +// loading libraries on Linux, Mac and Windows.

Looks like there is a word missing here: "system DNS libraries"? or ...?

@@ +9,5 @@
> +  Components.utils.import("resource://gre/modules/ctypes.jsm");
> +  Components.utils.import("resource://gre/modules/Services.jsm");
> +}
> +
> +const LOCATION = "resource:///modules/DNS.jsm";

Top-level const is risky, please use var.

::: chat/protocols/xmpp/test/test_dnsSrv.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +* http://creativecommons.org/publicdomain/zero/1.0/ */

That's nice, but you can just use the standard MPL header.

@@ +10,5 @@
> +Services.scriptloader.loadSubScript("resource:///components/xmpp.js", xmpp);
> +
> +var xmppSession = {};
> +Services.scriptloader.loadSubScript("resource:///modules/xmpp-session.jsm",
> +																		xmppSession);

Something is wrong with tabs/spaces/indents in this file ;)

::: chat/protocols/xmpp/xmpp-session.jsm
@@ +53,3 @@
>  
> +  // The User has specified a certain server or port, so we should not do
> +  // DNS SRV lookup.

What was flo's answer about the original reason for the server pref?

@@ +54,5 @@
> +  // The User has specified a certain server or port, so we should not do
> +  // DNS SRV lookup.
> +  // RFC 6120 (Section 3.2.3): When Not to Use SRV.
> +  if ((this._account.prefs.prefHasUserValue("server") ||
> +      this._account.prefs.prefHasUserValue("port")) &&

nit: indent

@@ +64,5 @@
> +
> +  // RFC 6120 (Section 3.2.1): SRV lookup.
> +  this._account.reportConnecting(_("connection.srvLookup"));
> +  DNS.srv("_xmpp-client._tcp." + this._domain).then(aResult => {
> +      this.LOG("SRV lookup: " + JSON.stringify(aResult));

If you move this inside _handleSrvQuery you can save yourself this helper function.

@@ +136,5 @@
> +      this._account.reportDisconnected();
> +
> +      // Since we don't receive a response to SRV query, we SHOULD attempt the
> +      // fallback process (use normal connect without SRV lookup).
> +      this.connect(this._host, this._port, this._security);

One of these two sections must be wrong (don't reportDisconnecting if you then call connect()...)

@@ +138,5 @@
> +      // Since we don't receive a response to SRV query, we SHOULD attempt the
> +      // fallback process (use normal connect without SRV lookup).
> +      this.connect(this._host, this._port, this._security);
> +
> +      this.WARN("Error during SRV: Lookup failed.");

Better warn before calling connect() (which will also add log output)

@@ +155,5 @@
> +      this._account.reportDisconnecting(Ci.prplIAccount.ERROR_OTHER_ERROR,
> +                                        _("connection.error.XMPPNotSupported"));
> +      this._account.reportDisconnected();
> +
> +      this.LOG("SRV: XMPP is not supported on this domain.");

Better log before disconnecting.

@@ +170,5 @@
> +    // _connectNextRecord method.
> +    this._srvRecords = aResult.slice(0);
> +
> +    this._connectNextRecord();
> +    return aResult;

Is this return value only used in tests?

@@ +174,5 @@
> +    return aResult;
> +  },
> +
> +  _connectNextRecord: function() {
> +    if (!this.srvTryToConnect) {

The getter is nice, but I think if (!this._srvRecords.length) would actually be more readable.

@@ +176,5 @@
> +
> +  _connectNextRecord: function() {
> +    if (!this.srvTryToConnect) {
> +      this.WARN("_connectNextRecord is called and there are no more records " +
> +                "to connect.");

If this only happens if there is a bug in the code, it should be this.ERROR.

@@ +200,5 @@
>    },
>  
>    /* Report errors to the account */
>    onError: function(aError, aException) {
> +    if (this.srvTryToConnect) {

Same here.

Comment 46

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #45)
> > +/* Any copyright is dedicated to the Public Domain.
> > +* http://creativecommons.org/publicdomain/zero/1.0/ */
> 
> That's nice, but you can just use the standard MPL header.

Actually clokep informs me most tests are public domain, so leave it as it is :-)

Comment 47

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: patch

(In reply to Patrick Cloke [:clokep] from comment #44)
> So if we reconnect at some point, do we do a DNS SRV request again?
Yes,
In disconnect: https://dxr.mozilla.org/comm-central/source/chat/protocols/xmpp/xmpp.jsm#2159
In connect: https://dxr.mozilla.org/comm-central/source/chat/protocols/xmpp/xmpp.jsm#1203

(In reply to aleth [:aleth] from comment #45)
> ::: chat/protocols/xmpp/xmpp-session.jsm
> @@ +53,3 @@
> >  
> > +  // The User has specified a certain server or port, so we should not do
> > +  // DNS SRV lookup.
> 
> What was flo's answer about the original reason for the server pref?

In this patch, I made this condition according to RFC 6120 (Section 3.2.3) (and not sure), so I needinfo him.

Comment 48

[Florian Quèze [:florian]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #47)

> (In reply to aleth [:aleth] from comment #45)
> > ::: chat/protocols/xmpp/xmpp-session.jsm
> > @@ +53,3 @@
> > >  
> > > +  // The User has specified a certain server or port, so we should not do
> > > +  // DNS SRV lookup.
> > 
> > What was flo's answer about the original reason for the server pref?
> 
> In this patch, I made this condition according to RFC 6120 (Section 3.2.3)
> (and not sure), so I needinfo him.

Not sure what the question actually is, but here is some context about the server hostname pref:
- it was created mostly for backward compatibility with the libpurple behavior.
- it is/was useful for example for the Google Talk for Google Apps case, where you have the JID that is eg. [e-mail], and the DNS SRV record points to talk.google.com, and talk.google.com (which obviously can't own a valid SSL cert for the instantbird.fr domain) sends a talk.google.com cert. When we set the connect host name to talk.google.com, we check the server certificate against the hostname provided there. It's fine for security as the hostname is user-entered, not a string coming from the network.

Comment 49

[aleth [:aleth]]

(In reply to Florian Quèze [:florian] [:flo] from comment #48)
> > In this patch, I made this condition according to RFC 6120 (Section 3.2.3)
> > (and not sure), so I needinfo him.
> 
> Not sure what the question actually is, but here is some context about the
> server hostname pref:

I think the question was whether to do DNS SRV on the host set by the pref or not (the current patch does this only if the domain of the server pref doesn't match the JID domain), and what to expect for the cert.

Comment 50

[aleth [:aleth]]

Comment on attachment 8749424 [details] [diff] [review]
patch

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

::: chat/protocols/xmpp/xmpp-session.jsm
@@ +5,5 @@
>  this.EXPORTED_SYMBOLS = ["XMPPSession", "XMPPDefaultResource"];
>  
> +// DNS SRV errors in XMPP.
> +var SRV_ERROR_LOOKUP_FAILED = -1;
> +var SRV_ERROR_XMPP_NOT_SUPPORTED = -2;

Please move these above the function that uses them.

@@ +58,5 @@
> +  // RFC 6120 (Section 3.2.3): When Not to Use SRV.
> +  if (Services.prefs.getBoolPref("chat.dns.srv.disable") ||
> +      ((this._account.prefs.prefHasUserValue("server") ||
> +        this._account.prefs.prefHasUserValue("port")) &&
> +       Services.eTLD.getBaseDomainFromHost(this._host) == this._domain)) {

I'm not sure about this last line, given flo's comment?

@@ +66,5 @@
>    }
> +
> +  // RFC 6120 (Section 3.2.1): SRV lookup.
> +  this._account.reportConnecting(_("connection.srvLookup"));
> +  DNS.srv("_xmpp-client._tcp." + this._domain)

this._host?

@@ +71,5 @@
> +     .then(aResult => this._handleSrvQuery(aResult))
> +     .catch((e) => {
> +       // This error probably happens while loading libraries and declaring
> +       // methods for DNS (e.g. the user has a problem with a library or does not
> +       // have it).

For this to be true, this should be in the reject function in then(), not a chained catch.

@@ +77,5 @@
> +
> +       // Since we are not able to perform SRV lookup, we will attempt the
> +       // fallback process (use normal connect without SRV lookup).
> +       this.connect(this._host, this._port, this._security, null, this._domain,
> +                    this._port);

Does the fallback connect need the last two parameters?

@@ +134,5 @@
> +      // method of the library when it called, user is offline, etc.).
> +      // Since we don't receive a response to SRV query, we SHOULD attempt the
> +      // fallback process (use normal connect without SRV lookup).
> +      this.connect(this._host, this._port, this._security);
> +      throw SRV_ERROR_LOOKUP_FAILED;

So this throws... which will be caught above... and then we connect again.

@@ +141,5 @@
> +    this.LOG("SRV lookup: " + JSON.stringify(aResult));
> +    if (aResult.length == 0) {
> +      // RFC 6120 (Section 3.2.1) and RFC 2782 (Usage rules): No SRV records,
> +      // try to login with the given domain name.
> +      this.connect(this._host, this._port, this._security);

You could consider moving all these fallback connects into a chained catch() above.

@@ +151,5 @@
> +      // service is decidedly not available at this domain.
> +      this._account.reportDisconnecting(Ci.prplIAccount.ERROR_OTHER_ERROR,
> +                                        _("connection.error.XMPPNotSupported"));
> +      this._account.reportDisconnected();
> +      throw SRV_ERROR_XMPP_NOT_SUPPORTED;

Same here. Somehow the code flow has become confused.

::: im/app/profile/all-instantbird.js
@@ +338,5 @@
>  // a JS implementation. This is used to land JS-prpls pref'ed off in nightlies.
>  pref("chat.prpls.forcePurple", "prpl-jabber");
>  
> +// Disable DNS SRV part to use normal connect without SRV lookup and it's
> +// enabled by default.

"Whether to disable SRV lookups that use the system DNS library."

@@ +339,5 @@
>  pref("chat.prpls.forcePurple", "prpl-jabber");
>  
> +// Disable DNS SRV part to use normal connect without SRV lookup and it's
> +// enabled by default.
> +pref("chat.dns.srv.disable", false);

This pref should be in chat/

Comment 51

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: patch

(In reply to aleth [:aleth] from comment #49)
> (In reply to Florian Quèze [:florian] [:flo] from comment #48)
> > > In this patch, I made this condition according to RFC 6120 (Section 3.2.3)
> > > (and not sure), so I needinfo him.
> > 
> > Not sure what the question actually is, but here is some context about the
> > server hostname pref:
> 
> I think the question was whether to do DNS SRV on the host set by the pref
> or not (the current patch does this only if the domain of the server pref
> doesn't match the JID domain), and what to expect for the cert.

I think I understood Florian's example (maybe I understood it wrongly) and this domain (kingant.net) is a real example.
In this patch, I think it's handled?

Comment 52

[aleth [:aleth]]

Comment on attachment 8750043 [details] [diff] [review]
patch

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

::: chat/modules/DNS.jsm
@@ +28,5 @@
> +  _open: function() {
> +    function findLibrary() {
> +      let lastException = null;
> +      let libnames = [ctypes.libraryName("resolv.9"),
> +              ctypes.libraryName("resolv")];

nit: indent

@@ +37,5 @@
> +          lastException = ex;
> +        }
> +      }
> +      throw("Could not find libresolv in any of " + libnames + " Exception: " +
> +            lastException + "\n");

Please use throw new Error("..."), that way you include file/line number information. Same elsewhere in this file.

@@ +109,5 @@
> +      let hostbuf = ctypes.char.array(this.NS_MAXCDNAME)();
> +      let hostlen = this.dn_expand(answer.addressOfElement(0),
> +                     answer.addressOfElement(length),
> +                     answer.addressOfElement(idx + 6),
> +                     hostbuf, this.NS_MAXCDNAME);

Nit: indentation

@@ +140,5 @@
> +    let ancount = this.ns_get16(answer.addressOfElement(6));
> +
> +    for (let qdidx = 0; qdidx < qdcount && idx < length; qdidx++) {
> +      idx += this.dn_skipname(answer.addressOfElement(idx),
> +                  answer.addressOfElement(length)) + this.NS_QFIXEDSZ;

nit: indent

@@ +145,5 @@
> +    }
> +
> +    for (let anidx = 0; anidx < ancount && idx < length; anidx++) {
> +      idx += this.dn_skipname(answer.addressOfElement(idx),
> +                  answer.addressOfElement(length));

same here

@@ +235,5 @@
> +    if (typeId == NS_T_SRV) {
> +      let srvdata = ctypes.cast(data, this.DNS_SRV_DATA);
> +      return new SRVRecord(srvdata.wPriority, srvdata.wWeight,
> +                 srvdata.pNameTarget.readString(),
> +                 srvdata.wPort);

indent

@@ +254,5 @@
> +  lookup: function(name, typeId) {
> +    let queryResultsSet = this.PDNS_RECORD();
> +    let qname = ctypes.jschar.array()(name);
> +    let dnsStatus = this.DnsQuery_W(qname, typeId, this.DNS_QUERY_STANDARD,
> +                    null, queryResultsSet.address(), null);

indent

::: chat/protocols/xmpp/xmpp-session.jsm
@@ +53,5 @@
> +  // normal connect is set.
> +  // RFC 6120 (Section 3.2.3): When Not to Use SRV.
> +  if (Services.prefs.getBoolPref("chat.dns.srv.disable") ||
> +      (this._account.prefs.prefHasUserValue("server") ||
> +        this._account.prefs.prefHasUserValue("port"))) {

nit: indent

@@ +70,5 @@
> +      this.ERROR("Error during SRV: " + aError);
> +
> +      // Since we are not able to perform SRV lookup, we will attempt the
> +      // fallback process (use normal connect without SRV lookup).
> +      this.connect(this._host, this._port, this._security);

Looks like you don't need this reject function any more as the improved catch() below will take care of it ;)

@@ +72,5 @@
> +      // Since we are not able to perform SRV lookup, we will attempt the
> +      // fallback process (use normal connect without SRV lookup).
> +      this.connect(this._host, this._port, this._security);
> +  }).catch(aError => {
> +    if (aError == this.SRV_ERROR_XMPP_NOT_SUPPORTED) {

Here you should use === as aError may be an Error object, not a number.

@@ +82,5 @@
> +                                        _("connection.error.XMPPNotSupported"));
> +      this._account.reportDisconnected();
> +      return;
> +    }
> +    else if (aError == this.SRV_ERROR_LOOKUP_FAILED)

Same here.

@@ +83,5 @@
> +      this._account.reportDisconnected();
> +      return;
> +    }
> +    else if (aError == this.SRV_ERROR_LOOKUP_FAILED)
> +      this.WARN("Error during SRV: Lookup failed.");

Is this always due to an error? What kind of errors are possible for example?

@@ +85,5 @@
> +    }
> +    else if (aError == this.SRV_ERROR_LOOKUP_FAILED)
> +      this.WARN("Error during SRV: Lookup failed.");
> +    else
> +      this.ERROR("Unknown SRV error: " + aError);

Use
this.ERROR("Error during SRV lookup:", aError);
That way if aError is an Error object, the correct line number/file information will be displayed together with the caught error.

@@ +142,5 @@
> +
> +  // Handles result of DNS SRV query and returns sorted results if it's OK,
> +  // otherwise returns error.
> +  _handleSrvQuery: function(aResult) {
> +    if ((typeof aResult) == "number" && aResult == -1)

Why the brackets around typeof aResult?

@@ +149,5 @@
> +    this.LOG("SRV lookup: " + JSON.stringify(aResult));
> +    if (aResult.length == 0) {
> +      // RFC 6120 (Section 3.2.1) and RFC 2782 (Usage rules): No SRV records,
> +      // try to login with the given domain name.
> +      this.connect(this._host, this._port, this._security);

Don't you need to return here.

Comment 53

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: patch

Comment 54

[aleth [:aleth]]

Comment on attachment 8750113 [details] [diff] [review]
patch

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

::: chat/modules/DNS.jsm
@@ +37,5 @@
> +          lastException = ex;
> +        }
> +      }
> +      throw new Error("Could not find libresolv in any of " + libnames +
> +                      " Exception: " + lastException + "\n");

Check with Fallen that replacing dump() with throw is OK in a worker context. Lines 329-333 suggest "yes" but let's be sure.

::: chat/protocols/xmpp/test/test_dnsSrv.js
@@ +97,5 @@
> +    try {
> +      session._handleSrvQuery(currentQuery.input);
> +      equal(session._srvRecords.length, currentQuery.output.length);
> +        for (let index = 0; index < session._srvRecords.length; index++)
> +          deepEqual(session._srvRecords[index], currentQuery.output[index]);

Nit: indent

It would be nice if there were not so many nits ;)

Comment 55

[aleth [:aleth]]

Comment on attachment 8750113 [details] [diff] [review]
patch

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

::: chat/modules/DNS.jsm
@@ +296,5 @@
> +// applies method in asynchronous way.
> +function asyncify(method) {
> +  return function(...args) {
> +    return new Promise((resolve, reject) => {
> +      let worker = new ChromeWorker(LOCATION);

Would a PromiseWorker make this whole asyncify helper function much simpler?

@@ +299,5 @@
> +    return new Promise((resolve, reject) => {
> +      let worker = new ChromeWorker(LOCATION);
> +      worker.onmessage = function(event) {
> +        if (event.data.hasOwnProperty("type") && event.data.type == "error")
> +          reject(event.data.message);

Don't you want to pass the error object here?

@@ +304,5 @@
> +        else
> +          resolve(event.data);
> +      };
> +      worker.onerror = function(event) {
> +        reject(event.message);

Similarly, here.

Comment 56

[Florian Quèze [:florian]]

(In reply to aleth [:aleth] from comment #49)
> (In reply to Florian Quèze [:florian] [:flo] from comment #48)
> > > In this patch, I made this condition according to RFC 6120 (Section 3.2.3)
> > > (and not sure), so I needinfo him.
> > 
> > Not sure what the question actually is, but here is some context about the
> > server hostname pref:
> 
> I think the question was whether to do DNS SRV on the host set by the pref
> or not

No, if the connect hostname pref is set, the pref replaces the DNS SRV lookup.

For the cert, we should accept the cert of the hostname from the pref (if possible it would be nice to also accept the hostname in the JID, but I'm not sure that's realistically possible).

Comment 57

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: patch

(In reply to aleth [:aleth] from comment #54) 
> Check with Fallen that replacing dump() with throw is OK in a worker
> context. Lines 329-333 suggest "yes" but let's be sure.

Fallen, I hope this patch will be OK for these changes in DNS.jsm.

Comment 58

[aleth [:aleth]]

Comment on attachment 8750466 [details] [diff] [review]
patch

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

That indeed simplified asyncify and the error handling quite a bit!

::: chat/modules/DNS.jsm
@@ +294,5 @@
> +  this.data = data;
> +}
> +
> +// applies method in asynchronous way.
> +function asyncify(method) {

Looks like you can inline this now.

@@ +321,5 @@
> +    self.close();
> +  };
> +  self.addEventListener("message", msg => worker.handleMessage(msg));
> +
> +  function execute(aArgs) {

Maybe something like execute(aOS, aMethod, aArgs) instead of passing an object in post()?

Comment 59

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8750466 [details] [diff] [review]
patch

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

Changes are fine with me. I am horrified by the style of putting else on a new line after the closing bracket, but I guess that is debatable. There is still a mix though, make sure it is consistent.

::: chat/modules/DNS.jsm
@@ +239,5 @@
> +      let srvdata = ctypes.cast(data, this.DNS_SRV_DATA);
> +      return new SRVRecord(srvdata.wPriority, srvdata.wWeight,
> +                           srvdata.pNameTarget.readString(),
> +                           srvdata.wPort);
> +    } else if (typeId == NS_T_TXT) {

Like here

Comment 60

[Patrick Cloke [:clokep]]

Comment on attachment 8750466 [details] [diff] [review]
patch

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

This version looks great! I mostly had a few minor nits and some comments about the test!

::: chat/protocols/xmpp/test/test_dnsSrv.js
@@ +25,5 @@
> +    reportDisconnecting: function(aConnectionErrorReason,
> +                                  aConnectionErrorMessage) {},
> +    reportDisconnected: function() {}
> +  },
> +  _connectNextRecord: function() {},

I'd suggest adding a marker (either a boolean or a counter) to both reportDisconnected and _connectNextRecord so you can assert whether they were called or not.

@@ +30,5 @@
> +  LOG: function(aMsg) {},
> +  WARN: function(aMsg) {},
> +};
> +
> +var session = new FakeXMPPSession();

Is it more realistic to create a new session for each test?

@@ +32,5 @@
> +};
> +
> +var session = new FakeXMPPSession();
> +
> +var TEST_DATA = [

I kind of like putting a comment above each input/output pair that explicitly says what's being tested by that pair. :)

@@ +69,5 @@
> +    ]
> +  },
> +  {
> +    input: [],
> +    output:[]

Nit: Missing a space.

::: chat/protocols/xmpp/xmpp-session.jsm
@@ +61,5 @@
> +
> +  // RFC 6120 (Section 3.2.1): SRV lookup.
> +  this._account.reportConnecting(_("connection.srvLookup"));
> +  DNS.srv("_xmpp-client._tcp." + this._host).then(
> +    aResult => this._handleSrvQuery(aResult)

I think you can have:

DNS.srv("_xmpp-client._tcp" + this._host).then(this._handleSrvQuery).catch()

@@ +134,5 @@
> +  SRV_ERROR_LOOKUP_FAILED: -1,
> +  SRV_ERROR_XMPP_NOT_SUPPORTED: -2,
> +
> +  // Handles result of DNS SRV query and returns sorted results if it's OK,
> +  // otherwise returns error.

This now throws an error, it doesn't return it.

Comment 61

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: patch

(In reply to Patrick Cloke [:clokep] from comment #60) 
> I think you can have:
> 
> DNS.srv("_xmpp-client._tcp" + this._host).then(this._handleSrvQuery).catch()

No, as we use arrow to get benefits of "Lexical this".

Comment 62

[aleth [:aleth]]

Comment on attachment 8751456 [details] [diff] [review]
patch

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

::: chat/modules/DNS.jsm
@@ +335,5 @@
> +     * @param typeId        The RR type to look up as a constant.
> +     * @return              A promise resolved when completed.
> +     */
> +    lookup: function(name, typeId)
> +    {

Nit: move bracket to previous line

@@ +336,5 @@
> +     * @return              A promise resolved when completed.
> +     */
> +    lookup: function(name, typeId)
> +    {
> +      return new BasePromiseWorker(LOCATION).post(

You're sure this is evaluated as (new BasePromiseWorker(LOCATION)).post ?
Maybe it's more readable to do
let worker = new ...;
return worker.post(...)

Comment 63

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: patch

Comment 64

[aleth [:aleth]]

Comment on attachment 8752516 [details] [diff] [review]
patch

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

Looks good to me!

Comment 65

[Abdelrahman Ahmed [:abdelrahman]]

Do you have any thoughts about this discussion on IRC (http://log.bezut.info/instantbird/160505/#m192) which was about proxy settings?

Comment 66

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #65)
> Do you have any thoughts about this discussion on IRC
> (http://log.bezut.info/instantbird/160505/#m192) which was about proxy
> settings?

I don't know much about the instantbird side of things. Regarding SRV lookups, if the users has the "remote dns" option set, this code won't adhere to that. The SRV lookup is done directly through OS methods, which are not proxied. I'm not sure how to solve this, aside from maybe writing your own dns client in javascript and sending the request via the socks proxy. Maybe there is an easier/better solution.

Comment 67

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/041e7b96f670c0c4998bad7d3ceebe8acd963d18
Bug 787369 - XMPP: Support SRV records. r=aleth,clokep