From c2429a080d74337c122271cf9e205e3d321d8a68 Mon Sep 17 00:00:00 2001
From: Tommi <tommi@webrtc.org>
Date: Fri, 3 Mar 2023 11:28:23 +0100
Subject: [PATCH] Simplify SctpDataChannel construction a bit.

This moves SctpDataChannel construction a step closer to RAII by moving the error checks out of SctpDataChannel::Init() and not construct an SctpDataChannel instance unless error checks have been done first in SctpDataChannel::Create.

Ideally the Init() method shouldn't be needed but there is test code that constructs an SctpDataChannel instance without running the Init()
steps but they're required by the SctpDataChannel::Create() path.

Bug: none
Change-Id: I8498693063c28355f901d27c4fe7bd45b7d4be26
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/295860
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39467}
---
 pc/sctp_data_channel.cc | 53 ++++++++++++++++++++++++-----------------
 pc/sctp_data_channel.h  |  7 +++++-
 2 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/pc/sctp_data_channel.cc b/pc/sctp_data_channel.cc
index 0c66e77f59..cb7c1c31d9 100644
--- a/pc/sctp_data_channel.cc
+++ b/pc/sctp_data_channel.cc
@@ -101,6 +101,23 @@ InternalDataChannelInit::InternalDataChannelInit(const DataChannelInit& base)
   }
 }
 
+bool InternalDataChannelInit::IsValid() const {
+  if (id < -1)
+    return false;
+
+  if (maxRetransmits.has_value() && maxRetransmits.value() < 0)
+    return false;
+
+  if (maxRetransmitTime.has_value() && maxRetransmitTime.value() < 0)
+    return false;
+
+  // Only one of these can be set.
+  if (maxRetransmits.has_value() && maxRetransmitTime.has_value())
+    return false;
+
+  return true;
+}
+
 bool SctpSidAllocator::AllocateSid(rtc::SSLRole role, int* sid) {
   int potential_sid = (role == rtc::SSL_CLIENT) ? 0 : 1;
   while (!IsSidAvailable(potential_sid)) {
@@ -145,11 +162,17 @@ rtc::scoped_refptr<SctpDataChannel> SctpDataChannel::Create(
     const InternalDataChannelInit& config,
     rtc::Thread* signaling_thread,
     rtc::Thread* network_thread) {
-  auto channel = rtc::make_ref_counted<SctpDataChannel>(
-      config, std::move(controller), label, signaling_thread, network_thread);
-  if (!channel->Init()) {
+  RTC_DCHECK(controller);
+
+  if (!config.IsValid()) {
+    RTC_LOG(LS_ERROR) << "Failed to initialize the SCTP data channel due to "
+                         "invalid DataChannelInit.";
     return nullptr;
   }
+
+  auto channel = rtc::make_ref_counted<SctpDataChannel>(
+      config, std::move(controller), label, signaling_thread, network_thread);
+  channel->Init();
   return channel;
 }
 
@@ -176,22 +199,7 @@ SctpDataChannel::SctpDataChannel(
       controller_(std::move(controller)) {
   RTC_DCHECK_RUN_ON(signaling_thread_);
   RTC_UNUSED(network_thread_);
-}
-
-bool SctpDataChannel::Init() {
-  RTC_DCHECK_RUN_ON(signaling_thread_);
-  if (config_.id < -1 ||
-      (config_.maxRetransmits && *config_.maxRetransmits < 0) ||
-      (config_.maxRetransmitTime && *config_.maxRetransmitTime < 0)) {
-    RTC_LOG(LS_ERROR) << "Failed to initialize the SCTP data channel due to "
-                         "invalid DataChannelInit.";
-    return false;
-  }
-  if (config_.maxRetransmits && config_.maxRetransmitTime) {
-    RTC_LOG(LS_ERROR)
-        << "maxRetransmits and maxRetransmitTime should not be both set.";
-    return false;
-  }
+  RTC_DCHECK(config_.IsValid());
 
   switch (config_.open_handshake_role) {
     case webrtc::InternalDataChannelInit::kNone:  // pre-negotiated
@@ -204,6 +212,10 @@ bool SctpDataChannel::Init() {
       handshake_state_ = kHandshakeShouldSendAck;
       break;
   }
+}
+
+void SctpDataChannel::Init() {
+  RTC_DCHECK_RUN_ON(signaling_thread_);
 
   // Try to connect to the transport in case the transport channel already
   // exists.
@@ -214,7 +226,6 @@ bool SctpDataChannel::Init() {
   // This has to be done async because the upper layer objects (e.g.
   // Chrome glue and WebKit) are not wired up properly until after this
   // function returns.
-  RTC_DCHECK(controller_);
   if (controller_->ReadyToSendData()) {
     AddRef();
     absl::Cleanup release = [this] { Release(); };
@@ -224,8 +235,6 @@ bool SctpDataChannel::Init() {
         OnTransportReady(true);
     });
   }
-
-  return true;
 }
 
 SctpDataChannel::~SctpDataChannel() {
diff --git a/pc/sctp_data_channel.h b/pc/sctp_data_channel.h
index f42818f8cc..91daaf7e4e 100644
--- a/pc/sctp_data_channel.h
+++ b/pc/sctp_data_channel.h
@@ -71,6 +71,11 @@ struct InternalDataChannelInit : public DataChannelInit {
   // The default role is kOpener because the default `negotiated` is false.
   InternalDataChannelInit() : open_handshake_role(kOpener) {}
   explicit InternalDataChannelInit(const DataChannelInit& base);
+
+  // Does basic validation to determine if a data channel instance can be
+  // constructed using the configuration.
+  bool IsValid() const;
+
   OpenHandshakeRole open_handshake_role;
 };
 
@@ -237,7 +242,7 @@ class SctpDataChannel : public DataChannelInterface,
     kHandshakeReady
   };
 
-  bool Init();
+  void Init();
   void UpdateState();
   void SetState(DataState state);
   void DisconnectFromTransport();