The default bug view has changed. See this FAQ.

Add per-PeerConnection STUN configuration

RESOLVED FIXED in mozilla21

Status

()

Core
WebRTC: Networking
P2
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: ekr, Assigned: ekr)

Tracking

unspecified
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.

Updated

5 years ago
Whiteboard: [WebRTC]

Updated

5 years ago
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
(Assignee)

Comment 2

4 years ago
This might be a good one for Jan-Ivar.
(Assignee)

Comment 3

4 years ago
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
(Assignee)

Comment 4

4 years ago
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.
(Assignee)

Comment 6

4 years ago
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.
(Assignee)

Comment 8

4 years ago
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)

Updated

4 years ago
Assignee: jib → ekr
(Assignee)

Comment 10

4 years ago
Created attachment 696725 [details] [diff] [review]
Per-ctx configurable STUN servers

* * *
Configure STUN servers from JS
(Assignee)

Comment 11

4 years ago
Created attachment 696730 [details] [diff] [review]
Per-ctx configurable STUN servers

* * *
Configure STUN servers from JS
(Assignee)

Updated

4 years ago
Attachment #696725 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #696730 - Flags: review?(adam)
(Assignee)

Updated

4 years ago
Blocks: 825703

Updated

4 years ago
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+], [nICEr]

Comment 12

4 years ago
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+
(Assignee)

Comment 13

4 years ago
> 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.
(Assignee)

Comment 14

4 years ago
Created attachment 698334 [details] [diff] [review]
Per-ctx configurable STUN servers

* * *
Configure STUN servers from JS
(Assignee)

Comment 15

4 years ago
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 16

4 years ago
Comment on attachment 698334 [details] [diff] [review]
Per-ctx configurable STUN servers

lgtm
Attachment #698334 - Flags: review?(adam) → review+

Updated

4 years ago
Attachment #696730 - Attachment is obsolete: true
Depends on: 829691
Depends on: 829856
No longer depends on: 829856
Created attachment 702857 [details] [diff] [review]
Per-ctx configurable STUN servers. r=adam

Patch got stale because of Bug 807494 landing. Refreshed.
Attachment #698334 - Attachment is obsolete: true
Attachment #702857 - Flags: review+

Updated

4 years ago
Whiteboard: [WebRTC], [blocking-webrtc+], [nICEr] → [WebRTC], [blocking-webrtc+], [nICEr], [nICEr-upstream-needed]
https://hg.mozilla.org/integration/mozilla-inbound/rev/0eda7bbdfedf
Target Milestone: --- → mozilla21
https://hg.mozilla.org/mozilla-central/rev/0eda7bbdfedf
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Updated

4 years ago
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.