Last Comment Bug 786236 - Add per-PeerConnection STUN configuration
: Add per-PeerConnection STUN configuration
Status: RESOLVED FIXED
[WebRTC], [blocking-webrtc+], [nICEr]...
:
Product: Core
Classification: Components
Component: WebRTC: Networking (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: mozilla21
Assigned To: Eric Rescorla (:ekr)
: Jason Smith [:jsmith]
:
Mentors:
Depends on: 829691
Blocks: 825703
  Show dependency treegraph
 
Reported: 2012-08-28 06:21 PDT by Eric Rescorla (:ekr)
Modified: 2013-01-25 22:02 PST (History)
9 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Per-ctx configurable STUN servers (15.53 KB, patch)
2012-12-31 07:42 PST, Eric Rescorla (:ekr)
no flags Details | Diff | Splinter Review
Per-ctx configurable STUN servers (15.54 KB, patch)
2012-12-31 07:55 PST, Eric Rescorla (:ekr)
adam: review+
Details | Diff | Splinter Review
Per-ctx configurable STUN servers (18.19 KB, patch)
2013-01-05 13:33 PST, Eric Rescorla (:ekr)
adam: review+
Details | Diff | Splinter Review
Per-ctx configurable STUN servers. r=adam (18.18 KB, patch)
2013-01-16 08:52 PST, Jan-Ivar Bruaroey [:jib]
jib: review+
Details | Diff | Splinter Review

Description Eric Rescorla (:ekr) 2012-08-28 06:21:08 PDT
Currently, nICEr expects to have a single system global STUN/TURN configuration. We need to make that per-ctx and expose that to the API.
Comment 1 Randell Jesup [:jesup] 2012-12-06 00:52:46 PST
Exposing it to the JS level might end up being a separate bug, or will need consultation from myself and/or ehugg and/or ekr.
Comment 2 Eric Rescorla (:ekr) 2012-12-06 05:35:45 PST
This might be a good one for Jan-Ivar.
Comment 3 Eric Rescorla (:ekr) 2012-12-06 06:06:31 PST
This needs the following work, from bottom to top:

1. Add a call nr_ice_set_stun_servers() [and for bonus points nr_ice_set_turn_servers)
    to media/mtransport/third_party/nICEr/src/ice/ice_ctx.c. Model it on nr_ice_fetch_{stun,turn}_servers().
    If you really ewant to get fancy, have it extend the list.

2. Suppress the global STUN configuration setting in:
    NrIceCtx::Create() [look for ice.stun.server.0.*].

   I would comment this out. If we provide a moz stun server, we may want to revisit it.

3. Add NrIceCtx::SetStunServers() and wire up below in media/mtransport/nricectx.cpp
4. Add code to the PC to read the configuration variables and parse out the STUN/TURN URLs
5. Wire the PC code up to the JS

3The change you need to n
Comment 4 Eric Rescorla (:ekr) 2012-12-24 07:36:11 PST
jib: are you planning to do this soonish or do you have more important things? I think it may be in the way of my work on ICE in a bit...
Comment 5 Jan-Ivar Bruaroey [:jib] 2012-12-27 21:50:55 PST
I'll work on this one today.
Comment 6 Eric Rescorla (:ekr) 2012-12-29 16:22:37 PST
jib: have you worked on this at all? If not, I'm going to do it...
Comment 7 Maire Reavy [:mreavy] 2012-12-29 18:25:17 PST
ekr -- can you do this?  jib is working on a blocker gUM bug after he lands his patches that are currently in flight.
Comment 8 Eric Rescorla (:ekr) 2012-12-29 19:03:25 PST
No problem. I just didn't want to start on it if he was already mostly done.
Comment 9 Jan-Ivar Bruaroey [:jib] 2012-12-29 19:26:39 PST
Thanks, I haven't worked on it.
Comment 10 Eric Rescorla (:ekr) 2012-12-31 07:42:13 PST
Created attachment 696725 [details] [diff] [review]
Per-ctx configurable STUN servers

* * *
Configure STUN servers from JS
Comment 11 Eric Rescorla (:ekr) 2012-12-31 07:55:54 PST
Created attachment 696730 [details] [diff] [review]
Per-ctx configurable STUN servers

* * *
Configure STUN servers from JS
Comment 12 Adam Roach [:abr] 2012-12-31 18:13:43 PST
Comment on attachment 696730 [details] [diff] [review]
Per-ctx configurable STUN servers

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

Looks fundamentally good. I've added some strong recommendations for additional safeguards, and a couple of nits, but nothing that changes the bones of the patch, so I'm r+'ing it.

::: media/mtransport/nricectx.cpp
@@ -275,5 @@
>        NR_reg_set_uchar((char *)"ice.pref.interface.wlan0", 232);
>      }
>  
> -    NR_reg_set_string((char *)"ice.stun.server.0.addr", (char *)"216.93.246.14");
> -    NR_reg_set_uint2((char *)"ice.stun.server.0.port",3478);

