Closed Bug 1021220 Opened 6 years ago Closed 5 years ago

Add verification that loopback is not getting used in mochitests

Categories

(Core :: WebRTC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox33 --- fixed
firefox34 --- fixed

People

(Reporter: drno, Assigned: drno)

References

Details

Attachments

(1 file, 6 obsolete files)

The loopback interface is on purpose excluded from getting used by ICE. We should add to our tests, that it is in fact not used.
1) Check the SDP for the absence of loopback
2) Check via the RTCP stats for the absence of loopback
First cut of a simple loopback absence verification.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=6d00c8dd2f23
A more complete implementation of this ticket.

Ekr: all this assumes loopback not being used. I would either replicate the test_peerConnection_SDPverification test case and flip the expectations, or flip the pref from within that test case and generate another offer after changing the pref.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=601ad2bb7f2b
Attachment #8443000 - Attachment is obsolete: true
Try run: https://tbpl.mozilla.org/?tree=Try&rev=e86d0cb27c50
Attachment #8443138 - Attachment is obsolete: true
Attachment #8443858 - Flags: review?(ekr)
Try run: https://tbpl.mozilla.org/?tree=Try&rev=eb2af96f2c79
Attachment #8443858 - Attachment is obsolete: true
Attachment #8443858 - Flags: review?(ekr)
Comment on attachment 8456426 [details] [diff] [review]
bug_1021220_verify_absence_of_loopback.patch

Byron you know SDP well enough, maybe you can help me out here :-)
Attachment #8456426 - Flags: review?(docfaraday)
Didn't we just add support for using loopback (behind a pref) so that mochitests could be run on an airplane or similar? Won't this checking fail in that case?
Flags: needinfo?(drno)
Blocks: 1027350
The loopback patch has not landed yet. I want to land this first, and then pick it up in bug 1027350 and extend these tests here to check for both alternatives (rather then writing new tests for 1027350).
Flags: needinfo?(drno)
Comment on attachment 8456426 [details] [diff] [review]
bug_1021220_verify_absence_of_loopback.patch

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

I have to head out, but I want to get some of my questions/feedback to you today. I'll finish the review tomorrow.

::: dom/media/tests/mochitest/pc.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +const LOOPBACK_ADDR = "127.0.0";

I could see something really bizarre happening where this is a suffix on an IP address instead of a prefix (IP addresses can in fact end in a 0; only use of the network identifier is forbidden), suggest adding a '.' to the end just in case.

