1373640 - Add DNS resolution to webextensions api

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, enhancement, P2)

Description

[ashley]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/603.1.30 (KHTML, like Gecko) Version/10.1 Safari/603.1.30

Steps to reproduce:

I develop the extension SixOrNot. Part of the core functionality of this relies on being able to do DNS lookups in order to determine whether a given site is accessible via IPv6. Currently I'm using the Components.classes["@mozilla.org/network/dns-service;1"].getService(Components.interfaces.nsIDNSService); interface to perform DNS queries (well, actually mostly I'm using js-ctypes to use the OS-native mechanism to do this on Windows/OSX/Linux because it more reliably returns IPv6 addresses - but I know there's no chance that this amazingly powerful technology is going to survive the change to WebExtensions).

From what I can tell there's no mechanism to do DNS lookups via the WebExtensions API. Given how fundamental being able to do this is to what SixOrNot does I would like to request that some mechanism be added to permit this.

Comment 1

[Andrew Swan [:aswan]]

Another option is to create a native application that does DNS lookups and communicate with it using native messaging.
That requires an extra setup step for users of your extension though so it may not meet your needs but it is an option...

Comment 2

[Caitlin Neiman [:caitmuenster]]

Hi Ashley, this has been added to the agenda for the January 23, 2018 WebExtensions APIs triage. Would you be able to join us? 

Here’s a quick overview of what to expect at the triage: 

* We normally spend 5 minutes per bug
* The more information in the bug, the better
* The goal of the triage is to give a general thumbs up or thumbs down on a proposal; we won't be going deep into implementation details

Relevant Links: 

* Wiki for the meeting: https://wiki.mozilla.org/WebExtensions/Triage#Next_Meeting
* Meeting agenda: https://docs.google.com/document/d/1Mc0h5OVd30WBjCORssdifZRXYQrk6WcTqX1cw6ADO9k/edit#
* Vision doc for WebExtensions: https://wiki.mozilla.org/WebExtensions/Vision

Comment 3

[ashley]

Yes, I can join tomorrow :)

Comment 4

[Caitlin Neiman [:caitmuenster]]

Hey Ashley, sorry to  have missed you at the meeting. This bug was approved in the design decision meeting, and patches are welcome.

Comment 5

[ashley]

Oh that's great to hear. Given this is, I guess, a matter of wrapping the existing nsIDNSService in a webextensions-y wrapper, is there perhaps an example of this being done for other interfaces? I really wouldn't know where to start in implementing this.

Comment 6

[Shane Caraveo (:mixedpuppy)]

After bug 1152332 and bug 1409878 is implemented, the remaining major api missing that old PAC scripts provides (which cannot be simply replicated in js) is DNS access.  Moving to P2 for that reason.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1373640 implement async dns resolve api for webextensions,

Review commit: https://reviewboard.mozilla.org/r/222522/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222522/

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1373640 implement async dns resolve api for webextensions,

Review commit: https://reviewboard.mozilla.org/r/222528/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222528/

Comment 9

[ashley]

Thank you for the quick implementation :)

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8953239 [details]
Bug 1373640 implement async dns resolve api for webextensions,

https://reviewboard.mozilla.org/r/222528/#review228794

::: toolkit/components/extensions/ext-dns.js:5
(Diff revision 1)
> +ChromeUtils.defineModuleGetter(this, "Services",
> +                               "resource://gre/modules/Services.jsm");

This isn't used.

::: toolkit/components/extensions/ext-dns.js:9
(Diff revision 1)
> +  "bypass_cache": Ci.nsIDNSService.RESOLVE_BYPASS_CACHE,
> +  "canonical_name": Ci.nsIDNSService.RESOLVE_CANONICAL_NAME,
> +  "priority_medium": Ci.nsIDNSService.RESOLVE_PRIORITY_MEDIUM,
> +  "priority_low": Ci.nsIDNSService.RESOLVE_PRIORITY_LOW,
> +  "speculate": Ci.nsIDNSService.RESOLVE_SPECULATE,
> +  "disable_ipv6": Ci.nsIDNSService.RESOLVE_DISABLE_IPV6,
> +  "disable_ipv4": Ci.nsIDNSService.RESOLVE_DISABLE_IPV4,
> +  "offline": Ci.nsIDNSService.RESOLVE_OFFLINE,
> +  "allow_name_collisions": Ci.nsIDNSService.RESOLVE_ALLOW_NAME_COLLISION,
> +  "disable_trr": Ci.nsIDNSService.RESOLVE_DISABLE_TRR,

Nit: Please sort.

