Parse ?transport=[udp|tcp] in TURN uri

RESOLVED FIXED in mozilla27

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jib, Assigned: jib)

Tracking

Trunk
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [WebRTC])

Attachments

(1 attachment, 3 obsolete attachments)

Created attachment 818629 [details] [diff] [review]
Parse query-string in TURN URI by hand
Attachment #818629 - Flags: review?(ekr)
Depends on: 906968
Created attachment 818643 [details] [diff] [review]
Parse query-string in TURN URI by hand (2)

Wrong query separator.
Attachment #818629 - Attachment is obsolete: true
Attachment #818629 - Flags: review?(ekr)
Attachment #818643 - Flags: review?(ekr)
Comment on attachment 818643 [details] [diff] [review]
Parse query-string in TURN URI by hand (2)

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

This is generally the kind of thing I expected, but I don't understand all
the subtleties of URL parsing. Can you get whoever reviewed the initial
code to review?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +601,4 @@
>        int32_t questionmark = path.FindChar('?');
>        if (questionmark >= 0) {
> +        const char match[] = "transport=";
> +        const size_t matchlen = (sizeof(match)-1)/sizeof(*match);

sizeof(char) == 1, so you can just do sizeof(match) -1
Attachment #818643 - Flags: review?(ekr)
Comment on attachment 818643 [details] [diff] [review]
Parse query-string in TURN URI by hand (2)

(In reply to Eric Rescorla (:ekr) from comment #2)
> This is generally the kind of thing I expected, but I don't understand all
> the subtleties of URL parsing. Can you get whoever reviewed the initial
> code to review?

That was bz (Bug 825703, Comment 30) though he's on vacation. Someone on IRC suggested annevk, and jesup has worked on nsURI AFAIK. Trying them.

FWIW, I looked at nsSimpleURI uses, and it looks like most code uses GetQuery() and does their own parsing anyways.

Reviewers: Bug 825703 Comment 43 has good info on spec.

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +601,4 @@
> >        int32_t questionmark = path.FindChar('?');
> >        if (questionmark >= 0) {
> > +        const char match[] = "transport=";
> > +        const size_t matchlen = (sizeof(match)-1)/sizeof(*match);
> 
> sizeof(char) == 1, so you can just do sizeof(match) -1

Sure (I'm just used to having available a #define lengthof(a) (sizeof(a)/sizeof(*a))
Attachment #818643 - Flags: review?(rjesup)
Attachment #818643 - Flags: review?(annevk)

Comment 4

5 years ago
Comment on attachment 818643 [details] [diff] [review]
Parse query-string in TURN URI by hand (2)

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

I write http://url.spec.whatwg.org/ No C++.

However, if you simply search for "?", what happens for "...#?transport=udp"? Also, do you want to support "?tr%61nsport=udp"? If you want to follow HTML conventions here you really ought to use http://url.spec.whatwg.org/#application/x-www-form-urlencoded
Attachment #818643 - Flags: review?(annevk)

Comment 5

5 years ago
Comment on attachment 818643 [details] [diff] [review]
Parse query-string in TURN URI by hand (2)

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +585,5 @@
>      nsAutoCString spec;
>      rv = url->GetSpec(spec);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    // TODO(jib@mozilla.com): Revisit once nsURI has STUN/TURN support (Bug 833509)

Please document why this is a simple URI and not a standard URI explicitly. It's a bit weird to have a simple URI and then try to parse out its query string.

@@ +601,4 @@
>        int32_t questionmark = path.FindChar('?');
>        if (questionmark >= 0) {
> +        const char match[] = "transport=";
> +        const size_t matchlen = (sizeof(match)-1)/sizeof(*match);

Or, use mozilla::ArrayLength.

@@ +603,5 @@
> +        const char match[] = "transport=";
> +        const size_t matchlen = (sizeof(match)-1)/sizeof(*match);
> +
> +        for (int32_t i = questionmark, endPos; i >= 0; i = endPos) {
> +          endPos = path.FindCharInSet("&;", i + 1);

Let's not try to handle ';'.

@@ +604,5 @@
> +        const size_t matchlen = (sizeof(match)-1)/sizeof(*match);
> +
> +        for (int32_t i = questionmark, endPos; i >= 0; i = endPos) {
> +          endPos = path.FindCharInSet("&;", i + 1);
> +          const nsCSubstring &fieldvaluepair = Substring(path, i + 1, endPos);

Please assign the return value to an nsDependentCString.  You're creating a reference to a temporary returned by Substring.  I don't even know why this compiles. :(

@@ +605,5 @@
> +
> +        for (int32_t i = questionmark, endPos; i >= 0; i = endPos) {
> +          endPos = path.FindCharInSet("&;", i + 1);
> +          const nsCSubstring &fieldvaluepair = Substring(path, i + 1, endPos);
> +          if (StringHead(fieldvaluepair, matchlen).EqualsLiteral(match)) {

Hmm, why not use StringBeginsWith?

@@ +607,5 @@
> +          endPos = path.FindCharInSet("&;", i + 1);
> +          const nsCSubstring &fieldvaluepair = Substring(path, i + 1, endPos);
> +          if (StringHead(fieldvaluepair, matchlen).EqualsLiteral(match)) {
> +            transport = Substring(fieldvaluepair, matchlen);
> +            ToLowerCase (transport);

Nit: whitespace before parentheses.

@@ +636,5 @@
>        if (!aDst->addTurnServer(host.get(), port,
>                                 username.get(),
> +                               credential.get(),
> +                               (transport.IsEmpty() ?
> +                                kNrIceTransportUdp : transport.get()))) {

This is a matter of style, but I'd have the function return kNrIceTransportUdp in its transport argument if it cannot find the transport in the query part of the URI, but it's your call which way you want to go.
Attachment #818643 - Flags: review?(rjesup) → review-
Created attachment 819226 [details] [diff] [review]
Parse query-string in TURN URI by hand (3)

Sorry, got side-tracked today.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #5)
> > +    // TODO(jib@mozilla.com): Revisit once nsURI has STUN/TURN support (Bug 833509)
> 
> Please document why this is a simple URI and not a standard URI explicitly.
> It's a bit weird to have a simple URI and then try to parse out its query
> string.

Will do.

> >        int32_t questionmark = path.FindChar('?');
> >        if (questionmark >= 0) {
> > +        const char match[] = "transport=";
> > +        const size_t matchlen = (sizeof(match)-1)/sizeof(*match);
> 
> Or, use mozilla::ArrayLength.

Ah, that's what it's called, thanks!

> > +          endPos = path.FindCharInSet("&;", i + 1);
> 
> Let's not try to handle ';'.

Agreed.

> > +        for (int32_t i = questionmark, endPos; i >= 0; i = endPos) {
> > +          endPos = path.FindCharInSet("&;", i + 1);
> > +          const nsCSubstring &fieldvaluepair = Substring(path, i + 1, endPos);
> 
> Please assign the return value to an nsDependentCString.  You're creating a
> reference to a temporary returned by Substring.  I don't even know why this
> compiles. :(

That is right off the wiki! https://developer.mozilla.org/en-US/docs/Mozilla_internal_string_guide#Substrings_.28string_fragments.29 :-)

> > +        for (int32_t i = questionmark, endPos; i >= 0; i = endPos) {
> > +          endPos = path.FindCharInSet("&;", i + 1);
> > +          const nsCSubstring &fieldvaluepair = Substring(path, i + 1, endPos);
> > +          if (StringHead(fieldvaluepair, matchlen).EqualsLiteral(match)) {
> 
> Hmm, why not use StringBeginsWith?

Nice!

> >        if (!aDst->addTurnServer(host.get(), port,
> >                                 username.get(),
> > +                               credential.get(),
> > +                               (transport.IsEmpty() ?
> > +                                kNrIceTransportUdp : transport.get()))) {
> 
> This is a matter of style, but I'd have the function return
> kNrIceTransportUdp in its transport argument if it cannot find the transport
> in the query part of the URI, but it's your call which way you want to go.

I would too, except here kNrIceTransportUdp is actually a std::string from a deeper include file, so this way turned out nicer (less copying back and forth between std::string and nsString).
Attachment #818643 - Attachment is obsolete: true
Attachment #819226 - Flags: review?(ehsan)

Updated

5 years ago
Attachment #819226 - Flags: review?(ehsan) → review+
Created attachment 819398 [details] [diff] [review]
Parse query-string in TURN URI by hand (4). r=ehsan

Rebased to land before Bug 928060. Carrying forward r+ from ehsan.
Attachment #819226 - Attachment is obsolete: true
Attachment #819398 - Flags: review+
Blocks: 906968
No longer depends on: 906968
Keywords: checkin-needed
This added 9 static initializers. Putting global variables in a header is not the best idea ever. Using a string to select between tcp and udp is not the best idea ever either.
My eagerness to land backfired. I'll wait for reviews on bug 906968 to resolve nricectx.h
No longer blocks: 906968
Depends on: 906968
https://hg.mozilla.org/mozilla-central/rev/50ad84fae401
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 928930
You need to log in before you can comment on or make changes to this bug.