Closed Bug 1264727 Opened 8 years ago Closed 8 years ago

[FlyWeb] JS implementation of web-page service advertisement takes a long time to be discovered

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: djvj, Unassigned)

References

Details

(Whiteboard: [necko-would-take])

Attachments

(8 files, 4 obsolete files)

3.08 KB, patch
sicking
: review+
Details | Diff | Splinter Review
1012 bytes, patch
justindarc
: review+
Details | Diff | Splinter Review
864 bytes, patch
justindarc
: review+
Details | Diff | Splinter Review
1.68 KB, patch
justindarc
: review+
Details | Diff | Splinter Review
4.42 KB, patch
justindarc
: review+
Details | Diff | Splinter Review
7.99 KB, patch
justindarc
: review+
Details | Diff | Splinter Review
14.05 KB, patch
djvj
: review+
Details | Diff | Splinter Review
3.19 KB, patch
justindarc
: review+
Details | Diff | Splinter Review
Not sure what the cause of this is, but when I was testing service advertisement and discovery, the services advertised by web-pages (and the JS-fallback implementation of mDNS advertising we ship) take a lot longer to show up in the list-of-discovered-services of other devices.  Hardware devices on the network show up a lot quicker.

Should investigate why and how this is happening and resolve it.
Whiteboard: [necko-would-take]
Tracked this down and identified a number of different issues with the existing implementation.  Fixed them.  Posting patches.
This patch fixes FlyWebService::publishServer.

The issue was that the server was not added to the list of published servers before being advertised.  The service-advertisement's "onServiceRegistered" callback happened on the same tick as advertise call from "publishServer", and thus the service was not visible.

This led to errors "onServiceRegistered" callback always threw an error inside the js impl of service advertisement.

This patch fixes things so that "PublishServer" registers the server before advertising it.
The JS implementation of mDNS was not sending the A record in the AR section of the PTR response.  This fixes that.
In the JS mDNS implementation, the |onServiceRegistered| callback was called once for every external-ip-addr that the service was registered on.  This fixes that to call |onServiceRegistered| at once per registration.
The JS mDNS implementation responds to queries for PTR records by broadcasting the response to the mDNS multicast address.  This patch fixes that behaviour to respond only to the originator of the request.
Patch to fix several issues in packet serialization in the JS mDNS implementation.
Attached patch 6-add-service-broadcasting.patch (obsolete) — Splinter Review
Adds service broadcasting when a service is added.  We need to do more here (like periodically advertising services), but for now we just advertise once when the service is added.
This patch fixes up service broadcasting to be proper.  Whenever a new service is added, it's advertised immediately, and again one second afterward.

From then on, the default TTL of 120s is used, and a timer keeps track of making sure new advertisements are sent out before that TTL expires.
Attachment #8742996 - Flags: review?(jonas)
Attachment #8742999 - Flags: review?(jdarcangelo)
Attachment #8743001 - Flags: review?(jdarcangelo)
Attachment #8743004 - Flags: review?(jdarcangelo)
Attachment #8743005 - Flags: review?(jdarcangelo)
Attachment #8743006 - Flags: review?(jdarcangelo)
Attachment #8743413 - Flags: review?(jdarcangelo)
Attachment #8742999 - Flags: review?(jdarcangelo) → review+
Attachment #8743001 - Flags: review?(jdarcangelo) → review+
Comment on attachment 8743004 [details] [diff] [review]
4-only-send-query-response-to-originating-address.patch

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

::: netwerk/dns/mdns/libmdns/fallback/MulticastDNS.jsm
@@ +89,5 @@
>            address: address,
>            port: aServiceInfo.port,
>            serviceName: aServiceInfo.serviceName,
> +          serviceType: serviceType,
> +          serviceAttrs: {}

There is an already-defined property for nsIDNSServiceInfo called `attributes` that we should probably use instead:

https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/mdns/nsIDNSServiceDiscovery.idl#55
Attachment #8743004 - Flags: review?(jdarcangelo) → review-
Comment on attachment 8743005 [details] [diff] [review]
5-fix-dns-packet-serialization.patch

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

