Catching more errors when parsing ICE server URLs.

Catching more errors when parsing ICE server URLs.

Every malformed URL should now produce an error message in JS, rather than
silently failing and possibly printing a warning message to the console (and
possibly crashing).

Also added some unit tests, and made "ParseIceServers" public.

BUG=445002
Committed: https://crrev.com/0a6c4ca942f3a25c15c7af64a9515d381c34cd9c
Cr-Commit-Position: refs/heads/master@{#10186}

[Taylor Brandstetter, 2015-09-15 22:03:54]

Patchset #4 (id:60001) has been deleted

[Taylor Brandstetter, 2015-09-15 22:36:04]

deadbeef@webrtc.org changed reviewers:
+ guoweis@webrtc.org, pthatcher@webrtc.org

[Taylor Brandstetter, 2015-09-15 22:36:05]

Beyond just catching more parsing errors, this CL also makes the PeerConnection
fail to be initialized if it encounters a parsing error. Previously, this would
only happen if there was an invalid "stun:"/"turn:"/etc. prefix. I'm pretty
confident we agreed this is the proper behavior, but it's probably worth
thinking about again.

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.cc (left):

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:266: 
I'm pretty confident this code can be safely removed, since it does the same
thing as the tokenizing code above. The only difference is that the code above
operates on the string "user@hostname:port", while the deleted code operates
"user@hostname". But this makes no difference since in either case it just peels
off the name before the '@' sign.

[pthatcher1, 2015-09-16 03:28:40]

lgtm

I think it would make sense to have tommi@ review this also.

[Taylor Brandstetter, 2015-09-16 17:19:05]

deadbeef@webrtc.org changed reviewers:
+ tommi@webrtc.org

[Taylor Brandstetter, 2015-09-16 17:19:06]

tommi@, could you take a quick look? Thanks.

[tommi, 2015-09-16 21:51:01]

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:71: "invalid"};
why do we need this last value?

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:74: STUN,     // Indicates a STUN server.
can you explicitly set this to 0 and add a comment for the enum that we actually
have a loop that assumes that the first value is 0 and all values are
incremental.
It would also be good to have a static_assert that INVALID equals
arraysize(kValidIceServiceTypes) - 1 (or arraysize(kValidIceServiceTypes) if we
don't need the "invalid" string above)

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:130: if
(type.compare(kValidIceServiceTypes[i]) == 0) {
nit: We don't actually need to create a new string just to compare a substring. 
I think that here we could just do:
if (in_str.compare(0, colonpos, kValidIceServiceTypes[i]) == 0) {

right?  I don't suppose there's a unit test that covers this function btw?

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:144: if
(in_str.find_first_not_of("0123456789") != std::string::npos) {
nit: could use isdigit() in a loop for O(n) ;)

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:147: return rtc::FromString(in_str, port);
out of curiosity I took a look at FromString() and it seems to create an
istringstream from the string before extracting the int and does some more
complex parsing too that you don't need here since you know you'll only be
dealing with digits.  I think this is probably enough and should generate
significantly smaller code:

if (in_str.empty())  <- this could also be the first thing the function checks
  return false;
*port = atoi(in_str.c_str());
return true;

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:156: std::string* host,
what about adding a DCHECK() at the top of this function:

DCHECK(host->empty());

(see the last check in the function)

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:158: if (in_str.at(0) == '[') {
Is there ever a chance that in_str is empty?

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:161: *host = in_str.substr(1, closebracket -
1);
nit: we don't really need to assign to host until after we've checked the port.
(I'm a fan of not having modified output parameters in case of failures)

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:175: *host = in_str.substr(0, colonpos);
same here

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:183: if (host->empty()) {
and here I'd skip the if() and just do:

return !host->empty();

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:193: StunConfigurations* stun_config,
can these params ever be null? If they're never allowed to be, can you add
DCHECKs to that effect?

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:249: } else if (tokens.size() ==
kTurnHostTokensNum) {
nit: since the previous scope has an early return, the 'else' part isn't needed.

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection_unittest.cc:1681:
EXPECT_FALSE(ParseUrl(":hostname"));
Can you add ":", "", "stun:" too?

https://codereview.webrtc.org/1344143002/diff/40001/webrtc/base/stringencode.cc
File webrtc/base/stringencode.cc (right):

https://codereview.webrtc.org/1344143002/diff/40001/webrtc/base/stringencode....
webrtc/base/stringencode.cc:579: DCHECK(fields);
since fields is deferenced anyway, this isn't needed (and discouraged actually)

https://codereview.webrtc.org/1344143002/diff/40001/webrtc/base/stringencode....
webrtc/base/stringencode.cc:580: fields->clear();
I'd prefer DCHECK(fields->empty()) instead.  |fields| will in practice always be
empty, so this is bloat, if there ever is a case where it's needed, the caller
can clear() before calling if appending is really not what's desired.

https://codereview.webrtc.org/1344143002/diff/40001/webrtc/base/stringencode....
webrtc/base/stringencode.cc:589: return fields->size();
if source is empty, won't fields->size() return 1? (I would expect the return
value to be 0 in that case)

https://codereview.webrtc.org/1344143002/diff/40001/webrtc/base/stringencode_...
File webrtc/base/stringencode_unittest.cc (right):

https://codereview.webrtc.org/1344143002/diff/40001/webrtc/base/stringencode_...
webrtc/base/stringencode_unittest.cc:308: EXPECT_EQ(3ul,
tokenize_with_empty_tokens("..c", '.', &fields));
add a test case for source == ""?

[Taylor Brandstetter, 2015-09-24 00:25:03]

deadbeef@webrtc.org changed reviewers:
- guoweis@webrtc.org

[Taylor Brandstetter, 2015-09-24 00:25:05]

I addressed most of your comments, Tommi. Note that the only real behavioral
change from last time is that we'll produce an error if you try to initialize a
PeerConnection with an IceServer with an empty URL.

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:71: "invalid"};
On 2015/09/16 21:51:00, tommi (webrtc) wrote:
> why do we need this last value?

It isn't; good catch.

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:74: STUN,     // Indicates a STUN server.
On 2015/09/16 21:51:01, tommi (webrtc) wrote:
> can you explicitly set this to 0 and add a comment for the enum that we
actually
> have a loop that assumes that the first value is 0 and all values are
> incremental.
> It would also be good to have a static_assert that INVALID equals
> arraysize(kValidIceServiceTypes) - 1 (or arraysize(kValidIceServiceTypes) if
we
> don't need the "invalid" string above)

Done.

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:130: if
(type.compare(kValidIceServiceTypes[i]) == 0) {
On 2015/09/16 21:51:00, tommi (webrtc) wrote:
> nit: We don't actually need to create a new string just to compare a
substring. 
> I think that here we could just do:
> if (in_str.compare(0, colonpos, kValidIceServiceTypes[i]) == 0) {
> 
> right?  I don't suppose there's a unit test that covers this function btw?

Done. And although there's no unit test for this function *individually*, it's
still covered by a test I added (IceServerParsingTest.ParseStunPrefixes).

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:144: if
(in_str.find_first_not_of("0123456789") != std::string::npos) {
On 2015/09/16 21:51:01, tommi (webrtc) wrote:
> nit: could use isdigit() in a loop for O(n) ;)

Done.

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:147: return rtc::FromString(in_str, port);
On 2015/09/16 21:51:00, tommi (webrtc) wrote:
> out of curiosity I took a look at FromString() and it seems to create an
> istringstream from the string before extracting the int and does some more
> complex parsing too that you don't need here since you know you'll only be
> dealing with digits.  I think this is probably enough and should generate
> significantly smaller code:
> 
> if (in_str.empty())  <- this could also be the first thing the function checks
>   return false;
> *port = atoi(in_str.c_str());
> return true;

atoi still allows whitespace and non-numeric characters though, so I still want
to do some sort of "is only digits" check. And "FromString(in_str, port)" looks
nicer (in my opinion) and is more consistent with other code than "*port =
atoi(in_str.c_str())"

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:156: std::string* host,
On 2015/09/16 21:51:01, tommi (webrtc) wrote:
> what about adding a DCHECK() at the top of this function:
> 
> DCHECK(host->empty());
> 
> (see the last check in the function)

Done.

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:158: if (in_str.at(0) == '[') {
On 2015/09/16 21:51:00, tommi (webrtc) wrote:
> Is there ever a chance that in_str is empty?

Not any longer.

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:161: *host = in_str.substr(1, closebracket -
1);
On 2015/09/16 21:51:00, tommi (webrtc) wrote:
> nit: we don't really need to assign to host until after we've checked the
port.
> (I'm a fan of not having modified output parameters in case of failures)

Done.

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:175: *host = in_str.substr(0, colonpos);
On 2015/09/16 21:51:00, tommi (webrtc) wrote:
> same here

Done.

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:183: if (host->empty()) {
On 2015/09/16 21:51:01, tommi (webrtc) wrote:
> and here I'd skip the if() and just do:
> 
> return !host->empty();

Done.

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:193: StunConfigurations* stun_config,
On 2015/09/16 21:51:01, tommi (webrtc) wrote:
> can these params ever be null? If they're never allowed to be, can you add
> DCHECKs to that effect?

Done.

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:249: } else if (tokens.size() ==
kTurnHostTokensNum) {
On 2015/09/16 21:51:00, tommi (webrtc) wrote:
> nit: since the previous scope has an early return, the 'else' part isn't
needed.

Done.

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/1344143002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection_unittest.cc:1681:
EXPECT_FALSE(ParseUrl(":hostname"));
On 2015/09/16 21:51:01, tommi (webrtc) wrote:
> Can you add ":", "", "stun:" too?

Done.

https://codereview.webrtc.org/1344143002/diff/40001/webrtc/base/stringencode.cc
File webrtc/base/stringencode.cc (right):

https://codereview.webrtc.org/1344143002/diff/40001/webrtc/base/stringencode....
webrtc/base/stringencode.cc:579: DCHECK(fields);
On 2015/09/16 21:51:01, tommi (webrtc) wrote:
> since fields is deferenced anyway, this isn't needed (and discouraged
actually)

Done.

https://codereview.webrtc.org/1344143002/diff/40001/webrtc/base/stringencode....
webrtc/base/stringencode.cc:580: fields->clear();
On 2015/09/16 21:51:01, tommi (webrtc) wrote:
> I'd prefer DCHECK(fields->empty()) instead.  |fields| will in practice always
be
> empty, so this is bloat, if there ever is a case where it's needed, the caller
> can clear() before calling if appending is really not what's desired.

There's a function specifically called "tokenize_append", so I think the
assumption is that "tokenize" (and by extension, tokenize_with_empty_tokens)
will overwrite the list. Also, if there's code that's doing a lot of tokenizing,
it would be convenient to just use one vector<string>, and not need to clear()
it every time.

https://codereview.webrtc.org/1344143002/diff/40001/webrtc/base/stringencode....
webrtc/base/stringencode.cc:589: return fields->size();
On 2015/09/16 21:51:01, tommi (webrtc) wrote:
> if source is empty, won't fields->size() return 1? (I would expect the return
> value to be 0 in that case)

This is actually the behavior I wanted, in this situation.

https://codereview.webrtc.org/1344143002/diff/40001/webrtc/base/stringencode_...
File webrtc/base/stringencode_unittest.cc (right):

https://codereview.webrtc.org/1344143002/diff/40001/webrtc/base/stringencode_...
webrtc/base/stringencode_unittest.cc:308: EXPECT_EQ(3ul,
tokenize_with_empty_tokens("..c", '.', &fields));
On 2015/09/16 21:51:01, tommi (webrtc) wrote:
> add a test case for source == ""?

Done.

[Taylor Brandstetter, 2015-10-06 17:31:47]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-10-06 17:31:48]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1344143002/#ps160001 (title:
"Merging with master.")

[commit-bot: I haz the power, 2015-10-06 17:31:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1344143002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1344143002/160001

[commit-bot: I haz the power, 2015-10-06 18:38:35]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-10-06 18:38:40]

Patchset 8 (id:??) landed as
https://crrev.com/0a6c4ca942f3a25c15c7af64a9515d381c34cd9c
Cr-Commit-Position: refs/heads/master@{#10186}