825703 - Remove hardwired STUN server -- or redirect to Moz stun server

Status: RESOLVED FIXED

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

Description

[Eric Rescorla (:ekr)]

We need to remove the current hardwired STUN server or at least move to one operated by Mozilla. Once bug 786236 is fixed, we can simply let the app supply it.

Comment 1

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

Attachment: pc_test.html sample with iceServers

A test case.

Comment 2

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

Attachment: Work in progress

Skeleton, doesn't do anything yet. Compiles, but pc::Initialize no longer gets called, so something is wrong. Do I need to clobber after changing idl files? Comments welcome.

Comment 3

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

Attachment: Work in progress

Nm. Forgot to look in my web console (no isObject() in scope). Skeleton now works. Onto the meat.

Comment 4

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

Attachment: Works minus DNS

Still need to add the DNS lookup.

Comment 5

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

Attachment: Stun configuration from JS

Ignores username, credentials and turn servers (except turn servers also act as stun servers) due to support missing in Bug 786236.

Remaining issue: Still defaults to the same hard-coded STUN server. Awaiting a Mozilla-approved replacement.

Comment 6

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

Attachment: pc_test.html sample with iceServers

Shows different configurations.

Comment 7

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

Comment on attachment 699413 [details] [diff] [review]
Stun configuration from JS

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

Needs major work to support async DNS resolution, or (temporarily) using numeric addresses only and warn for others.

This should work, but would cause MainThread stalls (UI impact).

Other than that issue, though, this is really good, thanks.

