1342579 - Test to verify we can connect to different certs and ICE credentials

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: Bug 1342579: verify that a single PC can connect to different endpoints.

Review commit: https://reviewboard.mozilla.org/r/115456/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/115456/

Comment 2

[Nils Ohlmeier [:drno]]

Comment on attachment 8841120 [details]
Bug 1342579: verify that a single PC can connect to different endpoints.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115456/diff/1-2/

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8841120 [details]
Bug 1342579: verify that a single PC can connect to different endpoints.

https://reviewboard.mozilla.org/r/115456/#review117578

Just a few suggestions for making this a little cleaner.

::: dom/media/tests/mochitest/test_peerConnection_threeUnbundledConnections.html:40
(Diff revision 2)
> +    if (state == "connected") {
> +      ok(true, str + " ICE connected");
> +      resolve();
> +    } else if (state == "failed") {
> +      ok(false, str + " ICE failed")
> +      resolve();

The resolve call can be placed outside the if statement.

::: dom/media/tests/mochitest/test_peerConnection_threeUnbundledConnections.html:71
(Diff revision 2)
> +      offer = pc1.localDescription;
> +      info("Original OFFER: " + JSON.stringify(offer));
> +      offer.sdp = sdputils.removeBundle(offer.sdp);
> +      //info("OFFER w/o BUNDLE: " + JSON.stringify(offer));
> +      offerAudio = new RTCSessionDescription(JSON.parse(JSON.stringify(offer)));
> +      offerAudio.sdp = offerAudio.sdp.replace(/m=video 9/, 'm=video 0');

`.replace('m=video 9', 'm=video 0')` works fine.

::: dom/media/tests/mochitest/test_peerConnection_threeUnbundledConnections.html:77
(Diff revision 2)
> +      //info("offerAudio: " + JSON.stringify(offerAudio));
> +      offerVideo = new RTCSessionDescription(JSON.parse(JSON.stringify(offer)));
> +      offerVideo.sdp = offerVideo.sdp.replace(/m=audio 9/, 'm=audio 0');
> +      //info("offerVideo: " + JSON.stringify(offerVideo));
> +    })
> +    .then(() => pc2.setRemoteDescription(offerVideo))

You can factor this whole block out.

```
var videoAnswer = {};
async function generatePartialAnswer(pc, offer, answer) {
  await pc.setRemoteDescription(offer);
  var a = await pc.createAnswer();
  await pc.setLocalDescription(a);
  sdp = pc.localDescription.sdp.split('\r\n');
  answer.sessionSection = sdp.slice(...);
  answer.fingerprint = sdp[ifp];
  
  ... see below
```

::: dom/media/tests/mochitest/test_peerConnection_threeUnbundledConnections.html:81
(Diff revision 2)
> +    })
> +    .then(() => pc2.setRemoteDescription(offerVideo))
> +    .then(() => pc2.createAnswer({}))  // check that createAnswer accepts arg.
> +    .then(answer2 => pc2.setLocalDescription(answer2))
> +    .then(() => {
> +      answer2 = pc2.localDescription;



::: dom/media/tests/mochitest/test_peerConnection_threeUnbundledConnections.html:83
(Diff revision 2)
> +    .then(() => pc2.createAnswer({}))  // check that createAnswer accepts arg.
> +    .then(answer2 => pc2.setLocalDescription(answer2))
> +    .then(() => {
> +      answer2 = pc2.localDescription;
> +      //info("ANSWER2: " + JSON.stringify(answer2));
> +      videoFP = answer2.sdp.match(/^a=fingerprint:(.)*$/gm)[0];

If you put the * inside the parentheses, this works better.

::: dom/media/tests/mochitest/test_peerConnection_threeUnbundledConnections.html:85
(Diff revision 2)
> +    .then(() => {
> +      answer2 = pc2.localDescription;
> +      //info("ANSWER2: " + JSON.stringify(answer2));
> +      videoFP = answer2.sdp.match(/^a=fingerprint:(.)*$/gm)[0];
> +      //info("Answer2 fingerprint: " + videoFP);
> +      sessionSection = answer2.sdp.match(/^[\s\S]*?m=audio/g)[0].replace('m=audio', '').replace(/a=fingerprint.*\r\n/g, '');

I wonder if this might not be easier if you split the answers into lists of lines and used array functions on them.  For instance:

```
a2lines = answer2.sdp.split('\r\n');
const m = a2lines.findIndex(l => l.match(/^m=audio /));
const afp = a2lines.findIndex(l => l.match(/^a=fingerprint/));
audioFP = a2lines[afp];
sessionSection = a2lines.slice(0, m).splice(afp, 1);
audioMsection = a2lines.slice(m);
```

Then you can do this at the end:

```
fakeAnswer = sessionSection.concat(audioMsection, [fp], videoMsection, [fp]).join('\r\n');
```

Comment 4

[Nils Ohlmeier [:drno]]

Comment on attachment 8841120 [details]
Bug 1342579: verify that a single PC can connect to different endpoints.

https://reviewboard.mozilla.org/r/115456/#review117578

> The resolve call can be placed outside the if statement.

No it can't because I don't want to resolve if e.g. "checking" is reached.

Comment 5

[Martin Thomson [:mt:]]

Comment on attachment 8841120 [details]
Bug 1342579: verify that a single PC can connect to different endpoints.

https://reviewboard.mozilla.org/r/115456/#review117578

> No it can't because I don't want to resolve if e.g. "checking" is reached.

*facepalm*.  Of course.

Comment 6

[Nils Ohlmeier [:drno]]

Comment on attachment 8841120 [details]
Bug 1342579: verify that a single PC can connect to different endpoints.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115456/diff/2-3/

Comment 7

[Nils Ohlmeier [:drno]]

Comment on attachment 8841120 [details]
Bug 1342579: verify that a single PC can connect to different endpoints.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115456/diff/3-4/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/830770d5c2b8
verify that a single PC can connect to different endpoints. r=mt

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/830770d5c2b8

Comment 10

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/mozilla-central/rev/830770d5c2b8#l2.12
>+<script type="application/javascript;version=1.8">

Please do not add a new versioned JavaScript. Currently ESLint ignores versioned scripts. See bug 1342144 for details.

Comment 11

[Nils Ohlmeier [:drno]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cedd1aab52c76a8f39bc9bf57a3798c0129353c
Bug 1342579: remove js version from threeUnbundledConnections test (really bug 1342144). r=mt

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2cedd1aab52c