@@ +1892,5 @@
> +    var audioTracks = 0;
> +    if (constraints.mandatory) {
> +      if (constraints.mandatory.OfferToReceiveAudio) {
> +        audioTracks++;
> +      }

Seems like this code only returns 0 or 1. Why is this written as if there was an outer loop here?

@@ +1932,5 @@
> +    var videoTracks = 0;
> +    if (constraints.mandatory) {
> +      if (constraints.mandatory.OfferToReceiveVideo) {
> +        videoTracks++;
> +      }

Same question here.

@@ +2052,5 @@
> +    ok(desc.sdp.length > 10, "SessionDescription body length is plausible");
> +    ok(desc.sdp.contains("a=ice-ufrag"), "ICE username is present in SDP");
> +    ok(desc.sdp.contains("a=ice-pwd"), "ICE password is present in SDP");
> +    ok(desc.sdp.contains("a=fingerprint"), "ICE fingerprint is present in SDP");
> +    ok(!desc.sdp.contains(LOOPBACK_ADDR), "loopback interface is absent from SDP");

Couldn't hurt to reference bug 1027350 here, just in case.

@@ +2053,5 @@
> +    ok(desc.sdp.contains("a=ice-ufrag"), "ICE username is present in SDP");
> +    ok(desc.sdp.contains("a=ice-pwd"), "ICE password is present in SDP");
> +    ok(desc.sdp.contains("a=fingerprint"), "ICE fingerprint is present in SDP");
> +    ok(!desc.sdp.contains(LOOPBACK_ADDR), "loopback interface is absent from SDP");
> +    ok(desc.sdp.contains("a=candidate"), "at least one ICE candidate is present in SDP");

Might want to add a TODO here pointing at bug 1041832, since that means we need to be ok when there aren't any candidates.

@@ +2136,5 @@
> +   *        The RTCSessionDescription from the local side to compare.
> +   * @param {object} remoteSdp
> +   *        The RTCSessionDescription from the far end to compare.
> +   */
> +  compareStatsWithSdps : function PCW_compateStateWithSdps(stats, localSdp, remoteSdp) {

Typo

@@ +2150,5 @@
> +      const blob = stats[key];
> +      if (blob.hasOwnProperty("ipAddress")) {
> +        const ip = blob.ipAddress;
> +        const port = blob.portNumber;
> +        ok(dumpIfFailed(!ip.contains("127.0.0")), "RTCP IP address " + ip + " is not loopback");

Let's use LOOPBACK_ADDR here.

@@ +2153,5 @@
> +        const port = blob.portNumber;
> +        ok(dumpIfFailed(!ip.contains("127.0.0")), "RTCP IP address " + ip + " is not loopback");
> +        if (blob.type === "localcandidate") {
> +          ok(dumpIfFailed(localSdp.sdp.contains(ip)), "RTCP IP address " + ip + " matches local SDP");
> +          ok(dumpIfFailed(localSdp.sdp.contains(port)), "RTCP port number " + port + " matches local SDP");

Are we reasonably sure that this SDP will contain trickle candidates? If not, we might want to have a TODO here pointing at bug 1041832 to verify.

@@ +2156,5 @@
> +          ok(dumpIfFailed(localSdp.sdp.contains(ip)), "RTCP IP address " + ip + " matches local SDP");
> +          ok(dumpIfFailed(localSdp.sdp.contains(port)), "RTCP port number " + port + " matches local SDP");
> +        } else if (blob.type === "remotecandidate") {
> +          ok(dumpIfFailed(remoteSdp.sdp.contains(ip)), "RTCP IP address " + ip + " matches remote SDP");
> +          ok(dumpIfFailed(remoteSdp.sdp.contains(port)), "RTCP port number " + port + " matches remote SDP");

Similar question here.
A new patch which addresses Byron's concerns so far.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=348a397fd5d6
Attachment #8456426 - Attachment is obsolete: true
Attachment #8456426 - Flags: review?(docfaraday)
Attachment #8462924 - Flags: review?(docfaraday)
Comment on attachment 8462924 [details] [diff] [review]
bug_1021220_verify_absence_of_loopback.patch

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

Some more questions and relatively minor issues.

::: dom/media/tests/mochitest/pc.js
@@ +1903,5 @@
> +   *        The options to be examined.
> +   */
> +  audioInOfferOptions : function
> +    PCW_audioInOfferOptions(options) {
> +    if ((!options) || (options.length === 0)) {

|length|? Is this an array with some named properties mixed in?

@@ +1941,5 @@
> +   *        The options to be examined.
> +   */
> +  videoInOfferOptions : function
> +    PCW_videoInOfferOptions(options) {
> +    if ((!options) || (options.length === 0)) {

Same here.

@@ +2074,5 @@
> +    //TODO: how can we check for absence/presence of m=application?
> +
> +    //TODO: how to handle media contraints + offer options
> +    var audioTracks = this.countAudioTracksInMediaConstraint(constraints);
> +    if (constraints.length == 0) {

Maybe use === here since countAudioTracksInMediaContraint does too? I doubt it will become an issue, but perhaps just for completeness.

@@ +2083,5 @@
> +      ok(!desc.sdp.contains("m=audio"), "audio m-line is absent from SDP");
> +    } else {
> +      ok(desc.sdp.contains("m=audio"), "audio m-line is present in SDP");
> +      ok(desc.sdp.contains("a=rtpmap:109 opus/48000/2"), "OPUS codec is present in SDP");
> +      ok(desc.sdp.contains("a=rtcp-mux"), "RTCP Mux is offered in SDP");

Ideally what we want to check is that we have an rtcp-mux in this media section, but that's a bit tricky. Might be worth leaving a comment?

@@ +2089,5 @@
> +    }
> +
> +    //TODO: how to handle media contraints + offer options
> +    var videoTracks = this.countVideoTracksInMediaConstraint(constraints);
> +    if (constraints.length == 0) {

Maybe use === here?

@@ +2166,5 @@
> +      const blob = stats[key];
> +      if (blob.hasOwnProperty("ipAddress")) {
> +        const ip = blob.ipAddress;
> +        const port = blob.portNumber;
> +        ok(dumpIfFailed(!ip.contains(LOOPBACK_ADDR)), "RTCP IP address " + ip + " is not loopback");

Do we mean RTP, or maybe RTP/RTCP here? I worry that the first time we hit this error, someone will think it is something specific to RTCP.
Addressed Byron's concerns.
Attachment #8462924 - Attachment is obsolete: true
Attachment #8462924 - Flags: review?(docfaraday)
Attachment #8463770 - Flags: review?(docfaraday)
Comment on attachment 8463770 [details] [diff] [review]
bug_1021220_verify_absence_of_loopback.patch

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

lgtm
Attachment #8463770 - Flags: review?(docfaraday) → review+
Keywords: checkin-needed
Keywords: checkin-needed
Removed code/tests which caused oranges on slow builds. And added support for H264 video tests.

Carrying forward r+=bwc

Try run: https://tbpl.mozilla.org/?tree=Try&rev=d5eeda80930a
Attachment #8463770 - Attachment is obsolete: true
Try run appears green.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ae4d414634ba
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.