1021220 - Add verification that loopback is not getting used in mochitests

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Nils Ohlmeier [:drno]]

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

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: bug_1021220_verify_absence_of_loopback.patch

First cut of a simple loopback absence verification.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=6d00c8dd2f23

Comment 2

[Nils Ohlmeier [:drno]]

Attachment: bug_1021220_verify_absence_of_loopback.patch

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

Comment 3

[Nils Ohlmeier [:drno]]

Attachment: bug_1021220_verify_absence_of_loopback.patch

Try run: https://tbpl.mozilla.org/?tree=Try&rev=e86d0cb27c50

Comment 4

[Nils Ohlmeier [:drno]]

Attachment: bug_1021220_verify_absence_of_loopback.patch

Try run: https://tbpl.mozilla.org/?tree=Try&rev=eb2af96f2c79

Comment 5

[Nils Ohlmeier [:drno]]

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 :-)

Comment 6

[Byron Campen [:bwc]]

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?

Comment 7

[Nils Ohlmeier [:drno]]

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).

Comment 8

[Byron Campen [:bwc]]

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.

Comment 9

[Nils Ohlmeier [:drno]]

Attachment: bug_1021220_verify_absence_of_loopback.patch

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

Try run: https://tbpl.mozilla.org/?tree=Try&rev=348a397fd5d6

Comment 10

[Byron Campen [:bwc]]

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.

Comment 11

[Nils Ohlmeier [:drno]]

Attachment: bug_1021220_verify_absence_of_loopback.patch

Addressed Byron's concerns.

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 8463770 [details] [diff] [review]
bug_1021220_verify_absence_of_loopback.patch

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

lgtm

Comment 13

[Nils Ohlmeier [:drno]]

Attachment: bug_1021220_verify_absence_of_loopback.patch

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

Comment 14

[Nils Ohlmeier [:drno]]

Try run appears green.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae4d414634ba

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ae4d414634ba

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fdf2f580b665