Closed Bug 1296464 Opened 4 years ago Closed 3 years ago

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


(Core Graveyard :: DOM: Flyweb, defect)

Not set


(firefox52 fixed)

Tracking Status
firefox52 --- fixed


(Reporter: djvj, Unassigned)




(4 files, 3 obsolete files)

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 "" or "").
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.
Attached patch fix-multiple-interface-bug.patch (obsolete) — Splinter Review
The previous patch was the wrong patch (unrelated bug).  This is the right one.
Attachment #8785453 - Attachment is obsolete: true
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)
Comment on attachment 8786372 [details] [diff] [review]

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]

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+
This patch just splits out the addition of 'init2' method to nsIUDPSocket.  Forwarding r+ from previous (larger) patch.
Attachment #8790465 - Flags: review+
Keywords: leave-open
Pushed by
Part 1 - Add init2 method to nsIUDPSocket to allow opening sockets on specific IPs. r=hurley
Attached patch bug1296464-mdns.patch (obsolete) — Splinter Review
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 ( 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 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)
Attached patch bug1296464.patchSplinter Review
Attachment #8791686 - Attachment is obsolete: true
Attachment #8791686 - Flags: review?(kvijayan)
Attachment #8792111 - Flags: review?(kvijayan)
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.
Comment on attachment 8792111 [details] [diff] [review]

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)
Comment on attachment 8793003 [details] [diff] [review]

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+
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
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Product: Toolkit → Core
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.