855769 - Wire up top half of TURN support

Status: VERIFIED FIXED

(Core :: WebRTC: Networking, defect, P2)

Description

[Eric Rescorla (:ekr)]

The TURN configuration needs to be wired up from PCImpl down to NrIceCtx.

NrIceCtx currently doesn't have a SetTurnServers fxn, but expect something like the following:

  nsresult SetTurnServers(const std::vector<NrIceTurnServer>& turn_servers);

struct NrIceTurnServer : public NrIceStun Server {
  static NrIceStunServer* Create(const std::string& addr, uint16_t port,
                                 const std::string& username, const std::string& password) {
  std::string username;
  std::string password;
};


These functions should sit behind a pref so we can turn off TURN if it's unstable

Comment 1

[Eric Rescorla (:ekr)]

Actual API
  nsresult SetTurnServers(const std::vector<NrIceTurnServer>& turn_servers);


class NrIceTurnServer : public NrIceStunServer {
 public:
  static NrIceTurnServer *Create(const std::string& addr, uint16_t port,
                                 const std::string& username,
                                 const std::vector<unsigned char>& password) {
    ScopedDeletePtr<NrIceTurnServer> server(
        new NrIceTurnServer(username, password));
    
    nsresult rv = server->Init(addr, port);
    if (NS_FAILED(rv))
      return nullptr;
    
    return server.forget();
  }

  nsresult ToNicerTurnStruct(nr_ice_turn_server *server) const;  

 private:
  NrIceTurnServer(const std::string& username,
                  const std::vector<unsigned char>& password) :
      username_(username), password_(password) {}

