1010007 - WebRTC mochitests need to read TURN server configuration

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Nils Ohlmeier [:drno]]

To be able to use TURN servers installed as a permanent network service the WebRTC mochitests need a solution to learn about the TURN server configuration of the local network.

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: Read TURN json config and feed it into RTCPeerConnection()

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

Comment 2

[Nils Ohlmeier [:drno]]

Attachment: read_turn_config.patch

Read TURN server config from turnConfig.js

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

Comment 3

[Nils Ohlmeier [:drno]]

Attachment: read_turn_config.patch

This reads TURN server configuration from turnConfig.js and only adds it to the PC configurations if the test has not disabled TURN or provided its own TURN server config to the PC already.

Now properly tested locally :-)

Comment 4

[Nils Ohlmeier [:drno]]

Try run for attachment 8422845 [details] [diff] [review]: https://tbpl.mozilla.org/?tree=Try&rev=b82bc62f352f

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8422845 [details] [diff] [review]
read_turn_config.patch

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

I have a concern about naming consistency in the config object for unit/pc.js and mochitest/pc.js. Some minor readability/code redundancy nits.

::: dom/media/tests/identity/test_peerConnection_peerIdentity.html
@@ +27,3 @@
>        peerIdentity: id2
>      },
> +    config_remote: {

This is a good modification, but dom/media/tests/unit/pc.js is still using the old names. There are also a couple of comments in pc.js that use the old names.

::: dom/media/tests/mochitest/pc.js
@@ +431,5 @@
>    options.is_local = "is_local" in options ? options.is_local : true;
>    options.is_remote = "is_remote" in options ? options.is_remote : true;
>  
> +  if ((!options.turn_disabled_local) &&
> +    (typeof turnServers !== 'undefined') && (turnServers.local)) {

This chunk of code would probably be more readable if we checked typeof turnServers !== 'undefined' once, and then the handling for local and remote inside.

@@ +436,5 @@
> +    if (!options.hasOwnProperty("config_local")) {
> +      options.config_local = turnServers.local;
> +    }
> +    else if (!options.config_local.hasOwnProperty("iceServers")) {
> +      options.config_local.iceServers = turnServers.local.iceServers;

These two code paths behave differently if turnServers.local has more than an iceServers property (in the former, all properties are copied, whereas only iceServers is copied in the latter). This might cause maintainability headaches later. Recommend making options.config_local = {} if it doesn't exist, and falling through to common logic (might just copy iceServers, might copy all props, I could argue either way, so won't be picky). Similar concern below.

::: dom/media/tests/mochitest/templates.js
@@ +152,5 @@
>          ok(true, "pc_local: ICE switched to 'connected' state");
>          myTest.next();
>        };
>        function onIceConnectedFailed () {
> +        if (typeof myTest._local_offer !== 'undefined') {

Maybe we should dump something if undefined? Also, these blocks of code are getting large enough that we may want to break them out into their own function.

::: dom/media/tests/mochitest/turnConfig.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* An example of how to specify two TURN server configs:
> + *   local: { iceServers: [{"username":"mozilla","credential":"mozilla","url":"turn:10.0.0.1"}] },
> + *   remote: { iceServers: [{"username":"firefox","credential":"firefox","url":"turn:10.0.0.2"}] }

I'd include a "var turnServers = {...}" in here, so that uncommenting would yield a syntactically valid config.

Comment 6

[Nils Ohlmeier [:drno]]

Attachment: read_turn_config.patch

Addressed Byron's concerns with the previous patch.

Comment 7

[Nils Ohlmeier [:drno]]

(In reply to Byron Campen [:bwc] from comment #5)
> ::: dom/media/tests/identity/test_peerConnection_peerIdentity.html
> @@ +27,3 @@
> >        peerIdentity: id2
> >      },
> > +    config_remote: {
> 
> This is a good modification, but dom/media/tests/unit/pc.js is still using
> the old names. There are also a couple of comments in pc.js that use the old
> names.

The file dom/media/tests/unit/pc.js does not exist on m-c.
 
> ::: dom/media/tests/mochitest/templates.js
> @@ +152,5 @@
> >          ok(true, "pc_local: ICE switched to 'connected' state");
> >          myTest.next();
> >        };
> >        function onIceConnectedFailed () {
> > +        if (typeof myTest._local_offer !== 'undefined') {
> 
> Maybe we should dump something if undefined?

This check is needed for steeplechase which runs only the "local" part of a test on one machine, and only the "remote" parts on the other machine.
I don't expect this to be an issue on regular one machine tests. And even then you would still get the logging from the failed test case with no SDP dumps at all.

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8423543 [details] [diff] [review]
read_turn_config.patch

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

Looks good.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/49733bcc9159

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/49733bcc9159