1349337 - Use DNS.jsm for autoconfig MX lookups

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, enhancement)

Description

[Patrick Cloke [:clokep]]

Generally this would change the getMX record of fetchConfig.js [1] to use DNS.jsm [2] instead of calling out to a web service to do this lookup. This is an alternative fixing bug 545866.

[1] https://dxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/fetchConfig.js#190-240
[2] https://dxr.mozilla.org/comm-central/source/chat/modules/DNS.jsm

Comment 1

[Paul Morris [:pmorris]]

Links to the same code on searchfox.org:

[1] https://searchfox.org/comm-central/source/mail/components/accountcreation/content/fetchConfig.js#172-221
[2] https://searchfox.org/comm-central/source/chat/modules/DNS.jsm

(The dxr links didn't work for me today.)

Comment 2

[Ben Bucksch (:BenB)]

Pre-condition: Need to add type MX to the DNS.jsm file. Not just the constant, but also the StructType and the mapAnswer() code. Not hard, but needs to be added to the file.

Comment 3

[Ben Bucksch (:BenB)]

Note: Firefox goes the opposite route with DoH. Kind-of ironic.
So, an alternative to this bug would be to use the Cloudflare DoH service via HTTPS instead of the Thunderbird MX web service.

Comment 4

[Paul Morris [:pmorris]]

Attachment: part1-handle-mx-records-0.patch

Part 1 of 3.

Comment 5

[Paul Morris [:pmorris]]

Attachment: part2-use-dns-js-0.patch

Part 2 of 3.

Comment 6

[Paul Morris [:pmorris]]

Attachment: part3-move-dns-js-0.patch

Part 3 of 3.

Tested and works on linux. The implementation in DNS.js is different on windows and that is not tested, outside of this try server run, currently in progress:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=dc6d67603c94441655941d413078e55d91ea23c9

I could set up a windows build in a virtual machine, but it would be nice to avoid that if this already works on windows.

Comment 7

[Paul Morris [:pmorris]]

I did a try run to test this on windows, with an additional test commit to make sure the MX DNS lookup was used, and got failing tests.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=15b4345c4b39378c999b164815a3987afbc18cab

Comment 8

[Paul Morris [:pmorris]]

OK, this is indeed working on windows 10.

I set up a windows build on an older dual-boot laptop and it works fine on windows 10 when I test it manually with a gmail account (both with and without commenting out the code to make sure the "MX" code path is used).

I ran the mozmill tests locally, and the failures from the run in comment 7 were caused by the commit that commented out some code for testing purposes. (Note they are not failing in the run from comment 6.) These are:

• account/test-mail-account-setup-wizard.js mozmill-one
• account/test-retest-config.js mozmill-one
• instrumentation/test-instrument-setup.js mozmill-one

Also, clokep said on IRC: "pmorris: Didn't test those DNS changes, but they look sane. :)"

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9079727 [details] [diff] [review]
part1-handle-mx-records-0.patch

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

::: chat/modules/DNS.jsm
@@ +128,5 @@
> +                                   aAnswer.addressOfElement(aLength),
> +                                   aAnswer.addressOfElement(aIdx + 2),
> +                                   hostbuf,
> +                                   this.NS_MAXCDNAME);
> +      let host = hostlen > -1 ? hostbuf.readString() : null;

would put parenthesis around hostlen > -1

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9079728 [details] [diff] [review]
part2-use-dns-js-0.patch

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

Should also remove mailnews.mx_service_url: https://searchfox.org/comm-central/rev/72694e3d97e48704952acf28871e0b64a38081f9/mailnews/mailnews.js#916

Other than that, looks good, and seems to work just fine in my testing

::: mail/components/accountcreation/content/fetchConfig.js
@@ +197,5 @@
> +      const sortedRecs = filteredRecs.sort((a, b) => a.prio > b.prio);
> +      const firstHost = sortedRecs[0].host;
> +      successCallback(firstHost);
> +    } else {
> +      errorCallback(new Exception("no hostname found in MX records"));

Can you capitalize, and add " for sanitizedDomain=" + sanitizedDomain
to this message

Comment 11

[Magnus Melin [:mkmelin]]

Not this bug, but I'll note that the DNS.txt lookup doesn't seem to work generally.
var {DNS} = ChromeUtils.import("resource:///modules/DNS.jsm");
DNS.txt("thunderbird.net").then(recs => {
alert(JSON.stringify(recs));
});
-> JavaScript error: resource:///modules/DNS.jsm, line 122: TypeError: malformed UTF-8 character sequence at offset 68

Comment 12

[Paul Morris [:pmorris]]

Attachment: part1-handle-mx-records-1.patch

1 of 3. Thanks for the review. Added the parens as suggested.

Comment 13

[Paul Morris [:pmorris]]

Attachment: part2-use-dns-js-1.patch

2 of 3. With requested changes.

Comment 14

[Paul Morris [:pmorris]]

Attachment: part3-move-dns-js-1.patch

3 of 3. No changes here except rebasing on the new patches 1 and 2.

Comment 15

[Paul Morris [:pmorris]]

Per IRC discussion with mkmelin and jorgk, the plan is to let this change "go through beta and later uplift to esr. then we can turn off the server side code after 60 is EOL'd".

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/846643850d69
Let DNS.jsm handle MX records. r=mkmelin
https://hg.mozilla.org/comm-central/rev/91ec560b7e52
Use DNS.jsm to query MX records. r=mkmelin
https://hg.mozilla.org/comm-central/rev/150123a64fce
Move DNS.jsm to mail/base/modules. r=mkmelin

Comment 17

[Jorg K (CEST = GMT+2)]

Please request uplift as you deem fit, maybe after fixing bug 1571076.

Comment 18

[[e-mail]]

Comment on attachment 9082624 [details] [diff] [review]
part2-use-dns-js-1.patch

>-      fetch = fetchConfigForMX(domain,
>+      fetch = await fetchConfigForMX(domain,
>         call.successCallback(), call.errorCallback());
>       call.setAbortable(fetch);
The whole point of these calls is that they're asynchronous. In particular, it's important that this whole function executes in a single microtask; `PriorityAbortable` will get very confused if some tasks complete before you've even finished adding tasks to it.

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 9082626 [details] [diff] [review]
part3-move-dns-js-1.patch

We should take this on beta, and later esr too.

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 9082626 [details] [diff] [review]
part3-move-dns-js-1.patch

(already on beta)

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 9082626 [details] [diff] [review]
part3-move-dns-js-1.patch

Haven't seen any problems with this, and some testing Jörg did showed it worked better than the server side version  - at least for his test case.

Comment 22

[Geoff Lankow (:darktrojan)]

This appears to need a rebase for 68.

Comment 23

[Ben Bucksch (:BenB)]

For reference: This needs to be backported together with bug 1573564.

Comment 24

[Geoff Lankow (:darktrojan)]

68.3.1:
https://hg.mozilla.org/releases/comm-esr68/rev/7fa87379c921fbdeb1bf454d5d6cb932c6741dd5
https://hg.mozilla.org/releases/comm-esr68/rev/1ba122fd48eee117830a586cfaff9d362f91198c
https://hg.mozilla.org/releases/comm-esr68/rev/eb227f230f8063c5d9c4ac77f759fdcb4d4d8b80