Closed
Bug 1296464
Opened 8 years ago
Closed 8 years ago
[FlyWeb] Machines with multiple interfaces cause duplicate advertisements on all interfaces
Categories
(Core Graveyard :: DOM: Flyweb, defect)
Core Graveyard
DOM: Flyweb
Tracking
(firefox52 fixed)
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: djvj, Unassigned)
References
Details
Attachments
(4 files, 3 obsolete files)
6.51 KB,
patch
|
u408661
:
review+
justindarc
:
feedback-
|
Details | Diff | Splinter Review |
2.34 KB,
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
9.18 KB,
patch
|
Details | Diff | Splinter Review | |
10.02 KB,
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
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•8 years ago
|
||
The previous patch was the wrong patch (unrelated bug). This is the right one.
Attachment #8785453 -
Attachment is obsolete: true
Reporter | ||
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
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•8 years ago
|
||
This patch just splits out the addition of 'init2' method to nsIUDPSocket. Forwarding r+ from previous (larger) patch.
Attachment #8790465 -
Flags: review+
Reporter | ||
Updated•8 years ago
|
Keywords: leave-open
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f2a771eb073a
Comment 9•8 years ago
|
||
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)
Comment 10•8 years ago
|
||
Attachment #8791686 -
Attachment is obsolete: true
Attachment #8791686 -
Flags: review?(kvijayan)
Attachment #8792111 -
Flags: review?(kvijayan)
Reporter | ||
Comment 11•8 years ago
|
||
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•8 years 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•8 years 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+
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd99c422dca35d2ce039d3774710680095fe4977 Bug 1296464 - [FlyWeb] Machines with multiple interfaces cause duplicate advertisements on all interfaces;r=djvj
Comment 16•8 years ago
|
||
Removing leave-open keyword so this bug can be closed once it makes it into m-c.
Keywords: leave-open
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bd99c422dca3
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee | ||
Updated•8 years ago
|
Product: Toolkit → Core
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•