Closed
Bug 1326483
Opened 8 years ago
Closed 8 years ago
UDP socket child allows network connectivity in offline mode
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: drno, Assigned: schien)
Details
(Whiteboard: [necko-active])
Attachments
(2 files)
Currently WebRTC's PeerConnection.js monitors and clones the network online/offline state to prevent the usage of WebRTC in offline mode.
I think the parent process should return errors to the UDP socket child implementation in case it tries to access to network (anything else then loopback).
Note: currently WebRTC's UDP socket child implementation can create and use network sockets even when Fx is in the offline mode.
Updated•8 years ago
|
Component: Networking → WebRTC: Networking
Reporter | ||
Comment 1•8 years ago
|
||
@michal: sorry this is not a WebRTC Networking bug, as this the problem here is not in WebRTC's networking code.
I reported this as in Networking as I think this should be fixed in the UDPSocketParent class, which according to some quick blame annotations has been used before to implement the UDPSocketParent class.
The WebRTC Networking folks can certainly help fixing this, e.g. by providing patches, once we have consensus from the Necko folks where to fix this.
Component: WebRTC: Networking → Networking
Updated•8 years ago
|
Assignee: nobody → schien
Whiteboard: [necko-active]
Comment 2•8 years ago
|
||
makes sense to me nils. thanks for noting the localhost bit
Assignee | ||
Comment 3•8 years ago
|
||
I'll recommend to add offline mode check in nsUDPSocket since there are usages that directly access nsUDPSocket without going through IPC.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•8 years ago
|
||
There are 5 users of nsUDPSocket in mozilla-central.
1. WebRTC: use via UDPSocket Parent, already handle the offline mode scenario.
2. UDPSocket WebAPI: use via UDPSocketParent, will throw NetworkError to content JS.
3. SimpleServiceDiscovery.jsm: use directly, have a retry timer if failed to create socket.
4. MulticastDNS.jsm: use directly, doesn't handle socket creation failure
5. DevTool: use directly, socket creation failure is caught but do nothing.
I'll file follow-up bugs for MulticastDNS.jsm and DevTool to notify them adding error handling.
Assignee | ||
Comment 7•8 years ago
|
||
Reporter | ||
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8823495 [details]
Bug 1326483 - Part 1, not allow non-loopback UDP socket be created in offline mode.
https://reviewboard.mozilla.org/r/102020/#review102400
I think the CheckIOStatus() call should also be made from the send() function, because WebRTC never connects it's UDP sockets. It leaves them unconnected and calls sendto() on the socket to send to different destinations.
Assignee | ||
Comment 9•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8823495 [details]
Bug 1326483 - Part 1, not allow non-loopback UDP socket be created in offline mode.
https://reviewboard.mozilla.org/r/102020/#review102400
IIRC, UDP socket will be detached once entering offline. Therefore, either checking attached state or calling `CheckIOStatus()` will be enough.
Updated•8 years ago
|
Attachment #8823495 -
Flags: review?(mcmanus)
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8823495 [details]
Bug 1326483 - Part 1, not allow non-loopback UDP socket be created in offline mode.
https://reviewboard.mozilla.org/r/102020/#review102574
Attachment #8823495 -
Flags: review?(mcmanus)
Updated•8 years ago
|
Attachment #8823495 -
Flags: review?(mcmanus) → review?(drno)
Reporter | ||
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8823495 [details]
Bug 1326483 - Part 1, not allow non-loopback UDP socket be created in offline mode.
https://reviewboard.mozilla.org/r/102020/#review102734
LGTM.
Manually verified that WebRTC no longer can make calls in offline mode. But it works for loopback with the fix mentioned below.
::: netwerk/base/nsUDPSocket.cpp:618
(Diff revision 2)
> {
> NS_ENSURE_TRUE(mFD == nullptr, NS_ERROR_ALREADY_INITIALIZED);
>
> - if (gIOService->IsNetTearingDown()) {
> - return NS_ERROR_FAILURE;
> + nsresult rv;
> +
> + rv = CheckIOStatus(&mAddr);
You need to use |aAddr| here instead of |mAddr|.
Attachment #8823495 -
Flags: review?(drno) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•8 years ago
|
||
Comment on attachment 8823495 [details]
Bug 1326483 - Part 1, not allow non-loopback UDP socket be created in offline mode.
you guys make a good team for udpsocket - keep it up :)
Attachment #8823495 -
Flags: review?(mcmanus) → review?(drno)
Updated•8 years ago
|
Attachment #8823962 -
Flags: review?(mcmanus) → review?(drno)
Reporter | ||
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8823495 [details]
Bug 1326483 - Part 1, not allow non-loopback UDP socket be created in offline mode.
https://reviewboard.mozilla.org/r/102020/#review103132
Attachment #8823495 -
Flags: review?(drno) → review+
Reporter | ||
Comment 16•8 years ago
|
||
mozreview-review |
Comment on attachment 8823962 [details]
Bug 1326483 - Part 2, test cases.
https://reviewboard.mozilla.org/r/102440/#review103268
Attachment #8823962 -
Flags: review?(drno) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•8 years ago
|
||
Hmm...test_udpsocket_offline.js is failed on Win 7 because fail to bind socket to ipv6 loopback address '::1'.
>WARNING: failed to bind socket: file c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/netwerk/base/nsUDPSocket.cpp, line 661
Any idea?
Assignee | ||
Comment 20•8 years ago
|
||
Maybe related to bug 892019?
Reporter | ||
Comment 21•8 years ago
|
||
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #20)
> Maybe related to bug 892019?
Quite possible. In all of our WebRTC tests we always check if IPv6 is available and skip tests if it's missing, because otherwise too many test fail :-(
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 24•8 years ago
|
||
(In reply to Nils Ohlmeier [:drno] from comment #21)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #20)
> > Maybe related to bug 892019?
>
> Quite possible. In all of our WebRTC tests we always check if IPv6 is
> available and skip tests if it's missing, because otherwise too many test
> fail :-(
I just figured out the issue on Win7. PR_InitializeNetAddr doesn't full memset 0 the ipv6 structure [1]. Therefore, PRNetAddr.ipv6.scope_id will have random value that cause PR_Bind fail. Test passed after manually memset 0 the PRNetAddr.
[1] http://searchfox.org/mozilla-central/source/nsprpub/pr/src/misc/prnetdb.c#1408
Assignee | ||
Comment 25•8 years ago
|
||
@mcmanus, `PR_InitializeNetAddr(PR_IpAddrNull, aPort, &prAddr)` will only memset 0 the ipv4 struct, however, ipv6 struct is larger than that. The last field of PRNetAddr.ipv6, i.e. scope_id, is most likely to have random value. Do you think this is something we should fix in NSPR?
Flags: needinfo?(mcmanus)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 28•8 years ago
|
||
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #25)
> @mcmanus, `PR_InitializeNetAddr(PR_IpAddrNull, aPort, &prAddr)` will only
> memset 0 the ipv4 struct, however, ipv6 struct is larger than that. The last
> field of PRNetAddr.ipv6, i.e. scope_id, is most likely to have random value.
> Do you think this is something we should fix in NSPR?
good find.
yes it should be fixed in nspr.. can you also provide a wrapper for gecko that zeroes it before calling nspr. That way we don't have to worry about syncing the library. (I'm assuming gecko is allocating the structure so its got a chance to do that. not sure of the stack)
Flags: needinfo?(mcmanus)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 31•8 years ago
|
||
Pushed by schien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0de6247a69de
Part 1, not allow non-loopback UDP socket be created in offline mode. r=drno
https://hg.mozilla.org/integration/autoland/rev/c858f870c6d7
Part 2, test cases. r=drno
Assignee | ||
Comment 32•8 years ago
|
||
(In reply to Patrick McManus [:mcmanus] from comment #28)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #25)
> > @mcmanus, `PR_InitializeNetAddr(PR_IpAddrNull, aPort, &prAddr)` will only
> > memset 0 the ipv4 struct, however, ipv6 struct is larger than that. The last
> > field of PRNetAddr.ipv6, i.e. scope_id, is most likely to have random value.
> > Do you think this is something we should fix in NSPR?
>
> good find.
>
> yes it should be fixed in nspr.. can you also provide a wrapper for gecko
> that zeroes it before calling nspr. That way we don't have to worry about
> syncing the library. (I'm assuming gecko is allocating the structure so its
> got a chance to do that. not sure of the stack)
Bug 1330490 is filed for the NSPR fix, patch uploaded for review as well. Not sure about the exact code should look like for the wrapper, however I've file bug 1330543 to follow up.
Comment 33•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0de6247a69de
https://hg.mozilla.org/mozilla-central/rev/c858f870c6d7
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•