Closed Bug 864118 Opened 7 years ago Closed 5 years ago

Enhance existing WebRTC mochitest framework to test against a TURN server

Categories

(Core :: WebRTC, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jsmith, Assigned: drno)

References

(Blocks 1 open bug)

Details

(Whiteboard: [WebRTC][blocking-webrtc-][turn][p=2, s=fx33])

Attachments

(1 file, 2 obsolete files)

We've landed support for TURN, but we're not running mochitest automation against it. This bug tracks the work to enhance the WebRTC mochitest automation to allow it to test against a TURN server vs. STUN server in a configurable manner.

Note - a lot of the tests will have some similarities running under STUN and TURN, so we need to be mindful of test reusability here.
Blocks: pc-tests
Priority: -- → P3
Whiteboard: [WebRTC][blocking-webrtc-][turn]
Depends on: 865296
Note: gated on having a TURN server available
Assignee: nobody → drno
Whiteboard: [WebRTC][blocking-webrtc-][turn] → [WebRTC][blocking-webrtc-][turn][p=2]
Target Milestone: --- → mozilla32
Depends on: 1010007
After discussing with Nils and Randell, we're going to move this to Fx 33.
No longer blocks: 970426
Target Milestone: mozilla32 → mozilla33
Whiteboard: [WebRTC][blocking-webrtc-][turn][p=2] → [WebRTC][blocking-webrtc-][turn][p=2, s=fx33]
Target Milestone: mozilla33 → mozilla35
Simple verification of what the RTCP stats tell us which ICE candidates are in use.
Attachment #8517067 - Flags: review?(docfaraday)
Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3138bbd22d77

BTW this is mostly for running in our new QA lab where some machines are setup to require TURN.
Fixed bugs from previous try run.

New try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8b9d850ba5b5
Attachment #8517067 - Attachment is obsolete: true
Attachment #8517067 - Flags: review?(docfaraday)
Attachment #8517158 - Flags: review?(docfaraday)
Comment on attachment 8517158 [details] [diff] [review]
bug_864118_add_turn_verification.patch

Review of attachment 8517158 [details] [diff] [review]:
-----------------------------------------------------------------

Need some fixes here I think.

