1247680 - [mDNS] Provide a JS implementation for mDNS service advertisement

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Justin D'Arcangelo [:justindarc]]

As a follow-up to Bug 1241368, the fallback JS implementation of mDNS should also handle service advertisement in addition to discovery. This is required for the ongoing FlyWeb work tracking in Bug 1228662.

Comment 1

[Justin D'Arcangelo [:justindarc]]

Assigning to myself since I already have a working implementation for FxOS that I should be able to re-purpose for this.

Comment 2

[Justin D'Arcangelo [:justindarc]]

Attachment: bug1247680.patch

Comment 3

[Kannan Vijayan [:djvj]]

Comment on attachment 8736802 [details] [diff] [review]
bug1247680.patch

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

r+ with edits.

::: netwerk/dns/mdns/libmdns/fallback/DNSRecord.jsm
@@ +33,5 @@
> +  }
> +
> +  serialize() {
> +    let writer = new DataWriter();
> +    

Nit: whitespace on blank line.

@@ +37,5 @@
> +    
> +    // Write `name` (ends with trailing 0x00 byte)
> +    writer.putLabel(this.name);
> +    writer.putValue(0x00);
> +    

Nit: whitespace on blank line.

::: netwerk/dns/mdns/libmdns/fallback/DNSResourceRecord.jsm
@@ +109,5 @@
> + * @private
> + */
> +function _parseA(recordData, packetData) {
> +  let reader = new DataReader(recordData);
> +  

Nit: whitespace on blank line.

::: netwerk/dns/mdns/libmdns/fallback/DataWriter.jsm
@@ +54,5 @@
> +    let parts = label.split('.');
> +    parts.forEach((part) => {
> +      let length = part.length;
> +      this.putValue(length);
> +      

Nit: whitespace on blank line.

::: netwerk/dns/mdns/libmdns/fallback/MulticastDNS.jsm
@@ +38,5 @@
> +
> +  startDiscovery(aServiceType, aListener) {
> +    DEBUG && debug('startDiscovery("' + aServiceType + '")');
> +    let { serviceType } = _parseServiceDomainName(aServiceType);
> +    

Nit: whitespace on blank line.

@@ +54,5 @@
> +
> +  stopDiscovery(aServiceType, aListener) {
> +    DEBUG && debug('stopDiscovery("' + aServiceType + '")');
> +    let { serviceType } = _parseServiceDomainName(aServiceType);
> +    

Nit: whitespace on blank line.

@@ +161,5 @@
> +      this._discovered.forEach((discovery, key) => {
> +        if (discovery.expireTime < Date.now()) {
> +          this._discovered.delete(key);
> +          return;
> +        }

Would be good to factor out the clearing of expired discoveries into its own method,
like |this._clearExpiredDiscoveries()| or something.

That way you could remove the check for expiry from this loop, and just precede the |forEach| with a call to that clear method.

@@ +423,5 @@
> +
> +    _networkInfo.listNetworkAddresses({
> +      onListedNetworkAddresses(aAddressArray) {
> +        _addresses = aAddressArray.filter((address) => {
> +          return address !== '127.0.0.1'        &&  // No IPv4 loopback

I thought we were going to allow advertising on loopback?

::: netwerk/dns/mdns/libmdns/moz.build
@@ +27,5 @@
>                  'external/mdnsresponder/mDNSShared',
>              ]
>          ]
>  
> +EXTRA_COMPONENTS += [

This buildconfig should be conditionalized to:

if CONFIG['MOZ_WIDGET_TOOLKIT'] in ('android', 'windows'):

Comment 4

[Justin D'Arcangelo [:justindarc]]

https://hg.mozilla.org/projects/larch/rev/d8da7ea96da13ef1060be0b2d098f59298d5e85b
Bug 1247680 - [mDNS] Provide a JS implementation for mDNS service advertisement, r=djvj

Comment 5

[Justin D'Arcangelo [:justindarc]]

Oops. Forgot to close this when we landed a while back.