r? to mcmanus about the DNS resolution (a pointer to code to Cargo-Cult would be handy)
r? to smaug for DOM side check (it looks good to me, but I'm not a DOM person)

::: dom/media/PeerConnection.js
@@ +233,5 @@
>      Ci.nsIDOMRTCPeerConnection, Ci.nsIDOMGlobalObjectConstructor
>    ]),
>  
>    // Constructor is an explicit function, because of nsIDOMGlobalObjectConstructor.
> +  constructor: function(win,iceServers) {

space after comma (nit)

@@ +241,5 @@
>      if (this._win) {
>        throw new Error("Constructor already called");
>      }
>      if (_globalPCList._networkdown) {
>        throw new Error("Can't create RTPPeerConnections when the network is down");

Correct this to "RTCPeerConnections".  oops

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +301,5 @@
> +  if (!JS_IsArrayObject(aCx, &servers) || !JS_GetArrayLength(aCx, &servers, &len)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  bool foundOne = false;
> +  for (size_t i = 0; i < len; i++) {

JS_GetElement takes a uint32_t, as is len

@@ +321,5 @@
> +          return NS_ERROR_FAILURE;
> +      }
> +#endif
> +      using namespace std;
> +      string url = NS_ConvertUTF16toUTF8 (JS_GetStringCharsZ(aCx,

nit: no space before (

@@ +338,5 @@
> +          port = atoi(url.substr(pos+1).c_str());
> +          url = url.substr(0,pos);
> +        }
> +        PRAddrInfo *ai;
> +        ai = PR_GetAddrInfoByName(url.c_str(), PR_AF_UNSPEC, PR_AI_ADDRCONFIG);

This may be (almost certainly is) a no-no on MainThread because it blocks, and we want to do network IO from the SocketTransport thread.

You probably want to use the DNS Service and use AsyncResolve(), or something like it.

If we limit it to numeric addresses for now, we can avoid the name lookup temporarily, but get configurability.

This also means that this function becomes async (and you should fire off all resolutions at the same time)!!!!  So, we probably need to add some way to know if the async resolution is done, and let users query it or block on it's being resolved.  Ekr should be consulted, since if you have N servers, you don't want to block STUN resolution because DNS for one address is slow.  Probably you want to block if none have been looked up.  In theory, when new answers come in new STUN requests should go out if none of the current ones have answered.

Yes, this is icky.  I'll CC mcmanus on this for comment.

Comment 8

[Patrick McManus [:mcmanus]]

Comment on attachment 699413 [details] [diff] [review]
Stun configuration from JS

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +338,5 @@
> +          port = atoi(url.substr(pos+1).c_str());
> +          url = url.substr(0,pos);
> +        }
> +        PRAddrInfo *ai;
> +        ai = PR_GetAddrInfoByName(url.c_str(), PR_AF_UNSPEC, PR_AI_ADDRCONFIG);

its not tricky; you can't block the thread. If you do that the whole snappy team will line up at your door :)

use nsIDNSService::AsyncResolve(). That will get you a cache too.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 699413 [details] [diff] [review]
Stun configuration from JS

I'd rather review this one the DNS handling is fixed.

Nit, win,iceServers -> win, iceServers
[{url:"stun:216.93.246.14"} -> [{ url: "stun:216.93.246.14" }

The other Initialize which isn't in .idl doesn't need to be
NS_IMETHOD. Normal method returning nsresult should work just fine.

Comment 10

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

Attachment: Stun configuration from JS (IP numbers only)

Removes the DNS lookup for now, which means only IP numbers will work. Also addresses a crash in the previous patch here (prematurely filed as Bug 825651).

Submitting for review, as it might be desirable for testing to push this out while we work on AsyncResolve (leave bug open).

Comment 11

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

>(prematurely filed as Bug 825651)
Whoops, Typo. I meant Bug 829691.

Comment 12

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

Comment on attachment 701232 [details] [diff] [review]
Stun configuration from JS (IP numbers only)

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +316,5 @@
> +    if (JS_GetProperty(aCx, opts, "url", &jsUrl)) {
> +      if (!JSVAL_IS_STRING(jsUrl)) {
> +        return NS_ERROR_FAILURE;
> +      }
> +#if 0 // TODO([e-mail]): Support username, credentials & turns servers

trivial nit: "TURN servers"

@@ +398,5 @@
> +                               nsIThread* aThread,
> +                               JSContext* aCx) {
> +  IceConfiguration ic;
> +  nsresult rv = ConvertIceConfiguration(aIceConfiguration, &ic, aCx);
> +  if (rv != NS_OK) {

if (NS_FAILED(rv)) {

Comment 13

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

Attachment: Stun configuration from JS (IP numbers only)

Fixes crash vulnerability with certain invalid input in previous patch reported as Bug 829856. Also improves logging of invalid input.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 701428 [details] [diff] [review]
Stun configuration from JS (IP numbers only)


>+class Console
>+{
>+public:
>+  std::ostringstream& log() {
{ should be in the next line

>+    return mOs;
>+  }
>+  ~Console() {
ditto

>+    CSFLogErrorS(logTag, "Console: " << mOs.str());
>+    nsCOMPtr<nsIConsoleService> cs (do_GetService(NS_CONSOLESERVICE_CONTRACTID));
>+    if (cs) {
>+      cs->LogStringMessage(NS_ConvertUTF8toUTF16(mOs.str().c_str()).get());
>+    }
>+  }
>+private:
>+  std::ostringstream mOs;
>+};
(Somewhat surprising to see stl streams in Mozilla code, but apparently they are used elsewhere in webrtc code too)
But anyhow, I don't think we should log such stuff to console. Just throw exception.

>+/**
>+ * In JS, iceConfiguration looks like this:
>+ *
>+ *    [ { url:"stun:216.93.246.14" },
>+ *      { url:"turn:[e-mail]", credential:"myPassword"} ]
>+ *
>+ * This function converts a jsval that looks like the above into an
>+ * IceConfiguration object.
>+ */
>+nsresult
>+PeerConnectionImpl::ConvertIceConfiguration(const JS::Value& aSrc,
>+                                            IceConfiguration *aDst, JSContext* aCx)
>+{
>+  if (!aSrc.isObject()) {
>+    return NS_ERROR_FAILURE;
>+  }
>+  JSObject& servers = aSrc.toObject();
>+  uint32_t len;
>+  if (!JS_IsArrayObject(aCx, &servers) || !JS_GetArrayLength(aCx, &servers, &len)) {
>+    return NS_ERROR_FAILURE;
>+  }
>+  for (uint32_t i = 0; i < len; i++) {
>+    jsval server;
>+    JS_GetElement(aCx, &servers, i, &server);
in this method could you please try to use JS API as little as possible.
I believe if you added WebIDL file which has something like the following should work:
dictionary iceConfigurationItem
{
  DOMString url;
  DOMString? credential;
};

Then for each element do something like
JSAutoCompartment ac(aCX, server);
mozilla::dom::iceConfigurationItemInit d;
d.Init(aCx, server);

And then just read the values from d.
d.url would be nsAString and so on.

>+      string scheme = url.substr(0, pos);
Mozilla strings please when dealing with stuff coming from Gecko.
And you could probably just create an URI object and read scheme from there.


>+      transform(scheme.begin(), scheme.end(), scheme.begin(), ::tolower);
>+      if (scheme == "stun" || scheme == "stuns" || scheme == "turn")
>+      {
{ should be in the previous line

>+        url = url.substr(scheme.length()+1);
in this kind of case there should be space before and after +

>+class IceConfiguration {
{ goes to next line

Comment 15

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

(In reply to Olli Pettay [:smaug] from comment #14)

> >+    CSFLogErrorS(logTag, "Console: " << mOs.str());
> >+    nsCOMPtr<nsIConsoleService> cs (do_GetService(NS_CONSOLESERVICE_CONTRACTID));
> >+    if (cs) {
> >+      cs->LogStringMessage(NS_ConvertUTF8toUTF16(mOs.str().c_str()).get());
> >+    }
> >+  }
> >+private:
> >+  std::ostringstream mOs;
> >+};
> (Somewhat surprising to see stl streams in Mozilla code, but apparently they
> are used elsewhere in webrtc code too)

They're only used in DEBUG-only code (per bsmedberg and others who were concerned they caused problems in production builds), and mostly because there's a lot of imported debugs that use them.

> But anyhow, I don't think we should log such stuff to console. Just throw
> exception.

The concern was issues of runtime errors versus "programming" errors.  Exactly where the STUN server config comes from is still up in the air, but having a locl user-edited config is likely to be an option, so avoiding everyone having to always wrap "new RTCPeerConnection" in a try block is preferable - and I believe a parity issue; I don't think Chrome or others are expecting exceptions for this, though we can bring it up on the list (we just had a big "how are errors handled" discussion that's still on-going - generally "programming errors under control of the dev" are error returns (though for new I guess it has to be exceptions, though none have been anticipated by anyone), and runtime errors are via error callbacks, for the async operations (which is most of them)).

Comment 16

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

>> (Somewhat surprising to see stl streams in Mozilla code, but apparently they
>> are used elsewhere in webrtc code too)
>
> They're only used in DEBUG-only code (per bsmedberg and others who were concerned
> they caused problems in production builds), and mostly because there's a lot of
> imported debugs that use them.

In case it isn't clear, the logging added here is not DEBUG-only. It notifies app-developers of problems in their STUN configuration. We can't use streams in production code?

> Just throw exception.
That would require failing, which this doesn't do at the moment. Instead, the peer connection continues in spite of these "configuration warnings". Since STUN is "grease" (to make connections succeed for more people) which doesn't alter the meaning of the connection, it seemed correct to continue. Would people expect us NOT to attempt to connect because of an optional invalid STUN configuration? It seemed to me that if it can connect at all, it should. If it can't, it will fail when it gives up.

Since the config is an array, same question if it contains both good and bad entries. All or nothing? Please advise.

Another thing to consider is that once we switch to AsyncResolve, things like DNS would happen later, so the value of validating pure IP-numbers in the array this early seems low, and redundant once there's a pattern for dealing with bad settings downstream. Therefore, the only thing that seemed fatal to me was a failure to convert the desires of the config file into something that was read later.

Thanks for the tip on JSAutoCompartment. I'll use that.

Comment 17

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

Attachment: Stun configuration from JS (IP numbers only)

Updated based on review to use WebIDL bindings for RTCIceServer configuration, and emit to Web Console. Config errors are reported as warnings and cannot fail PeerConnection (line-number always shows as 1, to hide PeerConnection.js).

Not yet localized, as some warnings are temporary until we get async DNS.

Avoids streams, but I couldn't use nsURI since it doesn't support the stun, stuns and turn protocols (see non-working reference patch if you are interested).

Comment 18

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

Attachment: For reference only: add'l patch attempts to use nsURI (NON-WORKING)

For reference only (in case I missed something) my attempt at using nsURI, as an additional patch ontop. Doesn't work since nsURI doesn't recognize stun/turn and reverts to nsSimpleURI without host and port. Maybe revisit if nsURI ever adds stun/stuns/turn protocols.

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 702902 [details] [diff] [review]
Stun configuration from JS (IP numbers only)

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

::: dom/media/PeerConnection.js
@@ +251,5 @@
>  
> +    // Optional RTCIceServerConfiguration.
> +    // TODO([e-mail]): Replace hardcoded default with Mozilla one TBP
> +//    this.iceServers = iceServers || [{ url: "stun:216.93.246.18" }];
> +    this.iceServers = iceServers || [{ url: "stun:50.112.159.100" }];

Please don't leave commented-out code in the tree.

@@ +252,5 @@
> +    // Optional RTCIceServerConfiguration.
> +    // TODO([e-mail]): Replace hardcoded default with Mozilla one TBP
> +//    this.iceServers = iceServers || [{ url: "stun:216.93.246.18" }];
> +    this.iceServers = iceServers || [{ url: "stun:50.112.159.100" }];
> +    

Trailing whitespace

::: dom/media/bridge/IPeerConnection.idl
@@ +1,4 @@
>  #include "nsIThread.idl"
>  #include "nsIDOMWindow.idl"
>  #include "nsIPropertyBag2.idl"
> +#include "nsIArray.idl"

Why?

::: dom/webidl/RTCIceServer.webidl
@@ +5,5 @@
> + */
> +
> +dictionary RTCIceServer {
> +    DOMString  url;
> +    DOMString? credential;

As long as we don't do anything with credential, comment this out, please.

::: dom/webidl/WebIDL.mk
@@ +188,5 @@
> +ifdef MOZ_WEBRTC
> +webidl_files += \
> +  RTCIceServer.webidl \
> +  $(NULL)
> +endif

Put it in the block above.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +331,5 @@
> +nsresult
> +PeerConnectionImpl::ConvertIceConfiguration(const JS::Value& aSrc,
> +                                            IceConfiguration *aDst, JSContext* aCx)
> +{
> +  JSObject& servers = aSrc.toObject();

Don't do this before checking aSrc.isObject(), that'll assert.

@@ +333,5 @@
> +                                            IceConfiguration *aDst, JSContext* aCx)
> +{
> +  JSObject& servers = aSrc.toObject();
> +  uint32_t len;
> +  if (!(aSrc.isObject() && JS_IsArrayObject(aCx, &servers) &&

Check mozilla::dom::IsArrayLike instead.

@@ +339,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +#ifdef MOZILLA_INTERNAL_API
> +  for (uint32_t i = 0; i < len; i++) {
> +    jsval jsServer;

s/jsval/JS::Value/g

@@ +340,5 @@
> +  }
> +#ifdef MOZILLA_INTERNAL_API
> +  for (uint32_t i = 0; i < len; i++) {
> +    jsval jsServer;
> +    JS_GetElement(aCx, &servers, i, &jsServer);

Check if JS_GetElement return false, and throw if it didn't

@@ +343,5 @@
> +    jsval jsServer;
> +    JS_GetElement(aCx, &servers, i, &jsServer);
> +    if (!jsServer.isObject()) {
> +      return NS_ERROR_FAILURE;
> +    }

Drop this check; the dictionary code does it for you.

@@ +344,5 @@
> +    JS_GetElement(aCx, &servers, i, &jsServer);
> +    if (!jsServer.isObject()) {
> +      return NS_ERROR_FAILURE;
> +    }
> +    JSAutoCompartment ac(aCx, JSVAL_TO_OBJECT(jsServer));

Move this out of the loop. and use &servers instead.

@@ +352,5 @@
> +    }
> +    if (server.mUrl.WasPassed()) {
> +      // TODO([e-mail]): Support username, credentials & TURN servers
> +      using namespace std;
> +      string url = NS_ConvertUTF16toUTF8(server.mUrl.Value()).get();

This really should use our existing URL parsing code... NS_NewURI should work, I think.

@@ +416,5 @@
> +                               JSContext* aCx) {
> +  IceConfiguration ic;
> +  nsresult rv = ConvertIceConfiguration(aIceConfiguration, &ic, aCx);
> +  if (NS_FAILED(rv)) {
> +    // Non-fatal, and Initialize must not fail this early.

You need to return failure; doing otherwise will cause assertions. It's only going to happen if people pass stupid arguments anyway.

Comment 20

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

>> +dictionary RTCIceServer {
>> +    DOMString  url;
>> +    DOMString? credential;
>
> As long as we don't do anything with credential, comment this out, please.

Actually, I should probably check for credentials, then warn and omit those servers from the list to avoid access denied downstream. Thanks for reminding me.

> This really should use our existing URL parsing code... NS_NewURI should work,
> I think.

It doesn't work, as I explained. See the extra patch. When applied, NS_NewURI (fails to recognize stun and) creates an nsSimpleURI which doesn't implement GetHost() or GetPort(). If you know of a workaround please let me know.

> You need to return failure; doing otherwise will cause assertions. It's only going to happen if
> people pass stupid arguments anyway.

Thanks, I didn't know that. Let me get an answer from the domain as to whether it is desirable to fail, and either restructure the code so that it can fail safely or maybe use bool in place of nresult here.

Comment 21

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #20)
> >> +dictionary RTCIceServer {
> >> +    DOMString  url;
> >> +    DOMString? credential;
> >
> > As long as we don't do anything with credential, comment this out, please.
> 
> Actually, I should probably check for credentials, then warn and omit those
> servers from the list to avoid access denied downstream. Thanks for
> reminding me.
> 
> > This really should use our existing URL parsing code... NS_NewURI should work,
> > I think.
> 
> It doesn't work, as I explained. See the extra patch. When applied,
> NS_NewURI (fails to recognize stun and) creates an nsSimpleURI which doesn't
> implement GetHost() or GetPort(). If you know of a workaround please let me
> know.

I've hacked the URI classes (more than almost anyone around on the project); we can file a followon bug to teach them about STUN: and TURN:.

> > You need to return failure; doing otherwise will cause assertions. It's only going to happen if
> > people pass stupid arguments anyway.
> 
> Thanks, I didn't know that. Let me get an answer from the domain as to
> whether it is desirable to fail, and either restructure the code so that it
> can fail safely or maybe use bool in place of nresult here.

We don't want to block use of a PeerConnection because the user added a syntax-errored STUN entry, especially if others work.  However, this is debatable (and is being so in IRC)

Comment 22

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

Comment on attachment 702902 [details] [diff] [review]
Stun configuration from JS (IP numbers only)

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

r+ modulo DOM reviewer comments and your comments in response.

Note for NS_NewURI I can probably teach it about STUN: and TURN:; I've made more mods to that code than most other current mozilla people.  File a followup bug on me to do so.

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Randell Jesup [:jesup] from comment #21)
> > > You need to return failure; doing otherwise will cause assertions. It's only going to happen if
> > > people pass stupid arguments anyway.
> > 
> > Thanks, I didn't know that. Let me get an answer from the domain as to
> > whether it is desirable to fail, and either restructure the code so that it
> > can fail safely or maybe use bool in place of nresult here.
> 
> We don't want to block use of a PeerConnection because the user added a
> syntax-errored STUN entry, especially if others work.  However, this is
> debatable (and is being so in IRC)

WebIDL specifies that we need to throw in a number of cases here. Besides the fact that you won't be able to avoid throwing when we move WebRTC to the new bindings, it's not really us to you to second-guess the applicable specs here. Also note that most cases where we need to throw are those where the content JS threw an exception during the conversion; continuing to use the JSContext without propagating the exception back into JS *will* cause bugs.

Comment 24

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

Understood.  Is there a subset of errors in the user-specified stun servers (which I assume normally will be a list of stun:xxxxxxx URIs with optional credentials) that do not deserve this treatment?  Part of the problem here is we haven't specified how the user will provide local candidate STUN/TURN servers; the simplest form would read it from an about:config pref (and perhaps encourage users who need this, which is a smallish-though-vocal subset) to install an extension that makes it easy to set.  If so, we can largely assume that UI element does gross parsing checks on the input, and reasonably throw here.

So, given that design, we can probably throw on all the errors.  We should file a bug on creating such a pref that can be read from JS apps to combine with theirs, OR to always force the "local candidates" to be added (which may make more sense, as many apps won't bother even if it's in the examples, and provides the user a way to make sure any app can be made to work in their network, as high-volume TURN servers may be out of the reach of non-commercial WebRTC apps).

Comment 25

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

Attachment: Stun configuration from JS (IP numbers only)

Updated based on review.
- Invalid permutations of RTCConfiguration now throw.
- Warns and omits (not yet supported) servers with credentials.
- Uses nsURI.
- mochitest.

Comment 26

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 704214 [details] [diff] [review]
Stun configuration from JS (IP numbers only)

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

Looks good to me, but I'd like bz to just look at the JSAutoCompartment; I don't understand the contract for that.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +292,5 @@
>    return NS_OK;
>  }
>  
> +#ifdef MOZILLA_INTERNAL_API
> +static void warn(JSContext* aCx, const nsCString& aMsg) {

static void
Warn(JSContext* aCx, const nsCString& aMsg)
{

(three lines, upper-case W)

@@ +318,5 @@
> + * into an RTCConfiguration object.
> + */
> +nsresult
> +PeerConnectionImpl::ConvertRTCConfiguration(const JS::Value& aSrc,
> +                                      RTCConfiguration *aDst, JSContext* aCx)

Indentation is off here

@@ +329,5 @@
> +  uint32_t len;
> +  if (!JS_GetArrayLength(aCx, &servers, &len)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  JSAutoCompartment ac(aCx, &servers);

I think this should be before the JS_GetArrayLength call.

@@ +336,5 @@
> +      JS::Value v;
> +      if (!(JS_GetElement(aCx, &servers, i, &v) && server.Init(aCx, nullptr, v))) {
> +        return NS_ERROR_FAILURE;
> +      }
> +    }

Why the braces?

@@ +343,5 @@
> +    }
> +    nsRefPtr<nsIURI> url; {
> +      nsresult res;
> +      res = NS_NewURI(getter_AddRefs(url), server.mUrl.Value());
> +      NS_ENSURE_SUCCESS(res, res);

rv, not res

@@ +344,5 @@
> +    nsRefPtr<nsIURI> url; {
> +      nsresult res;
> +      res = NS_NewURI(getter_AddRefs(url), server.mUrl.Value());
> +      NS_ENSURE_SUCCESS(res, res);
> +    }

Same here

@@ +355,5 @@
> +    }
> +    nsAutoCString spec; {
> +      nsresult res = url->GetSpec(spec);
> +      NS_ENSURE_SUCCESS(res, res);
> +    }

Same here about the braces

@@ +364,5 @@
> +      continue;
> +    }
> +    if (isTurn) {
> +      warn(aCx, nsPrintfCString(ICE_PARSING
> +          ": TURN servers not yet supported. Treating as STUN: \"%s\"", spec.get()));

Do you want to continue here?

@@ +382,5 @@
> +        port = atoi(s.substr(pos + 1).c_str());
> +        s = s.substr(0, pos);
> +      }
> +      host = s;
> +    }

Still confused about the braces

@@ +397,5 @@
>  PeerConnectionImpl::Initialize(IPeerConnectionObserver* aObserver,
>                                 nsIDOMWindow* aWindow,
> +                               const JS::Value &aRTCConfiguration,
> +                               nsIThread* aThread,
> +                               JSContext* aCx) {

{ on the next line

@@ +407,5 @@
> +                               nsIDOMWindow* aWindow,
> +                               const RTCConfiguration* aConfiguration,
> +                               const JS::Value* aRTCConfiguration,
> +                               nsIThread* aThread,
> +                               JSContext* aCx) {

Here too

@@ +413,5 @@
>    MOZ_ASSERT(aObserver);
>    MOZ_ASSERT(aThread);
>    mPCObserver = aObserver;
> +  mThread = aThread;
> +  

Trailing whitespace

@@ +721,4 @@
>    JSObject& constraints = aConstraints.toObject();
>  
>    // Mandatory constraints.
>    if (JS_GetProperty(aCx, &constraints, "mandatory", &mandatory)) {

Followup to make this function follow JSAPI rules, please.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +64,5 @@
>  
> +class RTCConfiguration
> +{
> +public:
> +  bool addServer(const std::string& addr, uint16_t port) {

{ on the next line

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to :Ms2ger from comment #26)
> @@ +721,4 @@
> >    JSObject& constraints = aConstraints.toObject();
> >  
> >    // Mandatory constraints.
> >    if (JS_GetProperty(aCx, &constraints, "mandatory", &mandatory)) {
> 
> Followup to make this function follow JSAPI rules, please.

Apparently bug 826807.

Comment 28

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

> >   RTCIceServer server; {
> >     JS::Value v;
> >     if (!(JS_GetElement(aCx, &servers, i, &v) && server.Init(aCx, nullptr, v))) {
> >       return NS_ERROR_FAILURE;
> >     }
> >   }
> Why the braces?

For the same reasons we declare local variables as near to their use as possible: Reducing the scope of local variable 'v' increases legibility and reduces the likelihood of error. Only 'server' is used below.

Avoids having server, jsServer, jsUrl, url, urlStr, etc. (type-conversions obscure function imho).
Should I have put the opening brace on its own line?

> >   ": TURN servers not yet supported. Treating as STUN: \"%s\"", spec.get()));
>
> Do you want to continue here?

Someone told me TURN servers also function as STUN servers, so I thought it made sense to add them as STUN servers for now.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 704214 [details] [diff] [review]
Stun configuration from JS (IP numbers only)

> Looks good to me, but I'd like bz to just look at the JSAutoCompartment;

That JSAutoCompartment makes no sense, sorry.  Either you're already in that compartment, or you crashed when calling JS_GetArrayLength.

What is that line trying to accomplish?

Comment 30

[Boris Zbarsky [:bzbarsky]]

On a side note, the "revisit once ..." code that hand-parses URIs gives me the willies.  If it were doing that with HTTP URIs, it would certainly be bogus, but maybe the protocol handler here forbids things like '@' in the path?

Comment 31

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

The code works with "turn:[e-mail]" fwiw. There is no protocol-handler (the protocol handler is simpleURI).

The JSAutoCompartment was above JS_GetArrayLength initially, I moved it by mistake. I got it from smaug in Comment 14, but it is perhaps not needed? The array is passed into the RTCPeerConnection constructor.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 704214 [details] [diff] [review]
Stun configuration from JS (IP numbers only)

Could you move the autocompartment to be before you access arraylength.

Comment 33

[Boris Zbarsky [:bzbarsky]]

> The code works with "turn:[e-mail]" fwiw.

Does it work with:

  turn://turn.example.org/[e-mail]/

?  Or more to the point, what does it end up doing in that case?  What _should_ it end up doing?

> but it is perhaps not needed? 

As far as I can tell, it shouldn't be needed in this code...

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Can we guarantee addons doing the right thing with compartments?

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Or do we not need to worry about them here?

Comment 36

[Boris Zbarsky [:bzbarsky]]

I was assuming no direct C++ calls to this code.  If we plan to have addons calling this code, then we do need some sort of compatment-enter, because there is in fact no way to guarantee that addons won't mess it up.  In fact, I can guarantee they _will_ mess it up.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Since this isn't perf critical code, and exposed via old style .idl, I'd prefer having the
autoentercompartment (rather than sg:crit bug when some addon does something unexpected)

Comment 38

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

Attachment: Stun configuration from JS (IP numbers only)

Updated based on review.
- Moved JSAutoCompartment GetArrayLength
- Added a test for "evil @".

> Does it work with:
>
>  turn://turn.example.org/[e-mail]/
>
> ?  Or more to the point, what does it end up doing in that case?
> What _should_ it end up doing?

I don't believe stun servers have paths (we convert them to IP addresses downstream), but just in case, I improved the code to tolerate the @-case you mentioned.

Comment 39

[Boris Zbarsky [:bzbarsky]]

> I don't believe stun servers have paths

Right.  My question is what the parsing code will end up doing given malicious input of various sorts (and someone who knows what we actually use the string for would be a better judge than me in deciding what sorts of malicious input are likely to be dangerous here).  Or is this a trusted API that will never get malicious input?

Comment 40

[Eric Rescorla (:ekr)]

Well, it's plumbed to some piece of content-provided JS-code. So clearly, nothing could go wrong...

Comment 41

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

This is the constructor to RTCPeerConnection, so this is app input. The string gets converted to an IP address and a port immediately in the addServer call by creating an NrIceStunServer from Bug 786236 which calls PR_StringToNetAddr:

+  NrIceStunServer() : addr_() {}
+
+  nsresult Init(const std::string& addr, uint16_t port) {
+    PRStatus status = PR_StringToNetAddr(addr.c_str(), &addr_);
+    if (status != PR_SUCCESS)
+      return NS_ERROR_INVALID_ARG;
+    addr_.inet.port = PR_htons(port);
+    return NS_OK;
+  }

So this is the end of parsing. The app is free to provide any IP address it wants.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 704745 [details] [diff] [review]
Stun configuration from JS (IP numbers only)

OK, so more or less in order, after ignoring the JS bits...

>+++ b/dom/webidl/RTCIceServer.webidl
>+    DOMString? credential;

Should that be "= null"?  Doesn't matter too much for this patch, since it's not like we do anything with it, but in general.

>+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp

>+static void warn(JSContext* aCx, const nsCString& aMsg) {
+      nsContentUtils::ReportToConsoleNonLocalized(NS_ConvertUTF8toUTF16 (aMsg.get()),

Why is that .get() there?  I don't think it should be.

>+PeerConnectionImpl::ConvertRTCConfiguration(const JS::Value& aSrc,
>+                                      RTCConfiguration *aDst, JSContext* aCx)

Please fix the indent.

>+  JSAutoCompartment ac(aCx, &servers);

This needs to be the very first thing after we've determined we have an object, if we really think we need it.  Right now IsArrayLike happens to not assert anything about compartments, but that looks like an oversight to me.

>+      if ((pos = s.find('@')) != s.npos && pos == s.find_first_of("@/")) {

I can't tell you whether this is in fact safe or not.  Again, someone who actually understands the structure of stun URIs and the implications of this test testing false needs to review this part.

My gut feeling though, is that the chances of any hand-written URI-parsing code not being a security bug are negligible.

>+      if ((pos = s.find_last_of(':')) != s.npos) {

  stun:[e-mail]/evil.com:22

and Seriously, hand-parsing URIs is not something you want to be doing, generally.  Again, whether this is a real problem or not is best determined by someone familiar with what this code is trying to do.

That said, I'm somewhat interested why this is using std::string instead of just using the existing nsCString you have.  And whether (1) we've verified that none of the std::string functionality we're using throws exceptions (because if it can throw exceptions, you can't use it in code compiled without exceptions support) and (2) we're OK with the various string copying we end up doing here as a result of using std::string.

As far as that goes, have we verified that none of the uses of std::vector in here can throw exceptions?  If not, I recommend using nsTArray instead...

Comment 43

[Adam Roach [:abr]]

For bz's benefit: the STUN URL scheme is defined in http://tools.ietf.org/html/draft-nandakumar-rtcweb-stun-uri-02 and TURN is described in http://tools.ietf.org/html/draft-petithuguenin-behave-turn-uri-03 (but note that neither of these have been adopted by an IETF working group, so they're still very much works in progress).

In layman's terms, the only valid components in the existing drafts are:

  <scheme>:<host>[:<port>]["?transport="<transport>]

Jan-Ivar points out that the W3C spec gives an example that doesn't conform to this syntax (http://dev.w3.org/2011/webrtc/editor/webrtc.html#rtcconfiguration-type contains a <username>) so there is still some harmonization necessary.

In any case, due to the nature of the STUN and TURN protocols, you're not going to see paths in their URLs, so I think it's safe to discard as invalid any that contain slashes.

Extra credit reading on the design of the URL syntax: http://blog.marc.petit-huguenin.org/2012/09/on-design-of-stun-and-turn-uri-formats.html

Comment 44

[Boris Zbarsky [:bzbarsky]]

Thanks.  So looks like the relevant BNF is:

      stun:<stun-host>:<stun-port>
      stuns:<stun-host>:<stun-port>

   turnURI   = scheme ":" host [ ":" port ] [ "?transport=" transport ]
   transport = "udp" / "tcp" / transport-ext
   transport-ext = 1*unreserved

We should probably actually enforce those restrictions somewhere.  ;)

Note that jib and I talked this over on IRC.  He's going to try using nsIURLParser.parseAuthority here for extracting the host and port, not least because that will handle things like the ':' in ipv6 addresses correctly.

Comment 45

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

I've filed bug 833509 to add protocol handlers for stun: etc. to the nsURI.

Comment 46

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

Attachment: Stun configuration from JS (IP numbers only)

Comment 47

[Timothy B. Terriberry (:derf)]

(In reply to Boris Zbarsky (:bz) from comment #42)
> just using the existing nsCString you have.  And whether (1) we've verified
> that none of the std::string functionality we're using throws exceptions
> (because if it can throw exceptions, you can't use it in code compiled
> without exceptions support) and (2) we're OK with the various string copying

Without speaking to the appropriateness of std::string vs. nsCString here, I thought we had stl-wrapper headers specifically designed to white-list a small set of STL classes that are safe to use (including std::string and std::vector). Is this not true? They get used a lot in Gecko.

(In reply to Boris Zbarsky (:bz) from comment #44)
> Note that jib and I talked this over on IRC.  He's going to try using
> nsIURLParser.parseAuthority here for extracting the host and port, not least
> because that will handle things like the ':' in ipv6 addresses correctly.

Does this also handle IRIs?

Comment 48

[Eric Rescorla (:ekr)]

(In reply to Timothy B. Terriberry (:derf) from comment #47)
> (In reply to Boris Zbarsky (:bz) from comment #42)
> > just using the existing nsCString you have.  And whether (1) we've verified
> > that none of the std::string functionality we're using throws exceptions
> > (because if it can throw exceptions, you can't use it in code compiled
> > without exceptions support) and (2) we're OK with the various string copying
> 
> Without speaking to the appropriateness of std::string vs. nsCString here, I
> thought we had stl-wrapper headers specifically designed to white-list a
> small set of STL classes that are safe to use (including std::string and
> std::vector). Is this not true? They get used a lot in Gecko.

We use std::string all over PeerConnection and the code it transitively includes.
The reason (aside from the fact that this is imported code which uses it)
is that using ns*String causes terrible linkage problems in the standalone
unit tests. Also, it's specifically on the approved list:

http://dxr.mozilla.org/mozilla-central/config/stl-headers.html


WRT to the question of exception safety, the situation is somewhat more
nuanced. As long as you are willing to accept a crash whenever an exception
is thrown, then it's fine to use exception-throwing code in code that
does -fno-exceptions. A prime example is new's throwing std::bad_alloc
on out of memory conditions.

Comment 49

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

I forgot to mention that the latest patch no longer hand-parses using std:string and uses nsURI + nsIURLParser.ParseAuthority() instead.

Comment 50

[Boris Zbarsky [:bzbarsky]]

> Is this not true?

I don't know.  Sounds like it is, based on Eric's comment, so good.

> Does this also handle IRIs?

It works on a char*; it assumes that you're working with a UTF-8 string before you call into it.  Should be OK, here, since the string is coming from a GetSpec() on an nsIURI.

Comment 51

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 705111 [details] [diff] [review]
Stun configuration from JS (IP numbers only)

r=me.  Thanks!

Comment 52

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 705111 [details] [diff] [review]
Stun configuration from JS (IP numbers only)

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

If bz has seen it, that's enough for me.

Comment 53

[Anne (:annevk)]

Can we make this scheme consistent with what we have for http/https and ws/wss? That way parsing would come for free: http://url.spec.whatwg.org/

I guess it is not too bad the way it is now.

Comment 54

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

https://tbpl.mozilla.org/?tree=Try&rev=24dc0466b7ce

Comment 55

[Eric Rescorla (:ekr)]

(In reply to Anne van Kesteren from comment #53)
> Can we make this scheme consistent with what we have for http/https and
> ws/wss? That way parsing would come for free: http://url.spec.whatwg.org/

These URIs are being designed at IETF. I would suggest sending comments to
the IETF BEHAVE WG.

Comment 56

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

Attachment: Stun configuration from JS (IP numbers only). r=bzbarsky, rjesup

Mochitest is now desktop-only to not bust android (learned from bug 827843).

Re-trying (w/mochitest-4 this time): https://tbpl.mozilla.org/?tree=Try&rev=6e967f889386

Comment 57

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b5016ab9ebb

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6b5016ab9ebb

Comment 59

[Martin Thomson [:mt:]]

Comment on attachment 705728 [details] [diff] [review]
Stun configuration from JS (IP numbers only). r=bzbarsky, rjesup

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

::: dom/media/PeerConnection.js
@@ +338,5 @@
> +   * ErrorMsg is passed in to detail which array-entry failed, if any.
> +   */
> +  _mustValidateRTCConfiguration: function(rtcConfig, errorMsg) {
> +    function isObject(obj) {
> +      return obj && (typeof obj === "object");

(typeof obj === "object") might be sufficient, but it might also not be, depending on whether you want to catch this being an Array or not.  Good news is, see next comment.

@@ +340,5 @@
> +  _mustValidateRTCConfiguration: function(rtcConfig, errorMsg) {
> +    function isObject(obj) {
> +      return obj && (typeof obj === "object");
> +    }
> +    function isArray(obj) {

Array.isArray() not good enough?

@@ +529,5 @@
>    },
>  
>    addIceCandidate: function(cand) {
>      if (!cand) {
> +      throw new Error ("NULL candidate passed to addIceCandidate!");

Inconsistent whitespace before open paren.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +362,5 @@
> +    url->SchemeIs("turn", &isTurn);
> +    url->SchemeIs("turns", &isTurns);
> +    if (!(isStun || isStuns || isTurn || isTurns)) {
> +      return NS_ERROR_FAILURE;
> +    }

Why duplicate all this validation stuff here?  The JavaScript already checks this.

Comment 60

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

(In reply to Martin Thomson [:mt] from comment #59)
> Comment on attachment 705728 [details] [diff] [review]
> Stun configuration from JS (IP numbers only). r=bzbarsky, rjesup
> 
> Review of attachment 705728 [details] [diff] [review]:

This is an old patch and does not need review, but thanks. ;-)

> ::: dom/media/PeerConnection.js
> @@ +338,5 @@
> > +   * ErrorMsg is passed in to detail which array-entry failed, if any.
> > +   */
> > +  _mustValidateRTCConfiguration: function(rtcConfig, errorMsg) {
> > +    function isObject(obj) {
> > +      return obj && (typeof obj === "object");
> 
> (typeof obj === "object") might be sufficient, but it might also not be,
> depending on whether you want to catch this being an Array or not.  Good
> news is, see next comment.
> 
> @@ +340,5 @@
> > +  _mustValidateRTCConfiguration: function(rtcConfig, errorMsg) {
> > +    function isObject(obj) {
> > +      return obj && (typeof obj === "object");
> > +    }
> > +    function isArray(obj) {
> 
> Array.isArray() not good enough?

This was one of my first patches. The most recent code is probably more helpful http://mxr.mozilla.org/mozilla-central/source/dom/media/PeerConnection.js#397

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +362,5 @@
> > +    url->SchemeIs("turn", &isTurn);
> > +    url->SchemeIs("turns", &isTurns);
> > +    if (!(isStun || isStuns || isTurn || isTurns)) {
> > +      return NS_ERROR_FAILURE;
> > +    }
> 
> Why duplicate all this validation stuff here?  The JavaScript already checks
> this.

The JS does basic dictionary and URI scheme checks, and gives nice readable error messages.
The c++ parses out the needed components from the URI, but when it fails it's just NS_ERROR_FAILURE.
Hence the partial overlap division of labor.

The pieces I thought you might be able to reuse in Bug 884573 is the code around ParseAuthority() here http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#604 - it handles everything but paths.