Reland "ice server parsing: return RTCError with more details"
This is a reland of commit c4b0bde7f2daabc4e0667fb73d096d1cf0819226
which changes the name of the new method and adds a deprecated
backward compatible variant with the old name.
Original change's description:
> ice server parsing: return RTCError with more details
>
> surfacing those errors to the API (without specific details) instead of just the logging.
>
> BUG=webrtc:14539
>
> Change-Id: Id907ebeb08b96b0e4225a016a37a12d58889091b
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/278340
> Reviewed-by: Henrik Boström <hbos@webrtc.org>
> Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> Commit-Queue: Philipp Hancke <phancke@microsoft.com>
> Cr-Commit-Position: refs/heads/main@{#38356}
Bug: webrtc:14539
Change-Id: I0a5482e123f25867582d62101b31ed207b95ea1a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/278962
Commit-Queue: Philipp Hancke <phancke@microsoft.com>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38364}
diff --git a/pc/ice_server_parsing.cc b/pc/ice_server_parsing.cc
index 8052cd8..1a30d2d 100644
--- a/pc/ice_server_parsing.cc
+++ b/pc/ice_server_parsing.cc
@@ -155,7 +155,7 @@
// Adds a STUN or TURN server to the appropriate list,
// by parsing `url` and using the username/password in `server`.
-RTCErrorType ParseIceServerUrl(
+RTCError ParseIceServerUrl(
const PeerConnectionInterface::IceServer& server,
absl::string_view url,
cricket::ServerAddresses* stun_servers,
@@ -186,20 +186,24 @@
std::vector<absl::string_view> transport_tokens =
rtc::split(tokens[1], '=');
if (transport_tokens[0] != kTransport) {
- RTC_LOG(LS_WARNING) << "Invalid transport parameter key.";
- return RTCErrorType::SYNTAX_ERROR;
+ LOG_AND_RETURN_ERROR(
+ RTCErrorType::SYNTAX_ERROR,
+ "ICE server parsing failed: Invalid transport parameter key.");
}
if (transport_tokens.size() < 2) {
- RTC_LOG(LS_WARNING) << "Transport parameter missing value.";
- return RTCErrorType::SYNTAX_ERROR;
+ LOG_AND_RETURN_ERROR(
+ RTCErrorType::SYNTAX_ERROR,
+ "ICE server parsing failed: Transport parameter missing value.");
}
absl::optional<cricket::ProtocolType> proto =
cricket::StringToProto(transport_tokens[1]);
if (!proto ||
(*proto != cricket::PROTO_UDP && *proto != cricket::PROTO_TCP)) {
- RTC_LOG(LS_WARNING) << "Transport parameter should always be udp or tcp.";
- return RTCErrorType::SYNTAX_ERROR;
+ LOG_AND_RETURN_ERROR(
+ RTCErrorType::SYNTAX_ERROR,
+ "ICE server parsing failed: Transport parameter should "
+ "always be udp or tcp.");
}
turn_transport_type = *proto;
}
@@ -207,8 +211,10 @@
auto [service_type, hoststring] =
GetServiceTypeAndHostnameFromUri(uri_without_transport);
if (service_type == ServiceType::INVALID) {
- RTC_LOG(LS_WARNING) << "Invalid transport parameter in ICE URI: " << url;
- return RTCErrorType::SYNTAX_ERROR;
+ RTC_LOG(LS_ERROR) << "Invalid transport parameter in ICE URI: " << url;
+ LOG_AND_RETURN_ERROR(
+ RTCErrorType::SYNTAX_ERROR,
+ "ICE server parsing failed: Invalid transport parameter in ICE URI");
}
// GetServiceTypeAndHostnameFromUri should never give an empty hoststring
@@ -221,22 +227,25 @@
}
if (hoststring.find('@') != absl::string_view::npos) {
- RTC_LOG(LS_WARNING) << "Invalid url: " << uri_without_transport;
- RTC_LOG(LS_WARNING)
- << "Note that user-info@ in turn:-urls is long-deprecated.";
- return RTCErrorType::SYNTAX_ERROR;
+ RTC_LOG(LS_ERROR) << "Invalid url with long deprecated user@host syntax: "
+ << uri_without_transport;
+ LOG_AND_RETURN_ERROR(RTCErrorType::SYNTAX_ERROR,
+ "ICE server parsing failed: Invalid url with long "
+ "deprecated user@host syntax");
}
auto [success, address, port] =
ParseHostnameAndPortFromString(hoststring, default_port);
if (!success) {
- RTC_LOG(LS_WARNING) << "Invalid hostname format: " << uri_without_transport;
- return RTCErrorType::SYNTAX_ERROR;
+ RTC_LOG(LS_ERROR) << "Invalid hostname format: " << uri_without_transport;
+ LOG_AND_RETURN_ERROR(RTCErrorType::SYNTAX_ERROR,
+ "ICE server parsing failed: Invalid hostname format");
}
if (port <= 0 || port > 0xffff) {
- RTC_LOG(LS_WARNING) << "Invalid port: " << port;
- return RTCErrorType::SYNTAX_ERROR;
+ RTC_LOG(LS_ERROR) << "Invalid port: " << port;
+ LOG_AND_RETURN_ERROR(RTCErrorType::SYNTAX_ERROR,
+ "ICE server parsing failed: Invalid port");
}
switch (service_type) {
@@ -249,8 +258,10 @@
if (server.username.empty() || server.password.empty()) {
// The WebRTC spec requires throwing an InvalidAccessError when username
// or credential are ommitted; this is the native equivalent.
- RTC_LOG(LS_WARNING) << "TURN server with empty username or password";
- return RTCErrorType::INVALID_PARAMETER;
+ LOG_AND_RETURN_ERROR(
+ RTCErrorType::INVALID_PARAMETER,
+ "ICE server parsing failed: TURN server with empty "
+ "username or password");
}
// If the hostname field is not empty, then the server address must be
// the resolved IP for that host, the hostname is needed later for TLS
@@ -263,10 +274,11 @@
if (!IPFromString(address, &ip)) {
// When hostname is set, the server address must be a
// resolved ip address.
- RTC_LOG(LS_WARNING)
- << "IceServer has hostname field set, but URI does not "
- "contain an IP address.";
- return RTCErrorType::INVALID_PARAMETER;
+ LOG_AND_RETURN_ERROR(
+ RTCErrorType::INVALID_PARAMETER,
+ "ICE server parsing failed: "
+ "IceServer has hostname field set, but URI does not "
+ "contain an IP address.");
}
socket_address.SetResolvedIP(ip);
}
@@ -287,15 +299,16 @@
default:
// We shouldn't get to this point with an invalid service_type, we should
// have returned an error already.
- RTC_DCHECK_NOTREACHED() << "Unexpected service type";
- return RTCErrorType::INTERNAL_ERROR;
+ LOG_AND_RETURN_ERROR(
+ RTCErrorType::INTERNAL_ERROR,
+ "ICE server parsing failed: Unexpected service type");
}
- return RTCErrorType::NONE;
+ return RTCError::OK();
}
} // namespace
-RTCErrorType ParseIceServers(
+RTCError ParseIceServersOrError(
const PeerConnectionInterface::IceServers& servers,
cricket::ServerAddresses* stun_servers,
std::vector<cricket::RelayServerConfig>* turn_servers) {
@@ -303,25 +316,26 @@
if (!server.urls.empty()) {
for (const std::string& url : server.urls) {
if (url.empty()) {
- RTC_LOG(LS_WARNING) << "Empty uri.";
- return RTCErrorType::SYNTAX_ERROR;
+ LOG_AND_RETURN_ERROR(RTCErrorType::SYNTAX_ERROR,
+ "ICE server parsing failed: Empty uri.");
}
- RTCErrorType err =
+ RTCError err =
ParseIceServerUrl(server, url, stun_servers, turn_servers);
- if (err != RTCErrorType::NONE) {
+ if (!err.ok()) {
return err;
}
}
} else if (!server.uri.empty()) {
// Fallback to old .uri if new .urls isn't present.
- RTCErrorType err =
+ RTCError err =
ParseIceServerUrl(server, server.uri, stun_servers, turn_servers);
- if (err != RTCErrorType::NONE) {
+
+ if (!err.ok()) {
return err;
}
} else {
- RTC_LOG(LS_WARNING) << "Empty uri.";
- return RTCErrorType::SYNTAX_ERROR;
+ LOG_AND_RETURN_ERROR(RTCErrorType::SYNTAX_ERROR,
+ "ICE server parsing failed: Empty uri.");
}
}
// Candidates must have unique priorities, so that connectivity checks
@@ -331,7 +345,14 @@
// First in the list gets highest priority.
turn_server.priority = priority--;
}
- return RTCErrorType::NONE;
+ return RTCError::OK();
+}
+
+RTCErrorType ParseIceServers(
+ const PeerConnectionInterface::IceServers& servers,
+ cricket::ServerAddresses* stun_servers,
+ std::vector<cricket::RelayServerConfig>* turn_servers) {
+ return ParseIceServersOrError(servers, stun_servers, turn_servers).type();
}
} // namespace webrtc
diff --git a/pc/ice_server_parsing.h b/pc/ice_server_parsing.h
index da5de10..549964e 100644
--- a/pc/ice_server_parsing.h
+++ b/pc/ice_server_parsing.h
@@ -27,7 +27,12 @@
//
// Intended to be used to convert/validate the servers passed into a
// PeerConnection through RTCConfiguration.
-RTC_EXPORT RTCErrorType
+RTC_EXPORT RTCError
+ParseIceServersOrError(const PeerConnectionInterface::IceServers& servers,
+ cricket::ServerAddresses* stun_servers,
+ std::vector<cricket::RelayServerConfig>* turn_servers);
+
+[[deprecated("use ParseIceServersOrError")]] RTC_EXPORT RTCErrorType
ParseIceServers(const PeerConnectionInterface::IceServers& servers,
cricket::ServerAddresses* stun_servers,
std::vector<cricket::RelayServerConfig>* turn_servers);
diff --git a/pc/ice_server_parsing_unittest.cc b/pc/ice_server_parsing_unittest.cc
index 1cb3686..408c790 100644
--- a/pc/ice_server_parsing_unittest.cc
+++ b/pc/ice_server_parsing_unittest.cc
@@ -62,8 +62,9 @@
server.tls_cert_policy = tls_certificate_policy;
server.hostname = hostname;
servers.push_back(server);
- return webrtc::ParseIceServers(servers, &stun_servers_, &turn_servers_) ==
- webrtc::RTCErrorType::NONE;
+ return webrtc::ParseIceServersOrError(servers, &stun_servers_,
+ &turn_servers_)
+ .ok();
}
protected:
@@ -229,8 +230,9 @@
server.username = "foo";
server.password = "bar";
servers.push_back(server);
- EXPECT_EQ(webrtc::RTCErrorType::NONE,
- webrtc::ParseIceServers(servers, &stun_servers_, &turn_servers_));
+ EXPECT_TRUE(
+ webrtc::ParseIceServersOrError(servers, &stun_servers_, &turn_servers_)
+ .ok());
EXPECT_EQ(1U, stun_servers_.size());
EXPECT_EQ(1U, turn_servers_.size());
}
@@ -245,8 +247,10 @@
server.username = "foo";
server.password = "bar";
servers.push_back(server);
- EXPECT_EQ(webrtc::RTCErrorType::NONE,
- webrtc::ParseIceServers(servers, &stun_servers_, &turn_servers_));
+
+ EXPECT_TRUE(
+ webrtc::ParseIceServersOrError(servers, &stun_servers_, &turn_servers_)
+ .ok());
EXPECT_EQ(2U, turn_servers_.size());
EXPECT_NE(turn_servers_[0].priority, turn_servers_[1].priority);
}
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 02cafc5..4615ce5 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -596,10 +596,10 @@
cricket::ServerAddresses stun_servers;
std::vector<cricket::RelayServerConfig> turn_servers;
- RTCErrorType parse_error =
- ParseIceServers(configuration.servers, &stun_servers, &turn_servers);
- if (parse_error != RTCErrorType::NONE) {
- return RTCError(parse_error, "ICE server parse failed");
+ RTCError parse_error = ParseIceServersOrError(configuration.servers,
+ &stun_servers, &turn_servers);
+ if (!parse_error.ok()) {
+ return parse_error;
}
// Add the turn logging id to all turn servers
@@ -1519,10 +1519,10 @@
// Parse ICE servers before hopping to network thread.
cricket::ServerAddresses stun_servers;
std::vector<cricket::RelayServerConfig> turn_servers;
- RTCErrorType parse_error =
- ParseIceServers(configuration.servers, &stun_servers, &turn_servers);
- if (parse_error != RTCErrorType::NONE) {
- return RTCError(parse_error, "ICE server parse failed");
+ RTCError parse_error = ParseIceServersOrError(configuration.servers,
+ &stun_servers, &turn_servers);
+ if (!parse_error.ok()) {
+ return parse_error;
}
// Add the turn logging id to all turn servers
for (cricket::RelayServerConfig& turn_server : turn_servers) {
