1241368 - [MDNS] Provide a JS implemented MDNS as a fallback mechanism

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

For platform that doesn't have built-in MDNS service, we can use our own MDNS service as a fallback mechanism.

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

This can be used for Android JB/KK because we cannot enable Java mDNS on those devices (see bug 1188935). We can also use this as an alternative before we solve the mdnsd power consumption issue on Android L because we have better control on this approach.

I'd like to hear the feedback from @mfinkle about this approach.

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: mdns-js-fallback.patch

1. only support service query
2. socket will be closed if no active discovery

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Tested on Android KK and L.

Comment 4

[u408661]

Comment on attachment 8712597 [details] [diff] [review]
mdns-js-fallback.patch

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

So in addition to addressing/answering all of my comments below, you'll also need a toolkit peer to look over the new jsm, as well, as I'm not qualified to r+ there.

::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.js
@@ +4,5 @@
>  "use strict";
>  
>  const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
>  
> +Cu.import("resource://gre/modules/MulticastDNSFallback.jsm");

This is my biggest question here - the bug says we are going to use this as a fallback mechanism, but here we're doing a wholesale replacement of MulticastDNS.jsm. It seems to make more sense to me to, instead of just replacing, make it a run-time switch (based on a pref or something similar), or (if this makes more sense) a build-time switch. The import line wouldn't change, but depending on how the switch is set (at whatever point in time), we would get either the native module like we do now, or this module.

@@ +134,5 @@
>          if (this.discoveryStarting || this.stopDiscovery) {
>            this.stopDiscovery = true;
>            return;
>          }
> +        this.mdns.stopDiscovery(aServiceType, listener);

I may be misreading, but I don't believe this change (or the similar one later on) is needed, as "this" should already be bound to "listener".

::: toolkit/modules/secondscreen/MulticastDNSFallback.jsm
@@ +13,5 @@
> +Cu.import('resource://gre/modules/Services.jsm');
> +Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> +
> +const MDNS_PORT = 5353;
> +const MDNS_ADDRESS = '224.0.0.251';

Might it be useful to also have the IPv6 MDNS address available for use?

