Closed Bug 786236 Opened 7 years ago Closed 7 years ago

Add per-PeerConnection STUN configuration

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: ekr, Assigned: ekr)

References

Details

(Whiteboard: [WebRTC], [blocking-webrtc+], [nICEr], [nICEr-upstream-needed] [qa-])

Attachments

(1 file, 3 obsolete files)

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.
Whiteboard: [WebRTC]
Whiteboard: [WebRTC] → [WebRTC], [blocking-webrtc+]
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.
Assignee: nobody → jib
Priority: -- → P2
This might be a good one for Jan-Ivar.
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
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...
I'll work on this one today.
jib: have you worked on this at all? If not, I'm going to do it...
ekr -- can you do this?  jib is working on a blocker gUM bug after he lands his patches that are currently in flight.
No problem. I just didn't want to start on it if he was already mostly done.
Thanks, I haven't worked on it.
Assignee: jib → ekr
* * *
Configure STUN servers from JS
* * *
Configure STUN servers from JS
Attachment #696725 - Attachment is obsolete: true
Attachment #696730 - Flags: review?(adam)
Blocks: 825703
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+], [nICEr]
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)?
Attachment #696730 - Flags: review?(adam) → 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.

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.
* * *
Configure STUN servers from JS
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?
Attachment #698334 - Flags: review?(adam)
Comment on attachment 698334 [details] [diff] [review]
Per-ctx configurable STUN servers

lgtm
Attachment #698334 - Flags: review?(adam) → review+
Attachment #696730 - Attachment is obsolete: true
Depends on: 829691
Depends on: 829856
No longer depends on: 829856
Patch got stale because of Bug 807494 landing. Refreshed.
Attachment #698334 - Attachment is obsolete: true
Attachment #702857 - Flags: review+
Whiteboard: [WebRTC], [blocking-webrtc+], [nICEr] → [WebRTC], [blocking-webrtc+], [nICEr], [nICEr-upstream-needed]
https://hg.mozilla.org/mozilla-central/rev/0eda7bbdfedf
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [WebRTC], [blocking-webrtc+], [nICEr], [nICEr-upstream-needed] → [WebRTC], [blocking-webrtc+], [nICEr], [nICEr-upstream-needed] [qa-]
You need to log in before you can comment on or make changes to this bug.