Revert "ice server parsing: return RTCError with more details"
This reverts commit c4b0bde7f2daabc4e0667fb73d096d1cf0819226.
Reason for revert: Breaks downstream tests.
Basically, ParseIceServers() and other functions have changed
the return type, and this breaks tests at compile time.
Is it possible to reland with backwards compatible versions that return
the previous type? Then they can be removed afterwards.
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: I4df936ff865f87759936c5d0425478fe51051dc8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/278960
Owners-Override: Mirko Bonadei <mbonadei@webrtc.org>
Auto-Submit: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#38359}
diff --git a/pc/ice_server_parsing.cc b/pc/ice_server_parsing.cc
index 16aa296..8052cd8 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`.
-RTCError ParseIceServerUrl(
+RTCErrorType ParseIceServerUrl(
const PeerConnectionInterface::IceServer& server,
absl::string_view url,
cricket::ServerAddresses* stun_servers,
@@ -186,24 +186,20 @@
std::vector<absl::string_view> transport_tokens =
rtc::split(tokens[1], '=');
if (transport_tokens[0] != kTransport) {
- LOG_AND_RETURN_ERROR(
- RTCErrorType::SYNTAX_ERROR,
- "ICE server parsing failed: Invalid transport parameter key.");
+ RTC_LOG(LS_WARNING) << "Invalid transport parameter key.";
+ return RTCErrorType::SYNTAX_ERROR;
}
if (transport_tokens.size() < 2) {
- LOG_AND_RETURN_ERROR(
- RTCErrorType::SYNTAX_ERROR,
- "ICE server parsing failed: Transport parameter missing value.");
+ RTC_LOG(LS_WARNING) << "Transport parameter missing value.";
+ return RTCErrorType::SYNTAX_ERROR;
}
absl::optional<cricket::ProtocolType> proto =
cricket::StringToProto(transport_tokens[1]);
if (!proto ||
(*proto != cricket::PROTO_UDP && *proto != cricket::PROTO_TCP)) {
- LOG_AND_RETURN_ERROR(
- RTCErrorType::SYNTAX_ERROR,
- "ICE server parsing failed: Transport parameter should "
- "always be udp or tcp.");
+ RTC_LOG(LS_WARNING) << "Transport parameter should always be udp or tcp.";
+ return RTCErrorType::SYNTAX_ERROR;
}
turn_transport_type = *proto;
}
@@ -211,10 +207,8 @@
auto [service_type, hoststring] =
GetServiceTypeAndHostnameFromUri(uri_without_transport);
if (service_type == ServiceType::INVALID) {
- 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");
+ RTC_LOG(LS_WARNING) << "Invalid transport parameter in ICE URI: " << url;
+ return RTCErrorType::SYNTAX_ERROR;
}
// GetServiceTypeAndHostnameFromUri should never give an empty hoststring
@@ -227,25 +221,22 @@
}
if (hoststring.find('@') != absl::string_view::npos) {
- 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");
+ 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;
}
auto [success, address, port] =
ParseHostnameAndPortFromString(hoststring, default_port);
if (!success) {
- RTC_LOG(LS_ERROR) << "Invalid hostname format: " << uri_without_transport;
- LOG_AND_RETURN_ERROR(RTCErrorType::SYNTAX_ERROR,
- "ICE server parsing failed: Invalid hostname format");
+ RTC_LOG(LS_WARNING) << "Invalid hostname format: " << uri_without_transport;
+ return RTCErrorType::SYNTAX_ERROR;
}
if (port <= 0 || port > 0xffff) {
- RTC_LOG(LS_ERROR) << "Invalid port: " << port;
- LOG_AND_RETURN_ERROR(RTCErrorType::SYNTAX_ERROR,
- "ICE server parsing failed: Invalid port");
+ RTC_LOG(LS_WARNING) << "Invalid port: " << port;
+ return RTCErrorType::SYNTAX_ERROR;
}
switch (service_type) {
@@ -258,10 +249,8 @@
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.
- LOG_AND_RETURN_ERROR(
- RTCErrorType::INVALID_PARAMETER,
- "ICE server parsing failed: TURN server with empty "
- "username or password");
+ RTC_LOG(LS_WARNING) << "TURN server with empty username or password";
+ return RTCErrorType::INVALID_PARAMETER;
}
// 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
@@ -274,11 +263,10 @@
if (!IPFromString(address, &ip)) {
// When hostname is set, the server address must be a
// resolved ip address.
- LOG_AND_RETURN_ERROR(
- RTCErrorType::INVALID_PARAMETER,
- "ICE server parsing failed: "
- "IceServer has hostname field set, but URI does not "
- "contain an IP address.");
+ RTC_LOG(LS_WARNING)
+ << "IceServer has hostname field set, but URI does not "
+ "contain an IP address.";
+ return RTCErrorType::INVALID_PARAMETER;
}
socket_address.SetResolvedIP(ip);
}
@@ -299,16 +287,15 @@
default:
// We shouldn't get to this point with an invalid service_type, we should
// have returned an error already.
- LOG_AND_RETURN_ERROR(
- RTCErrorType::INTERNAL_ERROR,
- "ICE server parsing failed: Unexpected service type");
+ RTC_DCHECK_NOTREACHED() << "Unexpected service type";
+ return RTCErrorType::INTERNAL_ERROR;
}
- return RTCError::OK();
+ return RTCErrorType::NONE;
}
} // namespace
-RTCError ParseIceServers(
+RTCErrorType ParseIceServers(
const PeerConnectionInterface::IceServers& servers,
cricket::ServerAddresses* stun_servers,
std::vector<cricket::RelayServerConfig>* turn_servers) {
@@ -316,26 +303,25 @@
if (!server.urls.empty()) {
for (const std::string& url : server.urls) {
if (url.empty()) {
- LOG_AND_RETURN_ERROR(RTCErrorType::SYNTAX_ERROR,
- "ICE server parsing failed: Empty uri.");
+ RTC_LOG(LS_WARNING) << "Empty uri.";
+ return RTCErrorType::SYNTAX_ERROR;
}
- RTCError err =
+ RTCErrorType err =
ParseIceServerUrl(server, url, stun_servers, turn_servers);
- if (!err.ok()) {
+ if (err != RTCErrorType::NONE) {
return err;
}
}
} else if (!server.uri.empty()) {
// Fallback to old .uri if new .urls isn't present.
- RTCError err =
+ RTCErrorType err =
ParseIceServerUrl(server, server.uri, stun_servers, turn_servers);
-
- if (!err.ok()) {
+ if (err != RTCErrorType::NONE) {
return err;
}
} else {
- LOG_AND_RETURN_ERROR(RTCErrorType::SYNTAX_ERROR,
- "ICE server parsing failed: Empty uri.");
+ RTC_LOG(LS_WARNING) << "Empty uri.";
+ return RTCErrorType::SYNTAX_ERROR;
}
}
// Candidates must have unique priorities, so that connectivity checks
@@ -345,7 +331,7 @@
// First in the list gets highest priority.
turn_server.priority = priority--;
}
- return RTCError::OK();
+ return RTCErrorType::NONE;
}
} // namespace webrtc
diff --git a/pc/ice_server_parsing.h b/pc/ice_server_parsing.h
index 50bbe2e..da5de10 100644
--- a/pc/ice_server_parsing.h
+++ b/pc/ice_server_parsing.h
@@ -27,7 +27,7 @@
//
// Intended to be used to convert/validate the servers passed into a
// PeerConnection through RTCConfiguration.
-RTC_EXPORT RTCError
+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 6e99e54..1cb3686 100644
--- a/pc/ice_server_parsing_unittest.cc
+++ b/pc/ice_server_parsing_unittest.cc
@@ -62,8 +62,8 @@
server.tls_cert_policy = tls_certificate_policy;
server.hostname = hostname;
servers.push_back(server);
- return webrtc::ParseIceServers(servers, &stun_servers_, &turn_servers_)
- .ok();
+ return webrtc::ParseIceServers(servers, &stun_servers_, &turn_servers_) ==
+ webrtc::RTCErrorType::NONE;
}
protected:
@@ -229,8 +229,8 @@
server.username = "foo";
server.password = "bar";
servers.push_back(server);
- EXPECT_TRUE(
- webrtc::ParseIceServers(servers, &stun_servers_, &turn_servers_).ok());
+ EXPECT_EQ(webrtc::RTCErrorType::NONE,
+ webrtc::ParseIceServers(servers, &stun_servers_, &turn_servers_));
EXPECT_EQ(1U, stun_servers_.size());
EXPECT_EQ(1U, turn_servers_.size());
}
@@ -245,9 +245,8 @@
server.username = "foo";
server.password = "bar";
servers.push_back(server);
-
- EXPECT_TRUE(
- webrtc::ParseIceServers(servers, &stun_servers_, &turn_servers_).ok());
+ EXPECT_EQ(webrtc::RTCErrorType::NONE,
+ webrtc::ParseIceServers(servers, &stun_servers_, &turn_servers_));
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 c201830..02cafc5 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;
- RTCError parse_error =
+ RTCErrorType parse_error =
ParseIceServers(configuration.servers, &stun_servers, &turn_servers);
- if (!parse_error.ok()) {
- return parse_error;
+ if (parse_error != RTCErrorType::NONE) {
+ return RTCError(parse_error, "ICE server parse failed");
}
// 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;
- RTCError parse_error =
+ RTCErrorType parse_error =
ParseIceServers(configuration.servers, &stun_servers, &turn_servers);
- if (!parse_error.ok()) {
- return parse_error;
+ if (parse_error != RTCErrorType::NONE) {
+ return RTCError(parse_error, "ICE server parse failed");
}
// Add the turn logging id to all turn servers
for (cricket::RelayServerConfig& turn_server : turn_servers) {
