Add additional check when setting RTCConfiguration

Check that ice_regather_interval_range is set only when continual
regathering is also set.

Bug: webrtc:7969
Change-Id: Ifcfeee744d817cf00914418d7e682f11528faf05
Reviewed-on: https://chromium-review.googlesource.com/569358
Commit-Queue: Steve Anton <steveanton@webrtc.org>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#19009}

webrtc/p2p/base/p2ptransportchannel.cc

@@ -433,6 +433,8 @@ void P2PTransportChannel::SetIceConfig(const IceConfig& config) {
   }
 
   if (config.regather_all_networks_interval_range) {
+    // Config validation is assumed to have already happened at the API layer.
+    RTC_DCHECK(config.continual_gathering_policy != GATHER_ONCE);
     config_.regather_all_networks_interval_range =
         config.regather_all_networks_interval_range;
     LOG(LS_INFO) << "Set regather_all_networks_interval_range to "

webrtc/p2p/base/p2ptransportchannel_unittest.cc

@@ -1374,8 +1374,6 @@ TEST_F(P2PTransportChannelTest, TestIceRegatherOnAllNetworksContinual) {
   config1.regather_all_networks_interval_range.emplace(
       kRegatherInterval, kRegatherInterval);
   IceConfig config2;
-  config2.regather_all_networks_interval_range.emplace(
-      kRegatherInterval, kRegatherInterval);
   CreateChannels(config1, config2);
 
   EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() &&

webrtc/pc/peerconnection.cc

@@ -216,6 +216,13 @@ bool SafeSetError(webrtc::RTCErrorType type, webrtc::RTCError* error) {
   return type == webrtc::RTCErrorType::NONE;
 }
 
+bool SafeSetError(webrtc::RTCError error, webrtc::RTCError* error_out) {
+  if (error_out) {
+    *error_out = std::move(error);
+  }
+  return error.ok();
+}
+
 }  // namespace
 
 namespace webrtc {
@@ -444,6 +451,13 @@ bool PeerConnection::Initialize(
     std::unique_ptr<rtc::RTCCertificateGeneratorInterface> cert_generator,
     PeerConnectionObserver* observer) {
   TRACE_EVENT0("webrtc", "PeerConnection::Initialize");
+
+  RTCError config_error = ValidateConfiguration(configuration);
+  if (!config_error.ok()) {
+    LOG(LS_ERROR) << "Invalid configuration: " << config_error.message();
+    return false;
+  }
+
   if (!allocator) {
     LOG(LS_ERROR) << "PeerConnection initialized without a PortAllocator? "
                   << "This shouldn't happen if using PeerConnectionFactory.";
@@ -517,6 +531,17 @@ bool PeerConnection::Initialize(
   return true;
 }
 
+RTCError PeerConnection::ValidateConfiguration(
+    const RTCConfiguration& config) const {
+  if (config.ice_regather_interval_range &&
+      config.continual_gathering_policy == GATHER_ONCE) {
+    return RTCError(RTCErrorType::INVALID_PARAMETER,
+                    "ice_regather_interval_range specified but continual "
+                    "gathering policy is GATHER_ONCE");
+  }
+  return RTCError::OK();
+}
+
 rtc::scoped_refptr<StreamCollectionInterface>
 PeerConnection::local_streams() {
   return local_streams_;
@@ -1161,6 +1186,12 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration,
     return SafeSetError(RTCErrorType::INVALID_MODIFICATION, error);
   }
 
+  // Validate the modified configuration.
+  RTCError validate_error = ValidateConfiguration(modified_config);
+  if (!validate_error.ok()) {
+    return SafeSetError(std::move(validate_error), error);
+  }
+
   // Note that this isn't possible through chromium, since it's an unsigned
   // short in WebIDL.
   if (configuration.ice_candidate_pool_size < 0 ||

webrtc/pc/peerconnection.h

@@ -394,6 +394,12 @@ class PeerConnection : public PeerConnectionInterface,
   // This function should only be called from the worker thread.
   void StopRtcEventLog_w();
 
+  // Ensures the configuration doesn't have any parameters with invalid values,
+  // or values that conflict with other parameters.
+  //
+  // Returns RTCError::OK() if there are no issues.
+  RTCError ValidateConfiguration(const RTCConfiguration& config) const;
+
   // Storing the factory as a scoped reference pointer ensures that the memory
   // in the PeerConnectionFactoryImpl remains available as long as the
   // PeerConnection is running. It is passed to PeerConnection as a raw pointer.

webrtc/pc/peerconnectioninterface_unittest.cc

@@ -797,6 +797,17 @@ class PeerConnectionInterfaceTest : public testing::Test {
     EXPECT_EQ(nullptr, pc);
   }
 
+  void CreatePeerConnectionExpectFail(
+      PeerConnectionInterface::RTCConfiguration config) {
+    PeerConnectionInterface::IceServer server;
+    server.uri = kTurnIceServerUri;
+    server.password = kTurnPassword;
+    config.servers.push_back(server);
+    rtc::scoped_refptr<PeerConnectionInterface> pc =
+        pc_factory_->CreatePeerConnection(config, nullptr, nullptr, &observer_);
+    EXPECT_EQ(nullptr, pc);
+  }
+
   void CreatePeerConnectionWithDifferentConfigurations() {
     CreatePeerConnectionWithIceServer(kStunAddressOnly, "");
     EXPECT_EQ(1u, port_allocator_->stun_servers().size());
@@ -3399,6 +3410,28 @@ TEST_F(PeerConnectionInterfaceTest, SetBitrateMaxNegativeFails) {
   EXPECT_FALSE(pc_->SetBitrate(bitrate).ok());
 }
 
+// ice_regather_interval_range requires WebRTC to be configured for continual
+// gathering already.
+TEST_F(PeerConnectionInterfaceTest,
+       SetIceRegatherIntervalRangeWithoutContinualGatheringFails) {
+  PeerConnectionInterface::RTCConfiguration config;
+  config.ice_regather_interval_range.emplace(1000, 2000);
+  config.continual_gathering_policy =
+      PeerConnectionInterface::ContinualGatheringPolicy::GATHER_ONCE;
+  CreatePeerConnectionExpectFail(config);
+}
+
+// Ensures that there is no error when ice_regather_interval_range is set with
+// continual gathering enabled.
+TEST_F(PeerConnectionInterfaceTest,
+       SetIceRegatherIntervalRangeWithContinualGathering) {
+  PeerConnectionInterface::RTCConfiguration config;
+  config.ice_regather_interval_range.emplace(1000, 2000);
+  config.continual_gathering_policy =
+      PeerConnectionInterface::ContinualGatheringPolicy::GATHER_CONTINUALLY;
+  CreatePeerConnection(config, nullptr);
+}
+
 // The current bitrate from Call's BitrateConfigMask is currently clamped by
 // Call's BitrateConfig, which comes from the SDP or a default value. This test
 // checks that a call to SetBitrate with a current bitrate that will be clamped