@@ +178,5 @@
> + * @constructor
> + */
> +let DNSPacket = function(opt_flags) {
> +  this.flags_ = opt_flags || 0; /* uint16 */
> +  this.data_ = {'qd': [], 'an': [], 'ns': [], 'ar': []};

It would be good to document what each of these corresponds to - and probably use some well-named constants when referring to them (both here and later) instead of just raw strings.

@@ +328,5 @@
> +/* end https://raw.githubusercontent.com/GoogleChrome/chrome-app-samples/master/mdns-browser/dns.js */
> +
> +/**
> + * Parse fully qualified domain name to service name, instance name,
> + * and domain name.

An example with what parts correspond to service, instance, and domain name would be useful here.

@@ +338,5 @@
> +    return element === '_tcp' || element === '_udp';
> +  });
> +
> +  return {
> +    instanceName: items.splice(0, idx-1).join('.'),

nit: spaces around '-'

@@ +344,5 @@
> +    domainName: items.join('.')
> +  };
> +}
> +
> +function _createPropertyPag(map) {

s/Pag/Bag/

@@ +363,5 @@
> +MulticastDNS.prototype = {
> +  socket: null,
> +  //public API
> +  startDiscovery: function(aServiceType, aListener) {
> +    DEBUG && debug('startDiscovery for ' + aServiceType);

Is this a common idiom in JSMs? It would seem to make more sense to put the check for DEBUG inside debug() to avoid the silly boilerplate everywhere.

@@ +369,5 @@
> +    this._addServiceListener(serviceType, aListener);
> +
> +    try {
> +      this._ensureSocket();
> +      this._query(serviceType + '.local', DNS_REC_TYPE_PTR);

Might it make sense to check for the pre-existence of '.local'?

@@ +374,5 @@
> +      aListener.onDiscoveryStarted(serviceType);
> +    } catch (e) {
> +      DEBUG && debug('onStartDiscoveryFailed: ' + serviceType + ' (' + e + ')');
> +      this._removeServiceListener(serviceType, aListener);
> +      aListener.onStartDiscoveryFailed(serviceType, Cr.NS_ERROR_FAILURE);

I assume this is an already-existant API, but the difference between "onDiscoveryStarted" and "onStartDiscoveryFailed" bugs me.

@@ +393,5 @@
> +  },
> +
> +  registerService: function(aServiceInfo, aListener) {
> +    DEBUG && debug('service registration is not supported');
> +    aListener.onRegistrationFailed(aServiceInfo, Cr.NS_ERROR_NOT_AVAILABLE);

I think raising NS_ERROR_NOT_IMPLEMENTED would be more appropriate here, rather than doing a callback to a listener (which would be more for asynchronous errors)

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #1)
> This can be used for Android JB/KK because we cannot enable Java mDNS on
> those devices (see bug 1188935). We can also use this as an alternative
> before we solve the mdnsd power consumption issue on Android L because we
> have better control on this approach.
> 
> I'd like to hear the feedback from @mfinkle about this approach.

We would definitely need to evaluate the power consumption affects with using this patch. It's good to have more control, and we should make sure we turn off the system at appropriate times.

In short, any mDNS changes make me worry, so let's be certain we do the right things.

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8712597 [details] [diff] [review]
mdns-js-fallback.patch

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

Thanks for your quick review and here is my response.

::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.js
@@ +4,5 @@
>  "use strict";
>  
>  const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
>  
> +Cu.import("resource://gre/modules/MulticastDNSFallback.jsm");

Ok will do it by pref.

@@ +134,5 @@
>          if (this.discoveryStarting || this.stopDiscovery) {
>            this.stopDiscovery = true;
>            return;
>          }
> +        this.mdns.stopDiscovery(aServiceType, listener);

"listener" is not bound to "this" and this is a bug in current code, see https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.js#127

::: toolkit/modules/secondscreen/MulticastDNSFallback.jsm
@@ +13,5 @@
> +Cu.import('resource://gre/modules/Services.jsm');
> +Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> +
> +const MDNS_PORT = 5353;
> +const MDNS_ADDRESS = '224.0.0.251';

Can IPv6 support be done in a follow-up bug?

@@ +178,5 @@
> + * @constructor
> + */
> +let DNSPacket = function(opt_flags) {
> +  this.flags_ = opt_flags || 0; /* uint16 */
> +  this.data_ = {'qd': [], 'an': [], 'ns': [], 'ar': []};

Done by well-named constants.

@@ +328,5 @@
> +/* end https://raw.githubusercontent.com/GoogleChrome/chrome-app-samples/master/mdns-browser/dns.js */
> +
> +/**
> + * Parse fully qualified domain name to service name, instance name,
> + * and domain name.

Done.

@@ +338,5 @@
> +    return element === '_tcp' || element === '_udp';
> +  });
> +
> +  return {
> +    instanceName: items.splice(0, idx-1).join('.'),

Done.

@@ +344,5 @@
> +    domainName: items.join('.')
> +  };
> +}
> +
> +function _createPropertyPag(map) {

Done. (my bad)

@@ +363,5 @@
> +MulticastDNS.prototype = {
> +  socket: null,
> +  //public API
> +  startDiscovery: function(aServiceType, aListener) {
> +    DEBUG && debug('startDiscovery for ' + aServiceType);

This can skip the string operation while calling logging function for production. Can remove most of overhead for debug logging.

@@ +374,5 @@
> +      aListener.onDiscoveryStarted(serviceType);
> +    } catch (e) {
> +      DEBUG && debug('onStartDiscoveryFailed: ' + serviceType + ' (' + e + ')');
> +      this._removeServiceListener(serviceType, aListener);
> +      aListener.onStartDiscoveryFailed(serviceType, Cr.NS_ERROR_FAILURE);

See https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/mdns/nsIDNSServiceDiscovery.idl#115 for the definition of these callback functions.

@@ +393,5 @@
> +  },
> +
> +  registerService: function(aServiceInfo, aListener) {
> +    DEBUG && debug('service registration is not supported');
> +    aListener.onRegistrationFailed(aServiceInfo, Cr.NS_ERROR_NOT_AVAILABLE);

Done.

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Mark Finkle (:mfinkle) from comment #5)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #1)
> > This can be used for Android JB/KK because we cannot enable Java mDNS on
> > those devices (see bug 1188935). We can also use this as an alternative
> > before we solve the mdnsd power consumption issue on Android L because we
> > have better control on this approach.
> > 
> > I'd like to hear the feedback from @mfinkle about this approach.
> 
> We would definitely need to evaluate the power consumption affects with
> using this patch. It's good to have more control, and we should make sure we
> turn off the system at appropriate times.
> 
> In short, any mDNS changes make me worry, so let's be certain we do the
> right things.

Thanks for the feedback, @mfinkle. I'd like to know more about the technique how Fennec team test for power consumption and the criteria.

The current design is that mDNS search can be completely turned off by gecko pref. We only send 1 UDP packet for device probing and receiving network packet for 10s for every 120s period, while Fennec in foreground. No mDNS probing while Fennec in background.

Comment 8

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: mdns-js-fallback.patch, v2

patch updated according to review comments.

Comment 9

[u408661]

Comment on attachment 8713082 [details] [diff] [review]
mdns-js-fallback.patch, v2

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

r=hurley for the bits under netwerk/, and f+ on the bits under toolkit/, but you still need a toolkit peer to review those

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #9)
> Comment on attachment 8713082 [details] [diff] [review]
> mdns-js-fallback.patch, v2
> 
> Review of attachment 8713082 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=hurley for the bits under netwerk/, and f+ on the bits under toolkit/, but
> you still need a toolkit peer to review those

Ah..yeah.. Maybe I should move MulticastDNSFallback.jsm into netwerk/ because it is not designed for direct import. The mDNS service interface should always go through nsIDNSService.
@nwgh how do you think?

Comment 11

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: mdns-js-fallback.patch

patch updated according to comment #10.

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 13

[u408661]

Comment on attachment 8714273 [details] [diff] [review]
mdns-js-fallback.patch

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

This all seems OK to me, but I do have two questions (one inline below). The other: is there a reason that MulticastDNSFallback.jsm isn't in the same place as MulticastDNS.jsm? Right now, MulticastDNS.jsm is in an android-specific location in the source tree. I think we should either (1) move MulticastDNSFallback to the same directory (if this is intended only for android), or (2) move MulticastDNS.jsm to the same directory as this new file (if they're intended to be more widely-used).

It seems like this is likely to be more widely used (for TVs and other such devices), so (2) is likely the right answer. I don't want to move android code around without a more professional opinion on those bits.

Flagging rnewman (who reviewed the original patch adding MulticastDNS.jsm) for an opinion.

::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.js
@@ +11,5 @@
> +if (Services.prefs.getBoolPref("network.mdns.use_js_fallback")) {
> +  Cu.import("resource://gre/modules/MulticastDNSFallback.jsm");
> +} else {
> +  Cu.import("resource://gre/modules/MulticastDNS.jsm");
> +}

Is there any reason that some other code might need to import MulticastDNS.jsm? I know nothing else does now, but we might want to make this switch inside MulticastDNS.jsm instead of where we import it, so that any future uses can have The Right Behavior without having to jump through the pref hoop themselves.

Comment 14

[u408661]

rnewman - see the top bit of comment 13 for my question.

Comment 15

[Richard Newman [:rnewman]]

(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #13)

> Flagging rnewman (who reviewed the original patch adding MulticastDNS.jsm)
> for an opinion.

It seems to be only referenced from plain ol' EXTRA_JS_MODULES in Fennec's moz.build, and only used from netwerk, so I see no reason not to move it if it won't affect functionality.

Please flag :nalexander for a quick review on the moz.build parts.

Comment 16

[u408661]

Comment on attachment 8714273 [details] [diff] [review]
mdns-js-fallback.patch

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

OK, let's move the original MulticastDNS.jsm to live alongside the new location of MulticastDNSFallback.jsm, and also please consider my other comment in the previous review. Once that's done, flag me and :nalexander for review, and that should be our final round. Thanks!

Comment 17

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #13)
> Comment on attachment 8714273 [details] [diff] [review]
> mdns-js-fallback.patch
> 
> Review of attachment 8714273 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This all seems OK to me, but I do have two questions (one inline below). The
> other: is there a reason that MulticastDNSFallback.jsm isn't in the same
> place as MulticastDNS.jsm? Right now, MulticastDNS.jsm is in an
> android-specific location in the source tree. I think we should either (1)
> move MulticastDNSFallback to the same directory (if this is intended only
> for android), or (2) move MulticastDNS.jsm to the same directory as this new
> file (if they're intended to be more widely-used).
> 
> It seems like this is likely to be more widely used (for TVs and other such
> devices), so (2) is likely the right answer. I don't want to move android
> code around without a more professional opinion on those bits.
> 
> Flagging rnewman (who reviewed the original patch adding MulticastDNS.jsm)
> for an opinion.
Maybe I should rename MulticastDNS.jsm to "MulticastDNSAndroid.jsm" for readability and I prefer option #2 as well.
> 
> ::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.js
> @@ +11,5 @@
> > +if (Services.prefs.getBoolPref("network.mdns.use_js_fallback")) {
> > +  Cu.import("resource://gre/modules/MulticastDNSFallback.jsm");
> > +} else {
> > +  Cu.import("resource://gre/modules/MulticastDNS.jsm");
> > +}
> 
> Is there any reason that some other code might need to import
> MulticastDNS.jsm? I know nothing else does now, but we might want to make
> this switch inside MulticastDNS.jsm instead of where we import it, so that
> any future uses can have The Right Behavior without having to jump through
> the pref hoop themselves.
MulticastDNSFallback.jsm is designed as a backend for platform that doesn't have built-in MDNS service, e.g. Windows before Windows 10. We planed use Android built-in MDNS service but here are the issues we encountered:
On Android JB/KK: built-in MDNS is unstable (bug 1188935) so we need to use the JS fallback.
On Android L/M: built-in MDNS have power management issue (bug 1194049) and incomplete functionality (bug 1228192). We need to use the JS fallback before those issues are fixed. Or, remove MulticastDNS.jsm (and related Java code) if those issue cannot be fixed by Mozilla.

Comment 18

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: mdns-js-fallback.patch

Comment 19

[u408661]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #17)
> Maybe I should rename MulticastDNS.jsm to "MulticastDNSAndroid.jsm" for
> readability and I prefer option #2 as well.

That seems a reasonable course of action to me.

> MulticastDNSFallback.jsm is designed as a backend for platform that doesn't
> have built-in MDNS service, e.g. Windows before Windows 10. We planed use
> Android built-in MDNS service but here are the issues we encountered:
> On Android JB/KK: built-in MDNS is unstable (bug 1188935) so we need to use
> the JS fallback.
> On Android L/M: built-in MDNS have power management issue (bug 1194049) and
> incomplete functionality (bug 1228192). We need to use the JS fallback
> before those issues are fixed. Or, remove MulticastDNS.jsm (and related Java
> code) if those issue cannot be fixed by Mozilla.

So in that case, we will have multiple MDNS implementations in the tree (this js one, one that calls natively into win 10 functions, perhaps still the java bridge for android native), correct? In that case, my suggestion stands (and gets even stronger) - the code importing a MDNS implementation should not have to care which one it is using, that should be handled either at build time, or at run time. Perhaps a combination of both - at build time, we determine which native implementation to have available (if any), and at run time we determine whether we can in fact use the native implementation, or if the fallback implementation is necessary (based either on a pref, or on knowledge of the specific platform, ie "this is Android KK, so the fallback is necessary" or "this is Windows 10, so use the native implementation"). Either way, I, as someone who wants to do MDNS, should just be able to "Cu.import(gre://resources/MulticastDNS.jsm);" and get The Right Backend, whatever that is.

Comment 20

[u408661]

Comment on attachment 8715171 [details] [diff] [review]
mdns-js-fallback.patch

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

Does splinter not show file moves? I'm not seeing any mention of MulticastDNS.jsm (except in the moz.build files).

Comment 21

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8715171 [details] [diff] [review]
mdns-js-fallback.patch

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

I agree with Nicholas Hurley that the patch is malformed, since some files have moved.

Comment 22

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

It's really sad that we cannot move file because of our review system cannot parse it correctly. Change history in that file will be discontinued after the file deletion. :(

Comment 23

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: MozReview Request: Bug 1241368 - provide JS implemented MDNS service as a fallback. r=nwgh,nalexander.

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

Comment 24

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I found reviewboard is able to display moved file correctly. Let me know if you don't want me to use 'hg mv' but simply delete/create file.

Comment 25

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8715621 [details]
MozReview Request: Bug 1241368 - provide JS implemented MDNS service as a fallback. r=nwgh,nalexander.

https://reviewboard.mozilla.org/r/33579/#review30209

lgtm.

Comment 26

[u408661]

Comment on attachment 8715621 [details]
MozReview Request: Bug 1241368 - provide JS implemented MDNS service as a fallback. r=nwgh,nalexander.

https://reviewboard.mozilla.org/r/33579/#review30335

::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.js:11
(Diff revision 1)
> +if (Services.prefs.getBoolPref("network.mdns.use_js_fallback")) {
> +  Cu.import("resource://gre/modules/MulticastDNSFallback.jsm");
> +} else {
> +  Cu.import("resource://gre/modules/MulticastDNSAndroid.jsm");
> +}

You still haven't responded to my previous comments about this portion of the code. I won't review further until either (1) a change is made, or (2) an explanation of why a change can't/shouldn't be made is offered.

Comment 27

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I forgot to mention that all mDNS users should use the nsIDNSServiceDiscovery XPCOM Service [1] via contract ID "
@mozilla.org/toolkit/components/mdnsresponder/dns-sd;1" but not JSM.

The build time configuration is already included in [2] to switch the XPCOM implementation. On Mac and Gonk we provide an XPCOM glue to native mDNSResponder service.

Does that answer your question in comment #19?

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/mdns/nsIDNSServiceDiscovery.idl#180
[2] https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/mdns/libmdns/moz.build

Comment 28

[u408661]

OK, excellent, that makes much more sense. Thanks!

Comment 29

[u408661]

Comment on attachment 8715621 [details]
MozReview Request: Bug 1241368 - provide JS implemented MDNS service as a fallback. r=nwgh,nalexander.

https://reviewboard.mozilla.org/r/33579/#review30465

Comment 30

[Doug Turner (:dougt)]

Are there tests?

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6c0d17c38bf

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d6c0d17c38bf

Comment 33

[Justin D'Arcangelo [:justindarc]]

While working on the FlyWeb implementation, I noticed that the native mDNS implementation for Android was deprecated in favor of this JS implementation in Fennec. However, this does not yet support service advertisement which we require. I opened Bug 1247680 to track work on implementing it.