[mDNS] Provide a JS implementation for mDNS service advertisement

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: justindarc, Assigned: justindarc)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ft:conndevices][necko-would-take])

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

2 years ago
Assigning to myself since I already have a working implementation for FxOS that I should be able to re-purpose for this.
Assignee: nobody → jdarcangelo
Blocks: 1228662
Status: NEW → ASSIGNED
See Also: → bug 1241368
Whiteboard: [ft:conndevices]
Whiteboard: [ft:conndevices] → [ft:conndevices][necko-would-take]
Depends on: 1259144
(Assignee)

Comment 2

a year ago
Created attachment 8736802 [details] [diff] [review]
bug1247680.patch
Attachment #8736802 - Flags: review?(kvijayan)
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'):
Attachment #8736802 - Flags: review?(kvijayan) → review+
(Assignee)

Comment 4

a year ago
https://hg.mozilla.org/projects/larch/rev/d8da7ea96da13ef1060be0b2d098f59298d5e85b
Bug 1247680 - [mDNS] Provide a JS implementation for mDNS service advertisement, r=djvj
Depends on: 1264034
Depends on: 1264727
Depends on: 1264730
Depends on: 1264732
(Assignee)

Comment 5

a year ago
Oops. Forgot to close this when we landed a while back.
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.