  std::string username_;
  std::vector<unsigned char> password_;
};

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Wired up TURN support in RTCPeerConnection config.

Compiles and seems to not crash. I may have gotten a little overzealous and hooked up more than was asked for.

Warning: NrIceTurnServer::ToNicerStruct() is incomplete and probably crashes if you provide password since I cast a string to a Data *. To be fixed. Handing off to Randell.

Comment 3

[Eric Rescorla (:ekr)]

I suggest that I take it from here, with the following exception.

Jesup: can you provide a patch to conditionalize TURN on a config preference?

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

>Jesup: can you provide a patch to conditionalize TURN on a config preference?

Sure, no problem.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Attachment: add pref to disable TURN if needed

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: add pref to disable TURN if needed

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Attachment: add pref to disable TURN if needed

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Attachment: add pref to disable TURN if needed

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Attachment: add pref to disable TURN if needed

Comment 10

[Eric Rescorla (:ekr)]

Attachment: Wire up TURN support in RTCPeerConnection config.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 731729 [details] [diff] [review]
Wire up TURN support in RTCPeerConnection config.

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

::: dom/media/tests/mochitest/test_peerConnection_bug825703.html
@@ +41,5 @@
>      try { pc = new mozRTCPeerConnection({ iceServers: [{ url:"http:0.0.0.0" }] }); } catch (e) { exception = e; }
>      ok(exception, "mozRTCPeerConnection({ iceServers: [{ url:\"http:0.0.0.0\" }] }) throws");
>      exception = null;
> +    try { pcs = new mozRTCPeerConnection({ iceServers: [{ url:"stun:0.0.0.0" }, { url:"stuns:x.net", foo:"" }, { url: "turn:[::192.9.5.5]:42", username:"p", credential:"p" }, { url:"turns:x.org:42", username:"p", credential:"p" }] }); } catch (e) { exception = e; }
> +    ok(!exception, "mozRTCPeerConnection({ iceServers: [{ url:\"stun:0.0.0.0\" }, { url:\"stuns:x.net\", foo:\"\" }, { url: \"turn:[::192.9.5.5]:42\", username:\"p\", credential:\"p\"}, { url:\"turn:x.org:42\", username:\"p\", credential:\"p\" }] }) succeeds");

Can we break up these lines and line them up so they're a bit readable?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +4,5 @@
>  
>  #include <string>
>  
>  #include "vcm.h"
> +include "CSFLog.h"

Typo

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +85,5 @@
> +    // username and password. Bug # ???
> +    std::vector<unsigned char> password;
> +
> +    for (size_t i=0; i < pwd.size(); ++i) {
> +      password.push_back(pwd[i]);

Wouldn't this be better?
std::vector<unsigned char> password(pwd.begin(), pwd.end());

Comment 12

[Eric Rescorla (:ekr)]

Attachment: Wire up top half of TURN support

Comment 13

[Eric Rescorla (:ekr)]

Attachment: Wire up TURN support in RTCPeerConnection config.

Comment 14

[Maire Reavy [:mreavy]]

I saw Bug 786235 is marked for checkin.  Which patches from this bug need to be checked in? When should this bug land relative to bug 786235?

Comment 15

[Eric Rescorla (:ekr)]

Attachment: Trivial mochitest for TURN

Comment 16

[Eric Rescorla (:ekr)]

Comment on attachment 735306 [details] [diff] [review]
Trivial mochitest for TURN

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

These tests are WIP:

1. They depend on a specific local TURN server.
2. I think the pref flipping in the PreffedOff test persists.

They are submitted here as a placeholder so that we have them when we are
ready for TURN testing.

Comment 17

[Eric Rescorla (:ekr)]

Comment on attachment 733602 [details] [diff] [review]
Wire up TURN support in RTCPeerConnection config.

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

Carrying forward r+ from jesup.

Comment 18

[Eric Rescorla (:ekr)]

Comment on attachment 733602 [details] [diff] [review]
Wire up TURN support in RTCPeerConnection config.

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

Carrying forward r+ from jesup.


Adam, can you commit for me.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/915ad1e8a9bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1e464427681

I believe the plan is to look to uplift this preffed-off to 22

Comment 20

[Ed Morley [:emorley]]

Target milestone is normally set to the milestone that existed on trunk when a bug lands - the status flags track uplifts. (Field name doesn't exactly map well :-/)

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/915ad1e8a9bb
https://hg.mozilla.org/mozilla-central/rev/e1e464427681

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 733602 [details] [diff] [review]
Wire up TURN support in RTCPeerConnection config.

This will need to be taken with the back-end patches for TURN support from bug 786235 and the pref patch in this bug.

NOTE: We're planning to land TURN with a pref that turns it off by default.  Since the changes cover a number of other parts we may need to modify, landing this in Aurora will greatly minimize the risk and effort to uplift any bugfixes from 23, now or when 22 gets to beta.  It also means that people using 22 can try it out (and if it survives largely bug-free, we might end up recommending it to people).

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a

User impact if declined: Turn will not be accessible via a pref in FF22.  Extra effort and risk doing other uplifts in Aurora and Beta 22.

Testing completed (on m-c, etc.): on m-c; being fuzzed by cdiehl since before it landed.  So far pretty clean.  We're working with releng on getting them to add a TURN server to our test setup; we've stood up our own TURN server on EC2 and are setting up some continuous integration builds since the mochitests/etc can't test it.

Risk to taking this patch (and alternatives if risky): Low since the majority of the changes (those not shared by ICE) will be disabled by by default.  ICE changes are generally being covered by existing tests and our continuous integration, though coverage will be better once we get STUN & TURN support in test.

String or IDL/UUID changes made by this patch: none

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 731600 [details] [diff] [review]
add pref to disable TURN if needed

[Approval Request Comment]

See other patch in this bug.  We'd land on Aurora with turn disabled instead of enabled.

Comment 24

[Jason Smith [:jsmith]]

Sanity tested this with some basic peer connection flows with the TURN server provided in the mochitest. I'm definitely seeing the basis of the feature landed and there, working in some cases and being buggy in others. Marking verified to indicate basic testing was done here, although I'll plan on doing more testing and automation work to get deeper coverage.

Followups that are already known:

- bug 864117
- bug 864114
- bug 864109

Comment 25

[Eric Rescorla (:ekr)]

The TURN server provided in the mochitest isn't doing anything useful. You're effectively testing w/o TURN.

Comment 26

[Jason Smith [:jsmith]]

(In reply to Eric Rescorla (:ekr) from comment #25)
> The TURN server provided in the mochitest isn't doing anything useful.
> You're effectively testing w/o TURN.

Ah okay. Let's talk more on Monday then about TURN testing and I'll revisit the sanity testing here later.

Comment 27

[Alex Keybl [:akeybl]]

Comment on attachment 731600 [details] [diff] [review]
add pref to disable TURN if needed

See https://bugzilla.mozilla.org/show_bug.cgi?id=786235#c29

Comment 28

[Jason Smith [:jsmith]]

So this time I did a bunch of sanity tests with bug 860775's TURN server specified and saw it working at a sanity level.