Note: There are a few cases of duplicates in user autocompletion which are being worked on.

[FlyWeb] Machines with multiple interfaces cause duplicate advertisements on all interfaces

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM: Flyweb
RESOLVED FIXED
11 months ago
9 months ago

People

(Reporter: djvj, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

11 months ago
Saw this on my windows box, which has a virtualbox-created virtual interface on 192.168.1.*.

When advertising a service, even the public, network-facing interface has two advertisements sent out: one for the public IF, and one for the virtual IF.

I think this is because nsIUDPSocket.init() doesn't let you open sockets on a specific ip (you only get to choose "0.0.0.0" or "127.0.0.1").
(Reporter)

Comment 1

11 months ago
Created attachment 8785453 [details] [diff] [review]
add-publish-server-user-prompt.patch

This patch seems to work.  It involves adding a new "init2" method to nsIUDPSocket that allows opening UDP sockets on specific interface IPs.  This seems to somehow disable receipt of multicast packets using those sockets, so we use the "broadcast" socket to receive those and use the per-interface sockets to advertise responses.

It does mean that for every query we receive, we will broadcast a response on all interfaces, which is a bit spammy, but not that bad.
(Reporter)

Comment 2

11 months ago
Created attachment 8785982 [details] [diff] [review]
fix-multiple-interface-bug.patch

The previous patch was the wrong patch (unrelated bug).  This is the right one.
Attachment #8785453 - Attachment is obsolete: true
(Reporter)

Comment 3

11 months ago
Created attachment 8786372 [details] [diff] [review]
fix-multiple-interface-bug.patch

Updated patch with debug messages removed.  Nick do you mind taking a look at this?

I'm adding a new initializer to nsIUDPSocket so that our mDNS implementation can open sockets specific to particular interfaces.
Attachment #8785982 - Attachment is obsolete: true
Attachment #8786372 - Flags: review?(hurley)
Blocks: 1298463
Comment on attachment 8786372 [details] [diff] [review]
fix-multiple-interface-bug.patch

Kannan: The JS parts need some additional reworking in this patch. When this patch is applied, if the FlyWeb tab remains open on Fennec for a short period of time, it causes the entire browser to freeze up. Eventually, when it unfreezes, you see the ADB logcat get flooded with a backlog of query packets. I think the logic around our socket management needs some additional rework before landing this. I'm almost thinking it might make sense to introduce a `MulticastDNSNetworkIface()` class that essentially does the bulk of the work from `MulticastDNS()`, except only for a single interface. It may make the code more manageable. Then again, maybe it wouldn't. I can take a closer look into this if you don't have the bandwidth. It might make sense to split this up into two patches. One that just adds `init2()` and the other that updates the mDNS impl.
Attachment #8786372 - Flags: feedback-
Comment on attachment 8786372 [details] [diff] [review]
fix-multiple-interface-bug.patch

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

So this all looks decent to me, but take :justindarc's feedback in mind before landing (and update the patch, re-requesting review as appropriate).

::: netwerk/base/nsUDPSocket.cpp
@@ +586,5 @@
> +  }
> +
> +  NetAddr addr;
> +
> +  if (aPort < 0)

Braces around body
Attachment #8786372 - Flags: review?(hurley) → review+
(Reporter)

Comment 6

10 months ago
Created attachment 8790465 [details] [diff] [review]
add-init2-to-nsiudpsocket.patch

This patch just splits out the addition of 'init2' method to nsIUDPSocket.  Forwarding r+ from previous (larger) patch.
Attachment #8790465 - Flags: review+
(Reporter)

Updated

10 months ago
Keywords: leave-open

Comment 7

10 months ago
Pushed by kvijayan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2a771eb073a
Part 1 - Add init2 method to nsIUDPSocket to allow opening sockets on specific IPs. r=hurley

Comment 8

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f2a771eb073a
Created attachment 8791686 [details] [diff] [review]
bug1296464-mdns.patch

Kannan: Here's the patch for fixing the mDNS impl to use the new init2() initializer on nsIUDPSocket. I eliminated the _broadcastSocket since it seems like we only actually need a single multicast socket on all interfaces (0.0.0.0) to handle it properly. I also removed the per-interface sockets from the multicast group since they do not actually need to listen for multicast packets (the 0.0.0.0 multicast socket is already listening for them). This patch fixes the issue for me where I wasn't able to see an ESP8266. This patch is also not exhibiting the eventual freeze-ups after prolonged use that the original patch was doing (I think it may have been caused by having all the sockets joining the multicast group).
Attachment #8791686 - Flags: review?(kvijayan)
Blocks: 1299866
Created attachment 8792111 [details] [diff] [review]
bug1296464.patch
Attachment #8791686 - Attachment is obsolete: true
Attachment #8791686 - Flags: review?(kvijayan)
Attachment #8792111 - Flags: review?(kvijayan)
(Reporter)

Comment 11

10 months ago
Created attachment 8793003 [details] [diff] [review]
fix-multiple-interfaces-bug.patch

Hey Justin,

Just tested this against windows, and aside from a minor issue it works fine.  I've attached a patch with the relevant changes here:

1. We add a call to _getBroadcastRecvSocket() to registerService.  Previously the patch only added the bcast recv socket instantiation to the 'query' method, which meant that if we just published a service, and the user never tried to do discovery from the host machine, the broadcast socket would not be initialized.

2. Removed the handler for the per-interface sockets.  Seems to work fine.
(Reporter)

Comment 12

10 months ago
Comment on attachment 8792111 [details] [diff] [review]
bug1296464.patch

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

I'm canceling review on this patch and r+-ing the slightly updated patch I posted.
Attachment #8792111 - Flags: review?(kvijayan)
(Reporter)

Comment 13

10 months ago
Comment on attachment 8793003 [details] [diff] [review]
fix-multiple-interfaces-bug.patch

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

r+-ing this patch, which is basically Justin's patch with a couple of small modifications (see attachment comment).
Attachment #8793003 - Flags: review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6bd76a14463
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd99c422dca35d2ce039d3774710680095fe4977
Bug 1296464 - [FlyWeb] Machines with multiple interfaces cause duplicate advertisements on all interfaces;r=djvj
Removing leave-open keyword so this bug can be closed once it makes it into m-c.
Keywords: leave-open

Comment 17

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bd99c422dca3
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Updated

9 months ago
Product: Toolkit → Core
You need to log in before you can comment on or make changes to this bug.