Closed
Bug 1027350
Opened 10 years ago
Closed 10 years ago
not possible to do end-to-end call test on non-networked machines
Categories
(Core :: WebRTC: Networking, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: dmosedale, Assigned: ekr)
References
Details
Attachments
(2 files, 4 obsolete files)
34.15 KB,
patch
|
bwc
:
review+
|
Details | Diff | Splinter Review |
8.11 KB,
patch
|
bwc
:
review+
|
Details | Diff | Splinter Review |
Eg in offline mode on plane. This is because ICE always ignores 127.0.0.1 when trying to connect calls. EKR and I talked about simply having a hidden pref that we could set in the profiles used for test runs which would allow ICE to gather from 127.0.0.1.
Reporter | ||
Comment 1•10 years ago
|
||
I've been told this is an issue for B2G testing as well, and implementing this would allow a bunch of complex code there to be removed.
Comment 2•10 years ago
|
||
I just thinking that the implementation of this should not only control if we add loopback IPs to its own SDP, but also only pay attention to loopback IPs in remote SDP's if it is turned on (to protect us from broken far end implementations).
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → ekr
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8444165 [details] [diff] [review] Allow loopback addresses for ICE Review of attachment 8444165 [details] [diff] [review]: ----------------------------------------------------------------- WIP. Not yet reviewed by me. Dmose, can you see if this works for you?
Attachment #8444165 -
Flags: feedback?(dmose)
Assignee | ||
Comment 5•10 years ago
|
||
dmose: note you will need to set "media.peerconnection.ice.loopback" to true. This may need a better name.
Reporter | ||
Comment 6•10 years ago
|
||
Comment on attachment 8444165 [details] [diff] [review] Allow loopback addresses for ICE Talked to ekr on IRC; should be able to get to this in a small number of days. Leaving feedback? so I don't forget.
Reporter | ||
Comment 7•10 years ago
|
||
Sorry for not yet having gotten to this; this week has been a little nuts. I still very much appreciate this work and look forward to it unblocking important stuff...
Reporter | ||
Comment 8•10 years ago
|
||
I was hoping to try this with the Talkilla functional tests, but for some reason, while they work with the download nightly, they don't work with the nightly that I build. Standard8, might you have insight on this? I tried to use loop to test it by hand, but right now we're depending on having the Mozilla push infrastructure available, so our functional testing story is going to need be a bit further along before I can really try this. Sorry for the delay!
Flags: needinfo?(standard8)
Comment 9•10 years ago
|
||
(In reply to Dan Mosedale (:dmose - needinfo? me for responses) from comment #8) > I was hoping to try this with the Talkilla functional tests, but for some > reason, while they work with the download nightly, they don't work with the > nightly that I build. Standard8, might you have insight on this? My only thought is that something has changed that has broken them. If the browser starts but doesn't do anything, that's typically browser incompatibility - but I just looked, and discovered that last time I did a maxversion bump, I bumped the max to 50.*... so hence shouldn't be an issue. If they are starting up & not doing anything, take a look at the add-ons manager. That might give you an indication.
Flags: needinfo?(standard8)
Comment 10•10 years ago
|
||
Bit un-rot Ekr's patch
Attachment #8444165 -
Attachment is obsolete: true
Attachment #8444165 -
Flags: feedback?(dmose)
Attachment #8454701 -
Flags: feedback?(dmose)
Comment 11•10 years ago
|
||
For me the patch works as expected. After turning on the pref the checks for absence of loopback fail. I see 127.0.0.1 in the SDP. And a basicAudio mochitest works with WiFi turned off (only loopback shows up in the SDP, but call still gets connected).
Comment 12•10 years ago
|
||
Try run: https://tbpl.mozilla.org/?tree=Try&rev=e047b8905977
Updated•10 years ago
|
Attachment #8454701 -
Flags: review?(docfaraday)
Comment 13•10 years ago
|
||
Before we land this I think we need to add tests which verify the Pref working as expected (run with Pref true and verify, turn off Pref and verify again).
Reporter | ||
Comment 14•10 years ago
|
||
Nils, based on your description, that sounds great to me. Because our functional testing stuff is blocked, this fell down my priority list to test more. If you're happy with it, I'd suggest it'd be fine to land sooner rather than later, and if I stumbled upon additional issues in the future (which I don't anticipate), I'll open another bug.
Reporter | ||
Updated•10 years ago
|
Attachment #8454701 -
Flags: feedback?(dmose) → feedback+
Comment 15•10 years ago
|
||
Fixed a wrong parameter in the new loopback unit test. Try run looks green now: https://tbpl.mozilla.org/?tree=Try&rev=1902aed4223d
Attachment #8454701 -
Attachment is obsolete: true
Attachment #8454701 -
Flags: review?(docfaraday)
Attachment #8454967 -
Flags: review?(docfaraday)
Comment 16•10 years ago
|
||
Realized that the new Pref media.peerconnection.ice.loopback is if-def'ed with MOZILLA_INTERNAL_API. Does this mean it is only available on certain builds, or do all builds including release builds this set?
Flags: needinfo?(ekr)
Comment 17•10 years ago
|
||
So, the code wrapped in ifdef pertains only to about:config, which only exists in the MOZILLA_INTERNAL_API variant. This might be enough to allow you to run mochitest over loopback, for example. For unit-tests, you have to set the param on the NrIceCtx directly.
Flags: needinfo?(ekr)
Assignee | ||
Comment 18•10 years ago
|
||
Yeah, I think this is the right answer.
Comment 19•10 years ago
|
||
Comment on attachment 8454967 [details] [diff] [review] Allow loopback addresses for ICE Review of attachment 8454967 [details] [diff] [review]: ----------------------------------------------------------------- Mostly minor stuff, although putting the NrIceCtx in the packet filter test seems strange to me, and I'd either need an explanation why it is needed, or removal, before I can give r+. ::: media/mtransport/test/ice_unittest.cpp @@ +83,5 @@ > return std::string(); > } > > +static std::string IsLoopbackCandidate(const std::string& candidate) { > + if (candidate.find("127.0.0.1") != std::string::npos) { Maybe make the pattern "127.0.0.", since loopback is more than just .1? @@ +239,5 @@ > expected_local_type_(NrIceCandidate::ICE_HOST), > expected_local_transport_(kNrIceTransportUdp), > expected_remote_type_(NrIceCandidate::ICE_HOST), > + expected_local_addr_(""), > + expected_remote_addr_(""), These are probably not necessary, since this is what the default c'tor does anyway. @@ +529,2 @@ > << ":" > + << port Thanks for fixing this. @@ +881,5 @@ > + void EnsurePeer() { > + if (!peer_) { > + peer_ = new IceTestPeer("P1", true, false); > + peer_->AddStream(1); > + } Maybe have EnsurePeer() return a pointer to peer_, so we can simplify some of the test code a bit? @@ +1160,5 @@ > > void SetUp() { > + // Set up enough of the ICE ctx to allow STUN to work. > + ice_ctx_ = NrIceCtx::Create("test", true); > + Why do we need this for the packet filter test? @@ +1368,5 @@ > ASSERT_TRUE(Gather()); > Connect(); > } > > +TEST_F(IceConnectTest, TestLoopbackOnlySorrtOf) { Typo. ::: media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c @@ +694,5 @@ > } > > if (mapped_addr) { > if (nr_stun_message_has_attribute(ctx->response, NR_STUN_ATTR_XOR_MAPPED_ADDRESS, &attr)) { > + if ((r=nr_stun_transport_addr_check(&attr->u.xor_mapped_address.unmasked, Looks like your editor messed with the indent. @@ +703,5 @@ > ABORT(r); > } > else if (nr_stun_message_has_attribute(ctx->response, NR_STUN_ATTR_MAPPED_ADDRESS, &attr)) { > + if ((r=nr_stun_transport_addr_check(&attr->u.mapped_address, > + ctx->mapped_addr_check_mask))) Similar thing here. ::: media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.h @@ +171,5 @@ > UINT4 rto_ms; /* retransmission time out */ > double retransmission_backoff_factor; > UINT4 maximum_transmits; > UINT4 final_retransmit_backoff_ms; > + UINT4 mapped_addr_check_mask; // What checks to run on mapped addresses. Use c comment. Also applies to relay addresses. ::: media/mtransport/third_party/nICEr/src/stun/stun_reg.h @@ +43,5 @@ > #define NR_STUN_REG_PREF_CLNT_RETRANSMIT_BACKOFF "stun.client.retransmission_backoff_factor" > #define NR_STUN_REG_PREF_CLNT_MAXIMUM_TRANSMITS "stun.client.maximum_transmits" > #define NR_STUN_REG_PREF_CLNT_FINAL_RETRANSMIT_BACKOFF "stun.client.final_retransmit_backoff" > > +#define NR_STUN_REG_ALLOW_LOOPBACK_ADDRS "stun.allow_loopback" For consistency, this should probably be NR_STUN_REG_PREF_ALLOW_LOOPBACK.
Attachment #8454967 -
Flags: review?(docfaraday) → review-
Updated•10 years ago
|
Assignee: ekr → drno
Assignee | ||
Comment 20•10 years ago
|
||
Comment on attachment 8454967 [details] [diff] [review] Allow loopback addresses for ICE Review of attachment 8454967 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/mtransport/test/ice_unittest.cpp @@ +239,5 @@ > expected_local_type_(NrIceCandidate::ICE_HOST), > expected_local_transport_(kNrIceTransportUdp), > expected_remote_type_(NrIceCandidate::ICE_HOST), > + expected_local_addr_(""), > + expected_remote_addr_(""), Convention in this code is to initialize everything. See, for instance, streams_()
Comment 21•10 years ago
|
||
this bug is showing as blocking a bug we're targeting to fix in Fx34. How far along is it? Is this a true blocker for bug 976116?
Flags: needinfo?(drno)
Assignee | ||
Comment 22•10 years ago
|
||
To the best of my knowledge it doesn't block anything other than convenient development.
Reporter | ||
Comment 23•10 years ago
|
||
It blocks automated testing of end-to-end calls in Tbpl, as the dependency indicates, because Tbpl tests that require non-local access to network resources won't be (and shouldn't be) allowed to land, as they will cause intermittent failures.
Assignee | ||
Comment 24•10 years ago
|
||
(In reply to Dan Mosedale (:dmose - needinfo? me for responses) from comment #23) > It blocks automated testing of end-to-end calls in Tbpl, as the dependency > indicates, because Tbpl tests that require non-local access to network > resources won't be (and shouldn't be) allowed to land, as they will cause > intermittent failures. You don't need non-local access, you just need non-localhost IP addresses. In any case, this seems like a category error, since if we can't use off-machine network resources, we can't do realistic webrtc testing. That's what needs to be fixed, not making the tests even more unrealistic.
Reporter | ||
Comment 25•10 years ago
|
||
I agree that there are multiple categories of tests that need to happen, but both can be useful. In this concrete example, my expectation/hope is that the functional tests we're writing will have env var settings so that they can be used both with real infrastructure (ie an end-to-end call test using the live infrastructure where the intent is to test the whole system), as well as with mocked infrastructure (for continuous integration environments, where the intent is to catch the most common problems as soon as they're introduced). For the Tbpl (CI) use case, which I think is pretty critical, I think this does depend.
Comment 26•10 years ago
|
||
I don't think this is a true blocker. We have working solutions for making test calls on TBPL. Both solutions require to activate some special testing code. That being said, as indicated above this depends on bug 1021220. I think 1021220 got a final review. Once Byron is happy with it I'm going to add the tests to this bug here. So we are left with a few hours, I would estimate as 1.
Flags: needinfo?(drno)
Assignee | ||
Comment 27•10 years ago
|
||
Nils, any chance you can unrot and land this?
Flags: needinfo?(drno)
Comment 28•10 years ago
|
||
Un-rot. Addressed comments. Do not have the time to address EnsurePeer() return value. According to a conversation we had the ice_ctx_ is need for the packet filter now to work properly. Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=53436edf9c70
Attachment #8454967 -
Attachment is obsolete: true
Attachment #8499835 -
Flags: review?(docfaraday)
Flags: needinfo?(drno)
Comment 29•10 years ago
|
||
Unrot for interdiff.
Updated•10 years ago
|
Attachment #8499843 -
Attachment is obsolete: true
Comment 30•10 years ago
|
||
Comment on attachment 8499835 [details] [diff] [review] 0001-Bug-1027350-Allow-loopback-addresses-for-ICE.patch Review of attachment 8499835 [details] [diff] [review]: ----------------------------------------------------------------- lgtm
Attachment #8499835 -
Flags: review?(docfaraday) → review+
Assignee | ||
Comment 31•10 years ago
|
||
From d490dacb3febabf4dfa9240bd68001063c8b9a98 Mon Sep 17 00:00:00 2001 unit test --- .../signaling/src/peerconnection/PeerConnectionImpl.cpp | 2 ++ .../signaling/src/peerconnection/PeerConnectionImpl.h | 4 ++++ .../src/peerconnection/PeerConnectionMedia.cpp | 17 ++++++++--------- .../signaling/src/peerconnection/PeerConnectionMedia.h | 6 ++++++ media/webrtc/signaling/test/signaling_unittests.cpp | 2 +- 5 files changed, 21 insertions(+), 10 deletions(-)
Assignee | ||
Updated•10 years ago
|
Assignee: drno → ekr
Assignee | ||
Updated•10 years ago
|
Attachment #8499884 -
Flags: review?(docfaraday)
Comment 32•10 years ago
|
||
Comment on attachment 8499884 [details] [diff] [review] [PATCH] - Allow loopback addresses for ICE in signaling Review of attachment 8499884 [details] [diff] [review]: ----------------------------------------------------------------- lgtm
Attachment #8499884 -
Flags: review?(docfaraday) → review+
Assignee | ||
Comment 33•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=eb5b7d946e1c
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 34•10 years ago
|
||
Sheriffs: checkin order is the order the patches are listed in the bug.
Comment 35•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e31ae14f7c5c https://hg.mozilla.org/integration/mozilla-inbound/rev/ccf50b47021d
Flags: in-testsuite+
Keywords: checkin-needed
Comment 36•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e31ae14f7c5c https://hg.mozilla.org/mozilla-central/rev/ccf50b47021d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Reporter | ||
Comment 37•10 years ago
|
||
Thanks, guys! This definitely brings us closer to automatically testable calls.
You need to log in
before you can comment on or make changes to this bug.
Description
•