::: toolkit/components/extensions/ext-dns.js:23
(Diff revision 1)
> +  "disable_trr": Ci.nsIDNSService.RESOLVE_DISABLE_TRR,
> +};
> +
> +function getErrorString(nsresult) {
> +  try {
> +    throw Components.Exception("", nsresult);

There's no need to throw this.

::: toolkit/components/extensions/ext-dns.js:25
(Diff revision 1)
> +    let name = e.name;
> +    let message = "DNS resolve failure";
> +    let match = name.match(/NS_ERROR_(.*)/);
> +    if (match && match.length > 1) {
> +      name = match[1];
> +    }
> +    match = e.toString().match(/\"(.*?)\"/);
> +    if (match && match.length > 1) {
> +      message = match[1];
> +    }
> +    return `${name}: ${message}`;

Please don't try to parse the Components.Exception message. If you want to use it to create an error string, then just use its formatted error whole.

::: toolkit/components/extensions/ext-dns.js:45
(Diff revision 1)
> +          let dnsFlags = 0;
> +          flags = flags || [];
> +          for (let flag of flags) {
> +            dnsFlags |= dnssFlags[flag];
> +          }

Nit: `let dnsFlags = flags.reduce((mask, flag) => mask | dnssFlags[flag], 0)`

::: toolkit/components/extensions/ext-dns.js:61
(Diff revision 1)
> +            let listener = (inRequest, inRecord, inStatus) => {
> +              if (inRequest === request) {
> +                if (!Components.isSuccessCode(inStatus)) {
> +                  return reject({message: getErrorString(inStatus)});
> +                }
> +                if (flags.includes("canonical_name")) {

This may as well be `dnsFlags & RESOLVE_CANONICAL_NAME`. That should be much faster.

::: toolkit/components/extensions/ext-dns.js:69
(Diff revision 1)
> +                while (true) {
> +                  try {
> +                    response.addresses.push(inRecord.getNextAddrAsString());
> +                  } catch (e) {
> +                    return resolve(response);
> +                  }

:

    while (inRecord.hasMore()) {
      response.addresses.push(inRecord.getNextAddrAsString());
    }

::: toolkit/components/extensions/schemas/dns.json:28
(Diff revision 1)
> +        "type": "object",
> +        "description": "An object encapsulating a DNS Record.",
> +        "properties": {
> +          "canonicalName": {
> +            "type": "string",
> +            "description": "The canonical hostname for this record.  this value is empty if the record was not fetched with the 'canonical_name' flag."

`"optional": true`

::: toolkit/components/extensions/schemas/dns.json:55
(Diff revision 1)
> +            "type": "string"
> +          },
> +          {
> +            "name": "flags",
> +            "optional": true,
> +            "type": "array",

`"default": []`

Comment 11

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1373640 implement async dns resolve api for webextensions,

Review commit: https://reviewboard.mozilla.org/r/223046/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/223046/

Comment 12

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8953858 [details]
Bug 1373640 implement async dns resolve api for webextensions,

carry forward r+

Comment 13

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39a35e2f37f92172ebdeb7a19c47d8bc74dd7943
Bug 1373640 implement async dns resolve api for webextensions, r=kmag

Comment 14

[Shane Caraveo (:mixedpuppy)]

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

Comment 15

[Dorel Luca [:dluca]]

Backed out changeset 39a35e2f37f9 (bug 1373640) for xpcshell failure on test/xpcshell/test_ext_dns.js on a CLOSED TREE

Log of the failure:
Backed out changeset 39a35e2f37f9 (bug 1373640) for xpcshell failure on test/xpcshell/test_ext_dns.js on a CLOSED TREE

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8443793228312aa70373f276dbba454f4e39a4d6

Push that got backout:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=39a35e2f37f92172ebdeb7a19c47d8bc74dd7943

Comment 16

[Shane Caraveo (:mixedpuppy)]

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

Comment 17

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8953858 [details]
Bug 1373640 implement async dns resolve api for webextensions,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223046/diff/1-2/

Comment 18

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8953858 [details]
Bug 1373640 implement async dns resolve api for webextensions,

Turned off ipv6 and updated tests to only ipv4 to deal with differences in test machines/platforms.  carrying forward r+

Comment 19

[Shane Caraveo (:mixedpuppy)]

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

Comment 20

[Shane Caraveo (:mixedpuppy)]

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

Comment 21

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8953858 [details]
Bug 1373640 implement async dns resolve api for webextensions,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223046/diff/2-3/

Comment 22

[Shane Caraveo (:mixedpuppy)]

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

Comment 23

[Kris Maglione [:kmag]]

Comment on attachment 8953858 [details]
Bug 1373640 implement async dns resolve api for webextensions,

https://reviewboard.mozilla.org/r/223046/#review229646

::: toolkit/components/extensions/schemas/dns.json:60
(Diff revision 3)
> +            "optional": true,
> +            "type": "array",
> +            "default": [],
> +            "items": {
> +              "type": "string",
> +              "enum": [

Late nit: Please make this a named enum type (`ResolveFlag`?) so callers can check for the presence of properties.

And sort :)

Comment 24

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8953858 [details]
Bug 1373640 implement async dns resolve api for webextensions,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223046/diff/3-4/

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c6468a59407
implement async dns resolve api for webextensions, r=kmag

Comment 26

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/583d3a323785
Backed out changeset 7c6468a59407 for failures on xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_dns.js a=backout on a CLOSED TREE

Comment 27

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8953858 [details]
Bug 1373640 implement async dns resolve api for webextensions,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223046/diff/4-5/

Comment 28

[Andreea Pavel [:apavel]]

(In reply to Pulsebot from comment #26)

Backed out for failing xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_dns.js

Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7c6468a5940722980ee3db5709e48aac0cb25559

Failure log test-verify (tier2): https://treeherder.mozilla.org/logviewer.html#?job_id=164730749&repo=autoland&lineNumber=1317
Xpcshell (tier1): https://treeherder.mozilla.org/logviewer.html#?job_id=164729862&repo=autoland&lineNumber=2054  

Backout: https://hg.mozilla.org/integration/autoland/rev/583d3a323785

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30b928272f74
implement async dns resolve api for webextensions, r=kmag

Comment 30

[Shane Caraveo (:mixedpuppy)]

sigh. no app directory on android 4.3?  

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=30b928272f74408e58382fe4b36cfa96ec14b1c4

Comment 31

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b246fb271c17
Backed out changeset 30b928272f74 for failures on xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_dns.js a=backout

Comment 32

[Kris Maglione [:kmag]]

Why was this backed out? I don't see any relevant tier 1 failures in that push.

Comment 33

[Shane Caraveo (:mixedpuppy)]

Kris: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=30b928272f74408e58382fe4b36cfa96ec14b1c4

Android 4.3 API16+ TV failures in the dns test file.  though I don't know why the particular failure is happening now (it happens before the dns api is used afaik)

Comment 34

[Kris Maglione [:kmag]]

That's a bug in the TV job. It runs changed tests in configurations that are supposed to be disabled. That happens every time we change an xpcshell test.

But TV is tier 2, and we don't backout for tier 2 failures.

Comment 35

[Shane Caraveo (:mixedpuppy)]

ok, re-landing as-is

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/998ff573d5cd
implement async dns resolve api for webextensions, r=kmag

Comment 37

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/998ff573d5cd

Comment 38

[Andreea Pavel [:apavel]]

Hi, :mixedpuppy, :kmag, please read comment 28 where I posted failure logs for both tier1 and tier2.  Feel free to CC us again if you need info. 

Regards,

Comment 39

[Shane Caraveo (:mixedpuppy)]

(In reply to Andreea Pavel [:apavel] from comment #38)
> Hi, :mixedpuppy, :kmag, please read comment 28 where I posted failure logs
> for both tier1 and tier2.  Feel free to CC us again if you need info. 
> 
> Regards,

The only errors I see related to this patch are TV, due to bug 1406407.

Comment 40

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #39)
> (In reply to Andreea Pavel [:apavel] from comment #38)
> > Hi, :mixedpuppy, :kmag, please read comment 28 where I posted failure logs
> > for both tier1 and tier2.  Feel free to CC us again if you need info. 
> > 
> > Regards,
> 
> The only errors I see related to this patch are TV, due to bug 1406407.

oh, there is a tier 1.  

coffee before comment.

I haven't seen that crop up since landing but will look.

Comment 41

[Marcin Rataj]

Hey, quick question: would it be technically possible to extend this API to do lookups for DNS records other than A/AAAA, or is it a totally different beast? 
I am personally interested in TXT records. Does it make sense to create a separate ticket for it?

Comment 42

[Philipp Kewisch [:Fallen][📱📆]]

This probably blocks on bug 14328.

Comment 43

[Julien Cristau [:jcristau]]

Shell, isn't this something for https://developer.mozilla.org/en-US/Firefox/Releases/60#Changes_for_add-on_and_Mozilla_developers rather than the user facing release notes?

Comment 44

[Will Bamberg [:wbamberg]]

I've added: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/dns for this, and filed https://github.com/mdn/browser-compat-data/pull/1679, although that's not merged yet.

Please let me know if this looks good.

Comment 45

[Shane Caraveo (:mixedpuppy)]

Thanks!