Closed Bug 1373640 Opened 7 years ago Closed 6 years ago

Add DNS resolution to webextensions api

Categories

(WebExtensions :: Untriaged, enhancement, P2)

Product:
WebExtensions
Component:
Untriaged
Type:
enhancement

Tracking

(relnote-firefox -, firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
relnote-firefox --- -
firefox60 --- fixed

People

(Reporter: ashley, Assigned: mixedpuppy)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, feature, Whiteboard: [design-decision-approved])

Attachments

(1 file, 2 obsolete files)

Bug 1373640 implement async dns resolve api for webextensions,
59 bytes, text/x-review-board-request
kmag
: review+
Details 
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.
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
Severity: normal → enhancement
status-firefox57: --- → unaffected
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
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]
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.
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: nobody → mixedpuppy
Attachment #8953236 - Attachment is obsolete: true
Attachment #8953236 - Flags: review?(kmaglione+bmo)
Thank you for the quick implementation :)
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+
Attachment #8953239 - Attachment is obsolete: true
Comment on attachment 8953858 [details]
Bug 1373640 implement async dns resolve api for webextensions,

carry forward r+
Attachment #8953858 - Flags: review?(kmaglione+bmo) → review+
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 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 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+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7c6468a59407
implement async dns resolve api for webextensions, r=kmag
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
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/30b928272f74
implement async dns resolve api for webextensions, r=kmag
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)
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.
ok, re-landing as-is
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/998ff573d5cd
implement async dns resolve api for webextensions, r=kmag
Depends on: 1442177
https://hg.mozilla.org/mozilla-central/rev/998ff573d5cd
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
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)
(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.
(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.
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.
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)
Thanks!
Flags: needinfo?(mixedpuppy)
relnote-firefox: ? → -
Flags: needinfo?(sescalante)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: