Add sanity checks of IceConfig parameters.

IceConfig contains a set of parameters that affect the behavior of ICE.
Inconsistent or conflicting parameters lead to erroneous or
unpredicatble behavior in the network stack. Sanity checks are now added
to validate IceConfig.

TBR=magjed@webrtc.org

Bug: webrtc:8993
Change-Id: I708bc3f1ef970872754a82a47a509bda15061ca6
Reviewed-on: https://webrtc-review.googlesource.com/60847
Commit-Queue: Qingsi Wang <qingsi@google.com>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Reviewed-by: Peter Thatcher <pthatcher@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22664}

p2p/base/p2ptransportchannel.cc

@@ -91,10 +91,23 @@ int CompareCandidatePairsByNetworkPreference(
   return a_and_b_equal;
 }
 
+uint32_t GetWeakPingIntervalInFieldTrial() {
+  uint32_t weak_ping_interval = ::strtoul(
+      webrtc::field_trial::FindFullName("WebRTC-StunInterPacketDelay").c_str(),
+      nullptr, 10);
+  if (weak_ping_interval) {
+    return static_cast<int>(weak_ping_interval);
+  }
+  return cricket::WEAK_PING_INTERVAL;
+}
+
 }  // unnamed namespace
 
 namespace cricket {
 
+using webrtc::RTCErrorType;
+using webrtc::RTCError;
+
 bool IceCredentialsChanged(const std::string& old_ufrag,
                            const std::string& old_pwd,
                            const std::string& new_ufrag,
@@ -130,12 +143,10 @@ P2PTransportChannel::P2PTransportChannel(const std::string& transport_name,
               true /* presume_writable_when_fully_relayed */,
               REGATHER_ON_FAILED_NETWORKS_INTERVAL,
               RECEIVING_SWITCHING_DELAY) {
-  uint32_t weak_ping_interval = ::strtoul(
-      webrtc::field_trial::FindFullName("WebRTC-StunInterPacketDelay").c_str(),
-      nullptr, 10);
-  if (weak_ping_interval) {
-    weak_ping_interval_ = static_cast<int>(weak_ping_interval);
-  }
+  weak_ping_interval_ = GetWeakPingIntervalInFieldTrial();
+  // Validate IceConfig even for mostly built-in constant default values in case
+  // we change them.
+  RTC_DCHECK(ValidateIceConfig(config_).ok());
   ice_event_log_.set_event_log(event_log);
 }
 
@@ -562,12 +573,64 @@ void P2PTransportChannel::SetIceConfig(const IceConfig& config) {
     RTC_LOG(LS_INFO) << "Set STUN keepalive interval to "
                      << config.stun_keepalive_interval_or_default();
   }
+
+  RTC_DCHECK(ValidateIceConfig(config_).ok());
 }
 
 const IceConfig& P2PTransportChannel::config() const {
   return config_;
 }
 
+RTCError P2PTransportChannel::ValidateIceConfig(const IceConfig& config) {
+  if (config.regather_all_networks_interval_range &&
+      config.continual_gathering_policy == GATHER_ONCE) {
+    return RTCError(RTCErrorType::INVALID_PARAMETER,
+                    "regather_all_networks_interval_range specified but "
+                    "continual gathering policy is GATHER_ONCE");
+  }
+
+  if (config.ice_check_interval_strong_connectivity_or_default() <
+      config.ice_check_interval_weak_connectivity.value_or(
+          GetWeakPingIntervalInFieldTrial())) {
+    return RTCError(RTCErrorType::INVALID_PARAMETER,
+                    "Ping interval of candidate pairs is shorter when ICE is "
+                    "strongly connected than that when ICE is weakly "
+                    "connected");
+  }
+
+  if (config.receiving_timeout_or_default() <
+      std::max(config.ice_check_interval_strong_connectivity_or_default(),
+               config.ice_check_min_interval_or_default())) {
+    return RTCError(
+        RTCErrorType::INVALID_PARAMETER,
+        "Receiving timeout is shorter than the minimal ping interval.");
+  }
+
+  if (config.backup_connection_ping_interval_or_default() <
+      config.ice_check_interval_strong_connectivity_or_default()) {
+    return RTCError(RTCErrorType::INVALID_PARAMETER,
+                    "Ping interval of backup candidate pairs is shorter than "
+                    "that of general candidate pairs when ICE is strongly "
+                    "connected");
+  }
+
+  if (config.stable_writable_connection_ping_interval_or_default() <
+      config.ice_check_interval_strong_connectivity_or_default()) {
+    return RTCError(RTCErrorType::INVALID_PARAMETER,
+                    "Ping interval of stable and writable candidate pairs is "
+                    "shorter than that of general candidate pairs when ICE is "
+                    "strongly connected");
+  }
+
+  if (config.ice_unwritable_timeout_or_default() > CONNECTION_WRITE_TIMEOUT) {
+    return RTCError(RTCErrorType::INVALID_PARAMETER,
+                    "The timeout period for the writability state to become "
+                    "UNRELIABLE is longer than that to become TIMEOUT.");
+  }
+
+  return RTCError::OK();
+}
+
 int P2PTransportChannel::check_receiving_interval() const {
   return std::max(MIN_CHECK_RECEIVING_INTERVAL,
                   config_.receiving_timeout_or_default() / 10);

p2p/base/p2ptransportchannel.h

@@ -28,6 +28,7 @@
 #include <vector>
 
 #include "api/candidate.h"
+#include "api/rtcerror.h"
 #include "logging/rtc_event_log/events/rtc_event_ice_candidate_pair_config.h"
 #include "logging/rtc_event_log/icelogger.h"
 #include "p2p/base/candidatepairinterface.h"
@@ -106,6 +107,7 @@ class P2PTransportChannel : public IceTransportInternal,
   // TODO(deadbeef): Use rtc::Optional instead of negative values.
   void SetIceConfig(const IceConfig& config) override;
   const IceConfig& config() const;
+  static webrtc::RTCError ValidateIceConfig(const IceConfig& config);
   void SetMetricsObserver(webrtc::MetricsObserverInterface* observer) override;
 
   // From TransportChannel:

p2p/base/p2ptransportchannel_unittest.cc

@@ -4423,7 +4423,7 @@ class P2PTransportChannelMostLikelyToWorkFirstTest
 // we have a selected connection.
 TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest,
        TestRelayRelayFirstWhenNothingPingedYet) {
-  const int max_strong_interval = 100;
+  const int max_strong_interval = 500;
   P2PTransportChannel& ch = StartTransportChannel(true, max_strong_interval);
   EXPECT_TRUE_WAIT(ch.ports().size() == 2, kDefaultTimeout);
   EXPECT_EQ(ch.ports()[0]->Type(), LOCAL_PORT_TYPE);
@@ -4480,7 +4480,7 @@ TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest,
 // in the first round.
 TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest,
        TestRelayRelayFirstWhenEverythingPinged) {
-  P2PTransportChannel& ch = StartTransportChannel(true, 100);
+  P2PTransportChannel& ch = StartTransportChannel(true, 500);
   EXPECT_TRUE_WAIT(ch.ports().size() == 2, kDefaultTimeout);
   EXPECT_EQ(ch.ports()[0]->Type(), LOCAL_PORT_TYPE);
   EXPECT_EQ(ch.ports()[1]->Type(), RELAY_PORT_TYPE);
@@ -4511,7 +4511,7 @@ TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest,
 // before we re-ping Relay/Relay connections again.
 TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest,
        TestNoStarvationOnNonRelayConnection) {
-  P2PTransportChannel& ch = StartTransportChannel(true, 100);
+  P2PTransportChannel& ch = StartTransportChannel(true, 500);
   EXPECT_TRUE_WAIT(ch.ports().size() == 2, kDefaultTimeout);
   EXPECT_EQ(ch.ports()[0]->Type(), LOCAL_PORT_TYPE);
   EXPECT_EQ(ch.ports()[1]->Type(), RELAY_PORT_TYPE);
@@ -4550,7 +4550,7 @@ TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest, TestTcpTurn) {
   config.ports.push_back(ProtocolAddress(kTurnTcpIntAddr, PROTO_TCP));
   allocator()->AddTurnServer(config);
 
-  P2PTransportChannel& ch = StartTransportChannel(true, 100);
+  P2PTransportChannel& ch = StartTransportChannel(true, 500);
   EXPECT_TRUE_WAIT(ch.ports().size() == 3, kDefaultTimeout);
   EXPECT_EQ(ch.ports()[0]->Type(), LOCAL_PORT_TYPE);
   EXPECT_EQ(ch.ports()[1]->Type(), RELAY_PORT_TYPE);

p2p/base/port.cc

@@ -147,6 +147,9 @@ const int64_t kForgetPacketAfter = 30000;  // 30 seconds
 
 namespace cricket {
 
+using webrtc::RTCErrorType;
+using webrtc::RTCError;
+
 // TODO(ronghuawu): Use "local", "srflx", "prflx" and "relay". But this requires
 // the signaling part be updated correspondingly as well.
 const char LOCAL_PORT_TYPE[] = "local";

p2p/base/port.h

@@ -19,6 +19,7 @@
 
 #include "api/candidate.h"
 #include "api/optional.h"
+#include "api/rtcerror.h"
 #include "logging/rtc_event_log/events/rtc_event_ice_candidate_pair.h"
 #include "logging/rtc_event_log/events/rtc_event_ice_candidate_pair_config.h"
 #include "logging/rtc_event_log/icelogger.h"
@@ -552,9 +553,6 @@ class Connection : public CandidatePairInterface,
 
   int unwritable_timeout() const;
   void set_unwritable_timeout(const rtc::Optional<int>& value_ms) {
-    // TODO(qingsi): Validate configuration parameters in
-    // PeerConnection::ValidateConfiguration.
-    RTC_CHECK_LT(value_ms.value_or(-1), CONNECTION_WRITE_TIMEOUT);
     unwritable_timeout_ = value_ms;
   }
   int unwritable_min_checks() const;

pc/peerconnection.cc

@@ -1028,7 +1028,9 @@ RTCError PeerConnection::ValidateConfiguration(
                     "ice_regather_interval_range specified but continual "
                     "gathering policy is GATHER_ONCE");
   }
-  return RTCError::OK();
+  auto result =
+      cricket::P2PTransportChannel::ValidateIceConfig(ParseIceConfig(config));
+  return result;
 }
 
 rtc::scoped_refptr<StreamCollectionInterface>

sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm

@@ -40,8 +40,8 @@
   config.tcpCandidatePolicy = RTCTcpCandidatePolicyDisabled;
   config.candidateNetworkPolicy = RTCCandidateNetworkPolicyLowCost;
   const int maxPackets = 60;
-  const int timeout = 1;
-  const int interval = 2;
+  const int timeout = 1500;
+  const int interval = 2000;
   config.audioJitterBufferMaxPackets = maxPackets;
   config.audioJitterBufferFastAccelerate = YES;
   config.iceConnectionReceivingTimeout = timeout;