Closed Bug 1027350 Opened 5 years ago Closed 5 years ago

not possible to do end-to-end call test on non-networked machines

Categories

(Core :: WebRTC: Networking, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: dmose, Assigned: ekr)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
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.
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).
Attached patch Allow loopback addresses for ICE (obsolete) — Splinter Review
Assignee: nobody → ekr
Status: NEW → ASSIGNED
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)
dmose: note you will need to set "media.peerconnection.ice.loopback" to true.

This may need a better name.
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.
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...
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)
(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)
Attached patch Allow loopback addresses for ICE (obsolete) — Splinter Review
Bit un-rot Ekr's patch
Attachment #8444165 - Attachment is obsolete: true
Attachment #8444165 - Flags: feedback?(dmose)
Attachment #8454701 - Flags: feedback?(dmose)
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).
Attachment #8454701 - Flags: review?(docfaraday)
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).
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.
Attachment #8454701 - Flags: feedback?(dmose) → feedback+
Attached patch Allow loopback addresses for ICE (obsolete) — Splinter Review
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)
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)
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)
Yeah, I think this is the right answer.
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-
Assignee: ekr → drno
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_()
Depends on: 1021220
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)
To the best of my knowledge it doesn't block anything other than convenient development.
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.
(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.
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.
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)
Nils, any chance you can unrot and land this?
Flags: needinfo?(drno)
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)
Attached patch Allow loopback addresses for ICE (obsolete) — Splinter Review
Unrot for interdiff.
Attachment #8499843 - Attachment is obsolete: true
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+
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: drno → ekr
Attachment #8499884 - Flags: review?(docfaraday)
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+
Keywords: checkin-needed
Sheriffs: checkin order is the order the patches are listed in the bug.
https://hg.mozilla.org/mozilla-central/rev/e31ae14f7c5c
https://hg.mozilla.org/mozilla-central/rev/ccf50b47021d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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.