Closed Bug 855769 Opened 7 years ago Closed 7 years ago

Wire up top half of TURN support

Categories

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

x86
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla23

People

(Reporter: ekr, Assigned: ekr)

References

Details

(Whiteboard: [webrtc][blocking-webrtc-])

Attachments

(3 files, 7 obsolete files)

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
Assignee: nobody → jib
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_;
};
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.
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?
Whiteboard: [WebRTC] [blocking-webrtc-]
>Jesup: can you provide a patch to conditionalize TURN on a config preference?

Sure, no problem.
Priority: -- → P2
Whiteboard: [WebRTC] [blocking-webrtc-] → [webrtc][blocking-webrtc-]
Attachment #731489 - Attachment is obsolete: true
Attachment #731490 - Attachment is obsolete: true
Attachment #731498 - Attachment is obsolete: true
Attachment #731500 - Attachment is obsolete: true
Attachment #731364 - Attachment is obsolete: true
Attachment #731729 - Flags: review?(rjesup)
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());
Attachment #731729 - Flags: review?(rjesup) → review+
Attached patch Wire up top half of TURN support (obsolete) — Splinter Review
Attachment #731729 - Attachment is obsolete: true
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?
Flags: needinfo?(jib)
Attachment #731861 - Attachment is obsolete: true
Assignee: jib → 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.
Attachment #735306 - Flags: review-
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.
Attachment #733602 - Flags: review+
Attachment #731600 - Flags: review+
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.
Attachment #733602 - Flags: checkin?(adam)
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
Flags: needinfo?(jib)
Whiteboard: [webrtc][blocking-webrtc-] → [webrtc][blocking-webrtc-][webrtc-uplift]
Target Milestone: --- → mozilla22
Attachment #733602 - Flags: checkin?(adam) → checkin+
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 :-/)
Target Milestone: mozilla22 → mozilla23
Keywords: verifyme
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
Attachment #733602 - Flags: approval-mozilla-aurora?
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.
Attachment #731600 - Flags: approval-mozilla-aurora?
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
Status: RESOLVED → VERIFIED
Keywords: verifyme
The TURN server provided in the mochitest isn't doing anything useful. You're effectively testing w/o TURN.
(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.
Status: VERIFIED → RESOLVED
Closed: 7 years ago7 years ago
Keywords: verifyme
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
Attachment #731600 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #733602 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Whiteboard: [webrtc][blocking-webrtc-][webrtc-uplift] → [webrtc][blocking-webrtc-]
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.