With the removal of a default server, the code in nr_ice_fetch_stun_servers will have a ct of 0, which means RCALLOC(sizeof(nr_ice_stun_server)*ct)) might return NULL (see http://c-faq.com/ansi/malloc0.html). The current code will abort context initialization if that happens.

Basically, I think we need to add a check at the top of nr_ice_fetch_stun_servers that checks ct against 0, and returns success if such is the case. This might not be an issue for the Mozilla tree and its PR implementation, but it will certainly help with portability when we push this patch upstream.

Alternately, you could simply avoid calling nr_ice_fetch_stun_servers from nr_ice_ctx_create if stun_server_ct == 0.

::: media/mtransport/nricectx.h
@@ +78,5 @@
>  
> +
> +struct NrIceStunServer {
> + public:
> +  static NrIceStunServer* Create(const std::string& addr, uint16_t port) {

If we're expecting the layer above us to do name -> address resolution, isn't passing the IP address as a string going to be a bit cumbersome?

In normal operation, we'll have a DNS name coming in from the javascript, which is then resolved into an IP address, serialized as a human-readable string, and then re-parsed into an IP address. Wouldn't it make more sense to have the upper layer pass this down as a PRNetAddr in the first place? At the very least, I would propose having a Create() that takes PRNetAddr as its parameters rather than the string.

::: media/mtransport/test/ice_unittest.cpp
@@ +79,5 @@
> +    ScopedDeletePtr<NrIceStunServer> server(NrIceStunServer::Create(
> +        std::string((char *)"216.93.246.14"), 3478));
> +    stun_servers.push_back(*server);
> +    ASSERT_TRUE(NS_SUCCEEDED(ice_ctx_->SetStunServers(stun_servers)));
> +    

Whitespace

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c
@@ +113,5 @@
>    }
>  
> +int nr_ice_ctx_set_stun_servers(nr_ice_ctx *ctx,nr_ice_stun_server *servers,int ct)
> +  {
> +    int r,_status;

Variable 'r' is unused.

@@ +135,5 @@
> +  }
> +
> +int nr_ice_ctx_set_turn_servers(nr_ice_ctx *ctx,nr_ice_turn_server *servers,int ct)
> +  {
> +    int r,_status;

Variable 'r' is unused.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +83,5 @@
> +  // settings.
> +  std::vector<NrIceStunServer> stun_servers;
> +  ScopedDeletePtr<NrIceStunServer> server(NrIceStunServer::Create(
> +      std::string((char *)"216.93.246.14"), 3478));
> +  stun_servers.push_back(*server);

I know we're hardcoding the input here, so the chance of a parsing error when creating the stun server object is approximately zero -- but I'm a bit antsy about the prospect of calling into code that has a contract allowing it to return null and then passing that returned pointer into a function that doesn't check for null. I'm worried that leaving this pattern here (without a null check) may lead a future implementor to copy the pattern (say, when implementing the final solution).

tl;dr: can we check for null before calling SetStunServers (or add code to SetStunServers that checks for null when traversing the vector)?
Comment 13 Eric Rescorla (:ekr) 2012-12-31 18:18:44 PST
> Looks fundamentally good. I've added some strong recommendations for
> additional safeguards, and a couple of nits, but nothing that changes the
> bones of the patch, so I'm r+'ing it.
> 
> ::: media/mtransport/nricectx.cpp
> @@ -275,5 @@
> >        NR_reg_set_uchar((char *)"ice.pref.interface.wlan0", 232);
> >      }
> >  
> > -    NR_reg_set_string((char *)"ice.stun.server.0.addr", (char *)"216.93.246.14");
> > -    NR_reg_set_uint2((char *)"ice.stun.server.0.port",3478);
> 
> With the removal of a default server, the code in nr_ice_fetch_stun_servers
> will have a ct of 0, which means RCALLOC(sizeof(nr_ice_stun_server)*ct))
> might return NULL (see http://c-faq.com/ansi/malloc0.html). The current code
> will abort context initialization if that happens.
> 
> Basically, I think we need to add a check at the top of
> nr_ice_fetch_stun_servers that checks ct against 0, and returns success if
> such is the case. This might not be an issue for the Mozilla tree and its PR
> implementation, but it will certainly help with portability when we push
> this patch upstream.
> 
> Alternately, you could simply avoid calling nr_ice_fetch_stun_servers from
> nr_ice_ctx_create if stun_server_ct == 0.

This seems like good advice.



> ::: media/mtransport/nricectx.h
> @@ +78,5 @@
> >  
> > +
> > +struct NrIceStunServer {
> > + public:
> > +  static NrIceStunServer* Create(const std::string& addr, uint16_t port) {
> 
> If we're expecting the layer above us to do name -> address resolution,
> isn't passing the IP address as a string going to be a bit cumbersome?
> 
> In normal operation, we'll have a DNS name coming in from the javascript,
> which is then resolved into an IP address, serialized as a human-readable
> string, and then re-parsed into an IP address. Wouldn't it make more sense
> to have the upper layer pass this down as a PRNetAddr in the first place? At
> the very least, I would propose having a Create() that takes PRNetAddr as
> its parameters rather than the string.

I think this is a good argument. I will do so.



> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
> @@ +83,5 @@
> > +  // settings.
> > +  std::vector<NrIceStunServer> stun_servers;
> > +  ScopedDeletePtr<NrIceStunServer> server(NrIceStunServer::Create(
> > +      std::string((char *)"216.93.246.14"), 3478));
> > +  stun_servers.push_back(*server);
> 
> I know we're hardcoding the input here, so the chance of a parsing error
> when creating the stun server object is approximately zero -- but I'm a bit
> antsy about the prospect of calling into code that has a contract allowing
> it to return null and then passing that returned pointer into a function
> that doesn't check for null. I'm worried that leaving this pattern here
> (without a null check) may lead a future implementor to copy the pattern
> (say, when implementing the final solution).
> 
> tl;dr: can we check for null before calling SetStunServers (or add code to
> SetStunServers that checks for null when traversing the vector)?

Willdo.
Comment 14 Eric Rescorla (:ekr) 2013-01-05 13:33:18 PST
Created attachment 698334 [details] [diff] [review]
Per-ctx configurable STUN servers

* * *
Configure STUN servers from JS
Comment 15 Eric Rescorla (:ekr) 2013-01-05 13:37:18 PST
Comment on attachment 698334 [details] [diff] [review]
Per-ctx configurable STUN servers

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

I revised this. Can you re-review?
Comment 16 Adam Roach [:abr] 2013-01-07 07:02:54 PST
Comment on attachment 698334 [details] [diff] [review]
Per-ctx configurable STUN servers

lgtm
Comment 17 Jan-Ivar Bruaroey [:jib] 2013-01-16 08:52:16 PST
Created attachment 702857 [details] [diff] [review]
Per-ctx configurable STUN servers. r=adam

Patch got stale because of Bug 807494 landing. Refreshed.
Comment 19 Ryan VanderMeulen [:RyanVM] 2013-01-24 09:46:40 PST
https://hg.mozilla.org/mozilla-central/rev/0eda7bbdfedf

Note You need to log in before you can comment on or make changes to this bug.