1264732 - [FlyWeb] JS implementation of mDNS discovery does not discover some services on Windows

Status: RESOLVED INVALID

(Core :: Networking, defect, P5)

Description

[Kannan Vijayan [:djvj]]

Testing on Windows 10, I found that our JS implementation of mDNS discovery does not see some services that are observed on MacOS, Android, and Linux.  This is odd since the Android and Linux builds should be using the same JS implementation as Windows.

More specifically, the service not being seen is the bonjour '_http._tcp' service exposed by the office printer.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: fix-query-packet-handling.patch

Windows taking a long time to discover services is due to a number of issues.

Firstly, query packets weren't being seen properly.  Secondly, repsonses to query packets weren't being seen by the query machine.

This patch fixes a number of things:

1. Clean ups the 'toJSON' implementation on DNSPacket and DNSRecord to be a bit nicer.
2. Changes query response to only respond to queries if the query name matches the service type.  Before, we would respond with every service, even if the types did not match.
3. Changes behaviour to advertise in response to queries, instead of responding directly to the socket doing the query.  I wasn't able to get direct response working, but broadcast responses worked.  This might need to be looked into in more detail in the future.

Comment 2

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8745573 [details] [diff] [review]
fix-query-packet-handling.patch

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

LGTM. Check my one comment regarding `cacheFlush` on the PTR record. I'm not sure if it is supposed to be set in that case or not. It was previously though.

::: netwerk/dns/mdns/libmdns/fallback/MulticastDNS.jsm
@@ +328,5 @@
>      // PTR Record
>      packet.addRecord('QD', new DNSRecord({
>        name: name,
>        recordType: DNS_RECORD_TYPES.PTR,
> +      classCode: DNS_CLASS_CODES.IN

Shouldn't you also set `cacheFlush: true`?

Comment 3

[Kannan Vijayan [:djvj]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #2)
> Comment on attachment 8745573 [details] [diff] [review]
> fix-query-packet-handling.patch
> 
> Review of attachment 8745573 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM. Check my one comment regarding `cacheFlush` on the PTR record. I'm not
> sure if it is supposed to be set in that case or not. It was previously
> though.
> 
> ::: netwerk/dns/mdns/libmdns/fallback/MulticastDNS.jsm
> @@ +328,5 @@
> >      // PTR Record
> >      packet.addRecord('QD', new DNSRecord({
> >        name: name,
> >        recordType: DNS_RECORD_TYPES.PTR,
> > +      classCode: DNS_CLASS_CODES.IN
> 
> Shouldn't you also set `cacheFlush: true`?

Good call, will do.

Comment 4

[Kannan Vijayan [:djvj]]

Attachment: fix-query-packet-handling.patch

Updated patch.  Comments addressed.  Forwarding r+ from previous patch.

Comment 5

[Kannan Vijayan [:djvj]]

http://hg.mozilla.org/projects/larch/rev/6bd05eded726

Comment 6

[Kannan Vijayan [:djvj]]

Attachment: dont-open-loopback-socket.patch

Simple patch to not open a socket listening for multicast on the loopback address.

Comment 7

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8746629 [details] [diff] [review]
dont-open-loopback-socket.patch

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

LGTM

Comment 8

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 9

[Kannan Vijayan [:djvj]]

Flyweb project is inactive.