Reland of: Adding error output param to SetConfiguration, using new RTCError type.

webrtc/api/peerconnection.cc

Index: webrtc/api/peerconnection.cc
diff --git a/webrtc/api/peerconnection.cc b/webrtc/api/peerconnection.cc
index ebce40ca5eb0cf37d03243663826ea911eda06c3..21d64cd3ada858c9429b50fc59aed583640ead57 100644
--- a/webrtc/api/peerconnection.cc
+++ b/webrtc/api/peerconnection.cc
@@ -49,6 +49,7 @@ using webrtc::DataChannel;
 using webrtc::MediaConstraintsInterface;
 using webrtc::MediaStreamInterface;
 using webrtc::PeerConnectionInterface;
+using webrtc::RtcError;
 using webrtc::RtpSenderInternal;
 using webrtc::RtpSenderInterface;
 using webrtc::RtpSenderProxy;
@@ -208,10 +209,11 @@ bool ParseHostnameAndPortFromString(const std::string& in_str,
 
 // Adds a STUN or TURN server to the appropriate list,
 // by parsing |url| and using the username/password in |server|.
-bool ParseIceServerUrl(const PeerConnectionInterface::IceServer& server,
-                       const std::string& url,
-                       cricket::ServerAddresses* stun_servers,
-                       std::vector<cricket::RelayServerConfig>* turn_servers) {
+RtcError ParseIceServerUrl(
+    const PeerConnectionInterface::IceServer& server,
+    const std::string& url,
+    cricket::ServerAddresses* stun_servers,
+    std::vector<cricket::RelayServerConfig>* turn_servers) {
   // draft-nandakumar-rtcweb-stun-uri-01
   // stunURI       = scheme ":" stun-host [ ":" stun-port ]
   // scheme        = "stun" / "stuns"
@@ -239,14 +241,14 @@ bool ParseIceServerUrl(const PeerConnectionInterface::IceServer& server,
     rtc::tokenize_with_empty_tokens(uri_transport_param, '=', &tokens);
     if (tokens[0] != kTransport) {
       LOG(LS_WARNING) << "Invalid transport parameter key.";
-      return false;
+      return RtcError::SYNTAX_ERROR;
     }
     if (tokens.size() < 2 ||
         !cricket::StringToProto(tokens[1].c_str(), &turn_transport_type) ||
         (turn_transport_type != cricket::PROTO_UDP &&
          turn_transport_type != cricket::PROTO_TCP)) {
       LOG(LS_WARNING) << "Transport param should always be udp or tcp.";
-      return false;
+      return RtcError::SYNTAX_ERROR;
     }
   }
 
@@ -256,7 +258,7 @@ bool ParseIceServerUrl(const PeerConnectionInterface::IceServer& server,
                                        &service_type,
                                        &hoststring)) {
     LOG(LS_WARNING) << "Invalid transport parameter in ICE URI: " << url;
-    return false;
+    return RtcError::SYNTAX_ERROR;
   }
 
   // GetServiceTypeAndHostnameFromUri should never give an empty hoststring
@@ -269,12 +271,12 @@ bool ParseIceServerUrl(const PeerConnectionInterface::IceServer& server,
   std::string username(server.username);
   if (tokens.size() > kTurnHostTokensNum) {
     LOG(LS_WARNING) << "Invalid user@hostname format: " << hoststring;
-    return false;
+    return RtcError::SYNTAX_ERROR;
   }
   if (tokens.size() == kTurnHostTokensNum) {
     if (tokens[0].empty() || tokens[1].empty()) {
       LOG(LS_WARNING) << "Invalid user@hostname format: " << hoststring;
-      return false;
+      return RtcError::SYNTAX_ERROR;
     }
     username.assign(rtc::s_url_decode(tokens[0]));
     hoststring = tokens[1];
@@ -291,12 +293,12 @@ bool ParseIceServerUrl(const PeerConnectionInterface::IceServer& server,
   std::string address;
   if (!ParseHostnameAndPortFromString(hoststring, &address, &port)) {
     LOG(WARNING) << "Invalid hostname format: " << uri_without_transport;
-    return false;
+    return RtcError::SYNTAX_ERROR;
   }
 
   if (port <= 0 || port > 0xffff) {
     LOG(WARNING) << "Invalid port: " << port;
-    return false;
+    return RtcError::SYNTAX_ERROR;
   }
 
   switch (service_type) {
@@ -306,16 +308,22 @@ bool ParseIceServerUrl(const PeerConnectionInterface::IceServer& server,
       break;
     case TURN:
     case TURNS: {
+      if (username.empty() || server.password.empty()) {
+        // The WebRTC spec requires throwing an InvalidAccessError when username
+        // or credential are ommitted; this is the native equivalent.
+        return RtcError::INVALID_PARAMETER;
+      }
       turn_servers->push_back(cricket::RelayServerConfig(
           address, port, username, server.password, turn_transport_type));
       break;
     }
-    case INVALID:
     default:
-      LOG(WARNING) << "Configuration not supported: " << url;
-      return false;
+      // We shouldn't get to this point with an invalid service_type, we should
+      // have returned an error already.
+      RTC_DCHECK(false) << "Unexpected service type";
+      return RtcError::INTERNAL_ERROR;
   }
-  return true;
+  return RtcError::NONE;
 }
 
 // Check if we can send |new_stream| on a PeerConnection.
@@ -427,6 +435,14 @@ void SetChannelOnSendersAndReceivers(CHANNEL* channel,
   }
 }
 
+// Helper to set an error and return from a method.
+bool SafeSetError(webrtc::RtcError error, webrtc::RtcError* error_output) {
+  if (error_output) {
+    *error_output = error;
+  }
+  return error == webrtc::RtcError::NONE;
+}
+
 }  // namespace
 
 namespace webrtc {
@@ -442,14 +458,84 @@ static const char* const kRtcErrorNames[] = {
     "NETWORK_ERROR",
     "INTERNAL_ERROR",
 };
+static_assert(static_cast<int>(RtcError::INTERNAL_ERROR) ==
+                  (arraysize(kRtcErrorNames) - 1),
+              "kRtcErrorNames must have as many strings as RtcError "
+              "has values.");
 
 std::ostream& operator<<(std::ostream& stream, RtcError error) {
   int index = static_cast<int>(error);
-  RTC_CHECK(index < static_cast<int>(sizeof(kRtcErrorNames) /
-                                     sizeof(kRtcErrorNames[0])));
   return stream << kRtcErrorNames[index];
 }
 
+bool PeerConnectionInterface::RTCConfiguration::operator==(
+    const PeerConnectionInterface::RTCConfiguration& o) const {
+  // This static_assert prevents us from accidentally breaking operator==.
+  struct stuff_being_tested_for_equality {
+    IceTransportsType type;
+    IceServers servers;
+    BundlePolicy bundle_policy;
+    RtcpMuxPolicy rtcp_mux_policy;
+    TcpCandidatePolicy tcp_candidate_policy;
+    CandidateNetworkPolicy candidate_network_policy;
+    int audio_jitter_buffer_max_packets;
+    bool audio_jitter_buffer_fast_accelerate;
+    int ice_connection_receiving_timeout;
+    int ice_backup_candidate_pair_ping_interval;
+    ContinualGatheringPolicy continual_gathering_policy;
+    std::vector<rtc::scoped_refptr<rtc::RTCCertificate>> certificates;
+    bool prioritize_most_likely_ice_candidate_pairs;
+    struct cricket::MediaConfig media_config;
+    bool disable_ipv6;
+    bool enable_rtp_data_channel;
+    bool enable_quic;
+    rtc::Optional<int> screencast_min_bitrate;
+    rtc::Optional<bool> combined_audio_video_bwe;
+    rtc::Optional<bool> enable_dtls_srtp;
+    int ice_candidate_pool_size;
+    bool prune_turn_ports;
+    bool presume_writable_when_fully_relayed;
+    bool enable_ice_renomination;
+    bool redetermine_role_on_ice_restart;
+  };
+  static_assert(sizeof(stuff_being_tested_for_equality) == sizeof(*this),
+                "Did you add something to RTCConfiguration and forget to "
+                "update operator==?");
+  return type == o.type && servers == o.servers &&
+         bundle_policy == o.bundle_policy &&
+         rtcp_mux_policy == o.rtcp_mux_policy &&
+         tcp_candidate_policy == o.tcp_candidate_policy &&
+         candidate_network_policy == o.candidate_network_policy &&
+         audio_jitter_buffer_max_packets == o.audio_jitter_buffer_max_packets &&
+         audio_jitter_buffer_fast_accelerate ==
+             o.audio_jitter_buffer_fast_accelerate &&
+         ice_connection_receiving_timeout ==
+             o.ice_connection_receiving_timeout &&
+         ice_backup_candidate_pair_ping_interval ==
+             o.ice_backup_candidate_pair_ping_interval &&
+         continual_gathering_policy == o.continual_gathering_policy &&
+         certificates == o.certificates &&
+         prioritize_most_likely_ice_candidate_pairs ==
+             o.prioritize_most_likely_ice_candidate_pairs &&
+         media_config == o.media_config && disable_ipv6 == o.disable_ipv6 &&
+         enable_rtp_data_channel == o.enable_rtp_data_channel &&
+         enable_quic == o.enable_quic &&
+         screencast_min_bitrate == o.screencast_min_bitrate &&
+         combined_audio_video_bwe == o.combined_audio_video_bwe &&
+         enable_dtls_srtp == o.enable_dtls_srtp &&
+         ice_candidate_pool_size == o.ice_candidate_pool_size &&
+         prune_turn_ports == o.prune_turn_ports &&
+         presume_writable_when_fully_relayed ==
+             o.presume_writable_when_fully_relayed &&
+         enable_ice_renomination == o.enable_ice_renomination &&
+         redetermine_role_on_ice_restart == o.redetermine_role_on_ice_restart;
+}
+
+bool PeerConnectionInterface::RTCConfiguration::operator!=(
+    const PeerConnectionInterface::RTCConfiguration& o) const {
+  return !(*this == o);
+}
+
 // Generate a RTCP CNAME when a PeerConnection is created.
 std::string GenerateRtcpCname() {
   std::string cname;
@@ -549,28 +635,33 @@ bool ParseConstraintsForAnswer(const MediaConstraintsInterface* constraints,
   return mandatory_constraints_satisfied == constraints->GetMandatory().size();
 }
 
-bool ParseIceServers(const PeerConnectionInterface::IceServers& servers,
-                     cricket::ServerAddresses* stun_servers,
-                     std::vector<cricket::RelayServerConfig>* turn_servers) {
+RtcError ParseIceServers(
+    const PeerConnectionInterface::IceServers& servers,
+    cricket::ServerAddresses* stun_servers,
+    std::vector<cricket::RelayServerConfig>* turn_servers) {
   for (const webrtc::PeerConnectionInterface::IceServer& server : servers) {
     if (!server.urls.empty()) {
       for (const std::string& url : server.urls) {
         if (url.empty()) {
           LOG(LS_ERROR) << "Empty uri.";
-          return false;
+          return RtcError::SYNTAX_ERROR;
         }
-        if (!ParseIceServerUrl(server, url, stun_servers, turn_servers)) {
-          return false;
+        RtcError err =
+            ParseIceServerUrl(server, url, stun_servers, turn_servers);
+        if (err != RtcError::NONE) {
+          return err;
         }
       }
     } else if (!server.uri.empty()) {
       // Fallback to old .uri if new .urls isn't present.
-      if (!ParseIceServerUrl(server, server.uri, stun_servers, turn_servers)) {
-        return false;
+      RtcError err =
+          ParseIceServerUrl(server, server.uri, stun_servers, turn_servers);
+      if (err != RtcError::NONE) {
+        return err;
       }
     } else {
       LOG(LS_ERROR) << "Empty uri.";
-      return false;
+      return RtcError::SYNTAX_ERROR;
     }
   }
   // Candidates must have unique priorities, so that connectivity checks
@@ -580,7 +671,7 @@ bool ParseIceServers(const PeerConnectionInterface::IceServers& servers,
     // First in the list gets highest priority.
     turn_server.priority = priority--;
   }
-  return true;
+  return RtcError::NONE;
 }
 
 PeerConnection::PeerConnection(PeerConnectionFactory* factory)
@@ -623,12 +714,18 @@ bool PeerConnection::Initialize(
     std::unique_ptr<rtc::RTCCertificateGeneratorInterface> cert_generator,
     PeerConnectionObserver* observer) {
   TRACE_EVENT0("webrtc", "PeerConnection::Initialize");
-  RTC_DCHECK(observer != nullptr);
+  if (!allocator) {
+    LOG(LS_ERROR) << "PeerConnection initialized without a PortAllocator? "
+                  << "This shouldn't happen if using PeerConnectionFactory.";
+    return false;
+  }
   if (!observer) {
+    // TODO(deadbeef): Why do we do this?
+    LOG(LS_ERROR) << "PeerConnection initialized without a "
+                  << "PeerConnectionObserver";
     return false;
   }
   observer_ = observer;
-
   port_allocator_ = std::move(allocator);
 
   // The port allocator lives on the network thread and should be initialized
@@ -1284,7 +1381,8 @@ PeerConnectionInterface::RTCConfiguration PeerConnection::GetConfiguration() {
   return configuration_;
 }
 
-bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration) {
+bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration,
+                                      RtcError* error) {
   TRACE_EVENT0("webrtc", "PeerConnection::SetConfiguration");
 
   if (session_->local_description() &&
@@ -1292,32 +1390,61 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration) {
           configuration_.ice_candidate_pool_size) {
     LOG(LS_ERROR) << "Can't change candidate pool size after calling "
                      "SetLocalDescription.";
-    return false;
+    return SafeSetError(RtcError::INVALID_MODIFICATION, error);
   }
-  // TODO(deadbeef): Return false and log an error if there are any unsupported
-  // modifications.
-  if (port_allocator_) {
-    if (!network_thread()->Invoke<bool>(
-            RTC_FROM_HERE,
-            rtc::Bind(&PeerConnection::ReconfigurePortAllocator_n, this,
-                      configuration))) {
-      LOG(LS_ERROR) << "Failed to apply configuration to PortAllocator.";
-      return false;
-    }
+
+  // The simplest (and most future-compatible) way to tell if the config was
+  // modified in an invalid way is to copy each property we do support
+  // modifying, then use operator==. There are far more properties we don't
+  // support modifying than those we do, and more could be added.

[pthatcher1, 2016/12/21 00:57:24]

That's a good approach.  I like it.

+  RTCConfiguration modified_config = configuration_;
+  modified_config.servers = configuration.servers;
+  modified_config.type = configuration.type;
+  modified_config.ice_candidate_pool_size =
+      configuration.ice_candidate_pool_size;
+  modified_config.prune_turn_ports = configuration.prune_turn_ports;
+  if (configuration != modified_config) {
+    LOG(LS_ERROR) << "Modifying the configuration in an unsupported way.";
+    return SafeSetError(RtcError::INVALID_MODIFICATION, error);
+  }
+
+  // Note that this isn't possible through chromium, since it's an unsigned
+  // short in WebIDL.
+  if (configuration.ice_candidate_pool_size < 0 ||
+      configuration.ice_candidate_pool_size > UINT16_MAX) {
+    return SafeSetError(RtcError::INVALID_RANGE, error);
+  }
+
+  // Parse ICE servers before hopping to network thread.
+  cricket::ServerAddresses stun_servers;
+  std::vector<cricket::RelayServerConfig> turn_servers;
+  RtcError parse_error =
+      ParseIceServers(configuration.servers, &stun_servers, &turn_servers);
+  if (parse_error != RtcError::NONE) {
+    return SafeSetError(parse_error, error);
   }
 
-  // TODO(deadbeef): Shouldn't have to hop to the network thread twice...
-  session_->SetIceConfig(session_->ParseIceConfig(configuration));
+  // In theory this shouldn't fail.
+  if (!network_thread()->Invoke<bool>(
+          RTC_FROM_HERE,
+          rtc::Bind(&PeerConnection::ReconfigurePortAllocator_n, this,
+                    stun_servers, turn_servers, modified_config.type,
+                    modified_config.ice_candidate_pool_size,
+                    modified_config.prune_turn_ports))) {
+    LOG(LS_ERROR) << "Failed to apply configuration to PortAllocator.";
+    return SafeSetError(RtcError::INTERNAL_ERROR, error);
+  }
 
   // As described in JSEP, calling setConfiguration with new ICE servers or
   // candidate policy must set a "needs-ice-restart" bit so that the next offer
   // triggers an ICE restart which will pick up the changes.
-  if (configuration.servers != configuration_.servers ||
-      configuration.type != configuration_.type) {
+  if (modified_config.servers != configuration_.servers ||
+      modified_config.type != configuration_.type ||
+      modified_config.prune_turn_ports != configuration_.prune_turn_ports) {
     session_->SetNeedsIceRestartFlag();
   }
-  configuration_ = configuration;
-  return true;
+  configuration_ = modified_config;
+  return SafeSetError(RtcError::NONE, error);
 }
 
 bool PeerConnection::AddIceCandidate(
@@ -1344,7 +1471,7 @@ void PeerConnection::RegisterUMAObserver(UMAObserver* observer) {
   }
 
   // Send information about IPv4/IPv6 status.
-  if (uma_observer_ && port_allocator_) {
+  if (uma_observer_) {
     port_allocator_->SetMetricsObserver(uma_observer_);
     if (port_allocator_->flags() & cricket::PORTALLOCATOR_ENABLE_IPV6) {
       uma_observer_->IncrementEnumCounter(
@@ -2337,7 +2464,8 @@ bool PeerConnection::InitializePortAllocator_n(
     const RTCConfiguration& configuration) {
   cricket::ServerAddresses stun_servers;
   std::vector<cricket::RelayServerConfig> turn_servers;
-  if (!ParseIceServers(configuration.servers, &stun_servers, &turn_servers)) {
+  if (ParseIceServers(configuration.servers, &stun_servers, &turn_servers) !=
+      RtcError::NONE) {
     return false;
   }
 
@@ -2383,19 +2511,17 @@ bool PeerConnection::InitializePortAllocator_n(
 }
 
 bool PeerConnection::ReconfigurePortAllocator_n(
-    const RTCConfiguration& configuration) {
-  cricket::ServerAddresses stun_servers;
-  std::vector<cricket::RelayServerConfig> turn_servers;
-  if (!ParseIceServers(configuration.servers, &stun_servers, &turn_servers)) {
-    return false;
-  }
+    const cricket::ServerAddresses& stun_servers,
+    const std::vector<cricket::RelayServerConfig>& turn_servers,
+    IceTransportsType type,
+    int candidate_pool_size,
+    bool prune_turn_ports) {
   port_allocator_->set_candidate_filter(
-      ConvertIceTransportTypeToCandidateFilter(configuration.type));
+      ConvertIceTransportTypeToCandidateFilter(type));
   // Call this last since it may create pooled allocator sessions using the
   // candidate filter set above.
   return port_allocator_->SetConfiguration(
-      stun_servers, turn_servers, configuration.ice_candidate_pool_size,
-      configuration.prune_turn_ports);
+      stun_servers, turn_servers, candidate_pool_size, prune_turn_ports);
 }
 
 bool PeerConnection::StartRtcEventLog_w(rtc::PlatformFile file,