Add DNS resolution to webextensions api

RESOLVED FIXED in Firefox 60

Status

enhancement
P2
normal
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: ashley, Assigned: mixedpuppy)

Tracking

(Blocks 2 bugs, {dev-doc-complete, feature})

Trunk
mozilla60
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(relnote-firefox -, firefox60 fixed)

Details

(Whiteboard: [design-decision-approved])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
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...
Whiteboard: design-decision-needed

Updated

2 years ago
Severity: normal → enhancement
Keywords: feature
Priority: -- → P5
Whiteboard: design-decision-needed → [design-decision-needed]
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
(Reporter)

Comment 3

a year ago
Yes, I can join tomorrow :)
Hey Ashley, sorry to  have missed you at the meeting. This bug was approved in the design decision meeting, and patches are welcome.
Whiteboard: [design-decision-needed] → [design-decision-approved]
(Reporter)

Comment 5

a year ago
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.
(Assignee)

Comment 6

a year ago
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.
Priority: P5 → P2
(Assignee)

Updated

a year ago
Assignee: nobody → mixedpuppy
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8953236 - Attachment is obsolete: true
Attachment #8953236 - Flags: review?(kmaglione+bmo)
(Reporter)

Comment 9

a year ago
Thank you for the quick implementation :)

Comment 10

a year ago
mozreview-review
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": []`
Attachment #8953239 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8953239 - Attachment is obsolete: true
(Assignee)

Comment 12

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

carry forward r+
Attachment #8953858 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Updated

a year ago
Flags: qe-verify-
Keywords: dev-doc-needed
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
Flags: needinfo?(mixedpuppy)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

a year ago
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+
Flags: needinfo?(mixedpuppy)
Attachment #8953858 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)

Comment 23

a year ago
mozreview-review
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 :)
Attachment #8953858 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)

Comment 25

a year ago
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7c6468a59407
implement async dns resolve api for webextensions, r=kmag

Comment 26

a year ago
Backout by aciure@mozilla.com:
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 hidden (mozreview-request)

Comment 29

a year ago
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/30b928272f74
implement async dns resolve api for webextensions, r=kmag

Comment 31

a year ago
Backout by aciure@mozilla.com:
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
Why was this backed out? I don't see any relevant tier 1 failures in that push.
Flags: needinfo?(aciure)
(Assignee)

Comment 33

a year ago
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)
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.
(Assignee)

Comment 35

a year ago
ok, re-landing as-is

Comment 36

a year ago
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/998ff573d5cd
implement async dns resolve api for webextensions, r=kmag
(Assignee)

Updated

a year ago
Depends on: 1442177

Comment 37

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/998ff573d5cd
Status: UNCONFIRMED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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,
Flags: needinfo?(aciure)
(Assignee)

Comment 39

a year ago
(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.
(Assignee)

Comment 40

a year ago
(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

a year ago
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?
This probably blocks on bug 14328.

Updated

a year ago
relnote-firefox: --- → ?
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?
Flags: needinfo?(sescalante)
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.
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 45

a year ago
Thanks!
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(sescalante)

Updated

10 months ago
Product: Toolkit → WebExtensions
Blocks: p2p
You need to log in before you can comment on or make changes to this bug.