Use DNS.jsm for autoconfig MX lookups
Categories
(MailNews Core :: Account Manager, enhancement)
Tracking
(thunderbird_esr68 fixed, thunderbird68+)
People
(Reporter: clokep, Assigned: pmorris)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 3 obsolete files)
7.65 KB,
patch
|
Details | Diff | Splinter Review | |
9.63 KB,
patch
|
Details | Diff | Splinter Review | |
4.81 KB,
patch
|
mkmelin
:
review+
mkmelin
:
approval-comm-esr68+
|
Details | Diff | Splinter Review |
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
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
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.
Assignee | ||
Comment 6•5 years ago
|
||
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.
Assignee | ||
Comment 7•5 years ago
|
||
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
Assignee | ||
Comment 8•5 years ago
|
||
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. :)"
Updated•5 years ago
|
Comment 9•5 years ago
|
||
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
Updated•5 years ago
|
Comment 10•5 years ago
|
||
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•5 years ago
|
||
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
Assignee | ||
Comment 12•5 years ago
|
||
1 of 3. Thanks for the review. Added the parens as suggested.
Assignee | ||
Comment 13•5 years ago
|
||
2 of 3. With requested changes.
Assignee | ||
Comment 14•5 years ago
•
|
||
3 of 3. No changes here except rebasing on the new patches 1 and 2.
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 15•5 years ago
|
||
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•5 years ago
|
||
Pushed by mozilla@jorgk.com:
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•5 years ago
|
||
Please request uplift as you deem fit, maybe after fixing bug 1571076.
Updated•5 years ago
|
Comment 18•5 years ago
|
||
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.
Updated•5 years ago
|
Comment 19•4 years ago
|
||
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•4 years ago
|
||
Comment on attachment 9082626 [details] [diff] [review] part3-move-dns-js-1.patch (already on beta)
Comment 21•4 years ago
|
||
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•4 years ago
|
||
This appears to need a rebase for 68.
Comment 23•4 years ago
|
||
For reference: This needs to be backported together with bug 1573564.
Comment 24•4 years ago
|
||
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
Description
•