Closed Bug 1623398 Opened 5 months ago Closed 5 months ago

DNS.jsm should not return -1 on errors

Categories

(Thunderbird :: Account Manager, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 76.0

People

(Reporter: mkmelin, Assigned: mkmelin)

Details

Attachments

(1 file)

Found during debugging of account setup, DNS.jsm can return -1 and we don't handle that properly. It's not very nice to return mixed types.

Attachment #9134192 - Flags: review?(paul)
Comment on attachment 9134192 [details] [diff] [review]
bug1623398_DNS_minusone.patch

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

Nice catch, changes look fine.
Attachment #9134192 - Flags: review?(paul) → review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/3ac3b884565b
DNS.jsm should not return -1 on errors, just an empty array. r=pmorris

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Summary: DNS.jsm should not return -1 → DNS.jsm should not return -1 on errors
Target Milestone: --- → Thunderbird 76.0
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/fa2ff756eb23
followup - remove now irrelevant test. rs=bustage-fix DONTBUILD

I believe this is trying to differentiate between a valid response with no records and an invalid / error response. Are we sure we want to treat these the same?

Where do we not handle this properly? Is the bug in the handler, not in the DNS code?

That's what it was used for, but I do think wrongly so. If it was a case we want to handle it should have thrown an Error instead of returning a different type than expected.
For the one case we were prepared for the -1, the only difference it made was a log statement and for other parts ended up doing the same as an empty array. The case where we didn't handle it properly I found in the account config code: https://searchfox.org/comm-central/rev/6ead4697de9615629cc9065d161126ba2d1ea057/mail/components/accountcreation/content/fetchConfig.js#246,248. That could of course be handled differently if we wanted, but it seems like a needless complication.

Alright. Thanks for the explanation!

Oh, and many seemingly normal calls seem to return -1,

var {DNS} = ChromeUtils.import("resource:///modules/DNS.jsm");
DNS.mx("localhost").then(recs => {
alert(JSON.stringify(recs));
});

DNS.srv("thunderbird.net").then(recs => {
alert(JSON.stringify(recs));
});

You need to log in before you can comment on or make changes to this bug.