::: dom/media/tests/mochitest/pc.js
@@ +2573,5 @@
> +   */
> +  checkStatsIceConnectionType : function PCW_checkStatsIceConnectionType(stats)
> +  {
> +    if (Object.keys(stats).length === 0) {
> +      info("checkStatsIceConnectionType called with emtpy stats");

s/emtpy/empty/

@@ +2585,5 @@
> +        lId = stats[name].localCandidateId;
> +        rId = stats[name].remoteCandidateId;
> +      }
> +    });
> +    info("Veryfying: local=" + JSON.stringify(stats[lId]) +

s/Veryfying/Verifying/

@@ +2593,5 @@
> +    var lIp = stats[lId].ipAddress;
> +    var rIp = stats[rId].ipAddress;
> +    if ((this.configuration) && (typeof this.configuration.iceServers !== 'undefined')) {
> +      info("Ice Server configured");
> +      var serverIp = this.configuration.iceServers[0].url.split(':')[1];

Hmm. This leaves a gotcha in place if we ever try to use an FQDN, but this is primarily intended for use in the lab, so maybe ok. I would add a note to the config-file warning about this.

@@ +2595,5 @@
> +    if ((this.configuration) && (typeof this.configuration.iceServers !== 'undefined')) {
> +      info("Ice Server configured");
> +      var serverIp = this.configuration.iceServers[0].url.split(':')[1];
> +      ok((lType === "relayed" || rType === "relayed") ||
> +         (lIp === serverIp || rIp === serverIp), "One peer uses a relay");

I think that second '||' was intended to be an '&&'? Even so, I'm a little worried about the intent here. Even if we use a TURN server, we will only select pairs that use it if there is no direct connectivity. Are we assuming that a configured TURN server implies that we're doing a test that will prevent direct connectivity? Additionally, we still have the relayed-classified-as-peer-reflexive corner-case to worry about.

@@ +2600,5 @@
> +    } else {
> +      info("P2P configured");
> +      ok(((lType !== "relayed") && (rType !== "relayed")), "Pure peer to peer call without a relay");
> +    }
> +    info("done");

Do we need this?

::: dom/media/tests/mochitest/templates.js
@@ +23,5 @@
>    if (typeof test._remote_answer !== 'undefined') {
>      dump("ERROR: SDP answer: " + test._remote_answer.sdp.replace(/[\r]/g, ''));
>    }
>  
> +  if ((test.pcLocal) && (typeof test.pcLocal._local_ice_candidates !== 'undefined')) {

This typeof check seems strange; what about _remote_ice_candidates and _ice_candidates_to_add? Should those not be logged if there are no local? Are we willing to log these as "undefined"? I'd be ok with logging "undefined" for all of these, but that's just me.

@@ +28,5 @@
> +    dump("pcLocal._local_ice_candidates: " + JSON.stringify(test.pcLocal._local_ice_candidates) + "\n");
> +    dump("pcLocal._remote_ice_candidates: " + JSON.stringify(test.pcLocal._remote_ice_candidates) + "\n");
> +    dump("pcLocal._ice_candidates_to_add: " + JSON.stringify(test.pcLocal._ice_candidates_to_add) + "\n");
> +  }
> +  if ((test.pcRemote) && (typeof test.pcRemote._local_ice_candidates !== 'undefined')) {

Same question here.
Attachment #8517158 - Flags: review?(docfaraday) → review-
Addressed some of bwc's comments.

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8694d6877e8a
Attachment #8517158 - Attachment is obsolete: true
Attachment #8521012 - Flags: review?(docfaraday)
(In reply to Byron Campen [:bwc] from comment #6)
> @@ +2593,5 @@
> > +    var lIp = stats[lId].ipAddress;
> > +    var rIp = stats[rId].ipAddress;
> > +    if ((this.configuration) && (typeof this.configuration.iceServers !== 'undefined')) {
> > +      info("Ice Server configured");
> > +      var serverIp = this.configuration.iceServers[0].url.split(':')[1];
> 
> Hmm. This leaves a gotcha in place if we ever try to use an FQDN, but this
> is primarily intended for use in the lab, so maybe ok. I would add a note to
> the config-file warning about this.

Yes, it is for our lab use cases only. I added a note.
 
> @@ +2595,5 @@
> > +    if ((this.configuration) && (typeof this.configuration.iceServers !== 'undefined')) {
> > +      info("Ice Server configured");
> > +      var serverIp = this.configuration.iceServers[0].url.split(':')[1];
> > +      ok((lType === "relayed" || rType === "relayed") ||
> > +         (lIp === serverIp || rIp === serverIp), "One peer uses a relay");
> 
> I think that second '||' was intended to be an '&&'? Even so, I'm a little
> worried about the intent here. Even if we use a TURN server, we will only
> select pairs that use it if there is no direct connectivity. Are we assuming
> that a configured TURN server implies that we're doing a test that will
> prevent direct connectivity? Additionally, we still have the
> relayed-classified-as-peer-reflexive corner-case to worry about.

Just filed bug 1097333 for the relay vs. reflexive thing.
The '||' is intentionally, because it only serves as a backup for the case that the relayed candidate got wrongly categorized as peer reflexive. Only in that case I want to compare IP addresses.
 
> ::: dom/media/tests/mochitest/templates.js
> @@ +23,5 @@
> >    if (typeof test._remote_answer !== 'undefined') {
> >      dump("ERROR: SDP answer: " + test._remote_answer.sdp.replace(/[\r]/g, ''));
> >    }
> >  
> > +  if ((test.pcLocal) && (typeof test.pcLocal._local_ice_candidates !== 'undefined')) {
> 
> This typeof check seems strange; what about _remote_ice_candidates and
> _ice_candidates_to_add? Should those not be logged if there are no local?
> Are we willing to log these as "undefined"? I'd be ok with logging
> "undefined" for all of these, but that's just me.

The main idea behind the check is to verify that these queues have been generated by the trickle ICE test functions (they don't exist if no trickle ICE is used). And the existence of one of them as treated as indicator that all three got added/exist. I guess because of the JSON.stringify() calls around them it should be safe to call them even if they are not defined...
Comment on attachment 8521012 [details] [diff] [review]
bug_864118_add_turn_verification.patch

Review of attachment 8521012 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm
Attachment #8521012 - Flags: review?(docfaraday) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9e7f014425c1
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.