All these changes look good to me. However, I'm not sure how this fixes the described issue of long discovery times.
Attachment #8743005 - Flags: review?(jdarcangelo) → review+
(In reply to Justin D'Arcangelo [:justindarc] from comment #10)
> Comment on attachment 8743005 [details] [diff] [review]
> 5-fix-dns-packet-serialization.patch
> 
> Review of attachment 8743005 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> All these changes look good to me. However, I'm not sure how this fixes the
> described issue of long discovery times.

Sorry, disregard that comment. I just noticed that patch was specifically for cleaning up packet serialization. LGTM.
Comment on attachment 8743006 [details] [diff] [review]
6-add-service-broadcasting.patch

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

Overall looks good, but should probably have the re-broadcast after 1000ms included now that we are properly multicasting the announcements and unicasting direct query responses. Also needs the same correction from Pt.4 regarding the `attributes` property of nsIDNSServiceInfo. I like that you cleaned up `_makeServicePacket()` for reuse.

::: netwerk/dns/mdns/libmdns/fallback/MulticastDNS.jsm
@@ +96,5 @@
>            serviceAttrs: {}
>          };
>  
>          services.push(serviceInfo);
> +        if (address != "127.0.0.1") {

nit: `!=` -> `!==`

@@ +105,5 @@
> +      this._getBroadcastSocket().then(socket => {
> +        let packet = this._makeServicePacket({
> +          serviceName: aServiceInfo.serviceName,
> +          serviceType: serviceType,
> +          serviceAttrs: {},

When you fix Pt.4 of this patch, be sure to update this to use the already-defined `attributes` property.

@@ +110,5 @@
> +          host: _hostname,
> +          port: aServiceInfo.port
> +        }, addressList);
> +        let data = packet.serialize();
> +        socket.send(MDNS_MULTICAST_GROUP, MDNS_PORT, data, data.length);

Re-broadcast here again after 1000ms.

@@ +140,5 @@
>  
>          let serviceInfo = services[index];
>  
>          services.splice(index, 1);
> +        if (services.length == 0) {

nit: `==` -> `===`

@@ +243,5 @@
>        });
>      });
>    }
>  
> +  _makeServicePacket(service, addresses) {

+1
Attachment #8743006 - Flags: review?(jdarcangelo) → review-
Comment on attachment 8743413 [details] [diff] [review]
7-revamp-service-broadcasting.patch

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

See my inline comments regarding `TryGet()`. It seems unnecessary.

::: netwerk/dns/mdns/libmdns/fallback/DataWriter.jsm
@@ +62,5 @@
>      }
>    }
>  
>    putLabel(label) {
> +    label = label.replace(/\.$/, '');

nit: Maybe add a comment here to clarify we're removing the trailing dots.

::: netwerk/dns/mdns/libmdns/fallback/MulticastDNS.jsm
@@ +28,5 @@
>  function debug(msg) {
>    Services.console.logStringMessage('MulticastDNS: ' + msg);
>  }
>  
> +function ServiceKey(svc) {

+1

@@ +37,5 @@
> +
> +function TryGet(obj, name) {
> +  try {
> +    return obj[name];
> +  } catch (err) {

This `catch` should never occur in the way you're using this function. Every place you are calling `TryGet`, you are guaranteed to be passing an object as the first param. Therefore, if the key you're requesting does not exist, `return obj['somekeyidonthave']` will just return `undefined` and no exception gets thrown. This seems to be adding unnecessary complication. I would just remove it and use `obj[name]` directly in the places where you're calling it.
Attachment #8743413 - Flags: review?(jdarcangelo) → review+
(In reply to Justin D'Arcangelo [:justindarc] from comment #13)
> @@ +37,5 @@
> > +
> > +function TryGet(obj, name) {
> > +  try {
> > +    return obj[name];
> > +  } catch (err) {
> 
> This `catch` should never occur in the way you're using this function. Every
> place you are calling `TryGet`, you are guaranteed to be passing an object
> as the first param. Therefore, if the key you're requesting does not exist,
> `return obj['somekeyidonthave']` will just return `undefined` and no
> exception gets thrown. This seems to be adding unnecessary complication. I
> would just remove it and use `obj[name]` directly in the places where you're
> calling it.

This is because the nsIDNSServiceInfo object throws exceptions if you try to access a property that has not been set.  So the access to serviceType and serviceName and port don't use it (because we verify elsewhere, using TryGet, that they exist), but the remaining properties are allowed to be not set, and thus can throw on access.

It's just the way nsIDNSServiceInfo is coded, kind of a pain to deal with.
(In reply to Justin D'Arcangelo [:justindarc] from comment #9)
> Comment on attachment 8743004 [details] [diff] [review]
> There is an already-defined property for nsIDNSServiceInfo called
> `attributes` that we should probably use instead:
> 
> https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/mdns/
> nsIDNSServiceDiscovery.idl#55

Yeah, later patches add that in.. just structuring the changes in terms of functional deltas for now, so this patch uses the empty object.

> Overall looks good, but should probably have the re-broadcast after 1000ms included now that we are
> properly multicasting the announcements and unicasting direct query responses. Also needs the same
> correction from Pt.4 regarding the `attributes` property of nsIDNSServiceInfo. I like that you cleaned up
> `_makeServicePacket()` for reuse.

Yeah, the rebroadcast and periodic broadcast stuff is in the last patch.  Same with 'attributes' property usage.
Attached patch 6-add-service-broadcasting.patch (obsolete) — Splinter Review
Updated patch, comments addressed.
Attachment #8743006 - Attachment is obsolete: true
Attachment #8743890 - Flags: review?(jdarcangelo)
Sorry, previous patch was the same one as last time.  _THIS_ is the updated patch.
Attachment #8743890 - Attachment is obsolete: true
Attachment #8743890 - Flags: review?(jdarcangelo)
Attachment #8743891 - Flags: review?(jdarcangelo)
Updated patch.  Comments addressed.  Carrying r+ from previous patch.
Attachment #8743892 - Flags: review+
Reposting to obsolete older variants of this patch.
Attachment #8743413 - Attachment is obsolete: true
Attachment #8743892 - Attachment is obsolete: true
Attachment #8743894 - Flags: review+
Comment on attachment 8743004 [details] [diff] [review]
4-only-send-query-response-to-originating-address.patch

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

Asking for a re-r? on the same patch (no changes).  The only comment here was to use the attributes field in the nsIDNSServiceInfo instead of using an empty object.

I prefer leaving that till the last patch for the following reasons: accessing the '.attributes' field could throw, and so this code will be an ugly try/catch that will be discarded anyway with the later TryGet() helper function approach in the last patch.  All the patches are meant to be submitted together and are split up only for readability and review purposes.

So I'd prefer to land this as is and let the later patch take care of carrying attributes from the incoming service info object.
Attachment #8743004 - Flags: review- → review?(jdarcangelo)
Comment on attachment 8743004 [details] [diff] [review]
4-only-send-query-response-to-originating-address.patch

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

::: netwerk/dns/mdns/libmdns/fallback/MulticastDNS.jsm
@@ +89,5 @@
>            address: address,
>            port: aServiceInfo.port,
>            serviceName: aServiceInfo.serviceName,
> +          serviceType: serviceType,
> +          serviceAttrs: {}

There is an already-defined property for nsIDNSServiceInfo called `attributes` that we should probably use instead:

https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/mdns/nsIDNSServiceDiscovery.idl#55
Attachment #8743004 - Flags: review?(jdarcangelo) → review+
Attachment #8743891 - Flags: review?(jdarcangelo) → review+
Comment on attachment 8742996 [details] [diff] [review]
1-fix-flywebservice-publishserver.patch

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

Looks good with the below.

::: dom/flyweb/FlyWebService.cpp
@@ +736,5 @@
>    });
>  
> +  mServers.AppendElement(server);
> +  auto autoRemovePublishedServer = MakeScopeExit([&] {
> +    mServers.RemoveElement(server);

Combine these two scope-exit handlers.
Attachment #8742996 - Flags: review?(jonas) → review+
Changes the parsing of incoming service responses to use the 'A' record in the packet instead of using the ip address of the sender.
Attachment #8744483 - Flags: review?(jdarcangelo)
Comment on attachment 8744483 [details] [diff] [review]
add-a-record-parsing.patch

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

R+ with comments

::: netwerk/dns/mdns/libmdns/fallback/MulticastDNS.jsm
@@ +48,5 @@
> +  if (parts.length != 4) {
> +    return false;
> +  }
> +  for (let part of parts) {
> +    let partInt = Number.parseInt(part);

Should use `parseInt(part, 10)` to ensure it is interpreted as base-10.

@@ +486,5 @@
>          return;
>        }
>  
> +      let host = services[name].host;
> +      let port = services[name].port;

Can't you do `let { host, port } = services[name];` ?
Attachment #8744483 - Flags: review?(jdarcangelo) → review+
This bug is good to close now.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: