Switch from pointer to ID for OnSctpDataChannelStateChanged

* The pointer isn't needed for this notification. Arguably using
  the internal id is more consistent with the stats code.
* Using the int makes it safer down the line to post the operation
  from the network thread to the signaling thread rather than post
  an object reference.

Bug: none
Change-Id: I1e9eb31d8386dca3feaa90ee3267ea98eb3e81ec
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/299144
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39696}

pc/data_channel_controller.cc

@@ -69,7 +69,7 @@ void DataChannelController::OnChannelStateChanged(
   if (state == DataChannelInterface::DataState::kClosed)
     OnSctpDataChannelClosed(channel);
 
-  pc_->OnSctpDataChannelStateChanged(channel, state);
+  pc_->OnSctpDataChannelStateChanged(channel->internal_id(), state);
 }
 
 void DataChannelController::OnDataReceived(

pc/peer_connection.cc

@@ -2115,11 +2115,11 @@ void PeerConnection::ResetSctpDataMid() {
 }
 
 void PeerConnection::OnSctpDataChannelStateChanged(
-    DataChannelInterface* channel,
+    int channel_id,
     DataChannelInterface::DataState state) {
   RTC_DCHECK_RUN_ON(signaling_thread());
   if (stats_collector_)
-    stats_collector_->OnSctpDataChannelStateChanged(channel, state);
+    stats_collector_->OnSctpDataChannelStateChanged(channel_id, state);
 }
 
 PeerConnection::InitializePortAllocatorResult

pc/peer_connection.h

@@ -317,7 +317,7 @@ class PeerConnection : public PeerConnectionInternal,
   absl::optional<rtc::SSLRole> GetSctpSslRole_n() override;
 
   void OnSctpDataChannelStateChanged(
-      DataChannelInterface* channel,
+      int channel_id,
       DataChannelInterface::DataState state) override;
 
   bool ShouldFireNegotiationNeededEvent(uint32_t event_id) override;

pc/peer_connection_internal.h

@@ -177,8 +177,11 @@ class PeerConnectionInternal : public PeerConnectionInterface,
   // Functions needed by DataChannelController
   virtual void NoteDataAddedEvent() {}
   // Handler for sctp data channel state changes.
+  // The `channel_id` is the same unique identifier as used in
+  // `DataChannelStats::internal_id and
+  // `RTCDataChannelStats::data_channel_identifier`.
   virtual void OnSctpDataChannelStateChanged(
-      DataChannelInterface* channel,
+      int channel_id,
       DataChannelInterface::DataState state) {}
 };
 

pc/rtc_stats_collector.cc

@@ -2493,17 +2493,18 @@ void RTCStatsCollector::PrepareTransceiverStatsInfosAndCallStats_s_w_n() {
 }
 
 void RTCStatsCollector::OnSctpDataChannelStateChanged(
-    DataChannelInterface* channel,
+    int channel_id,
     DataChannelInterface::DataState state) {
   RTC_DCHECK_RUN_ON(signaling_thread_);
   if (state == DataChannelInterface::DataState::kOpen) {
-    bool result = internal_record_.opened_data_channels.insert(channel).second;
+    bool result =
+        internal_record_.opened_data_channels.insert(channel_id).second;
     RTC_DCHECK(result);
     ++internal_record_.data_channels_opened;
   } else if (state == DataChannelInterface::DataState::kClosed) {
     // Only channels that have been fully opened (and have increased the
     // `data_channels_opened_` counter) increase the closed counter.
-    if (internal_record_.opened_data_channels.erase(channel)) {
+    if (internal_record_.opened_data_channels.erase(channel_id)) {
       ++internal_record_.data_channels_closed;
     }
   }

pc/rtc_stats_collector.h

@@ -91,7 +91,7 @@ class RTCStatsCollector : public rtc::RefCountInterface {
   void WaitForPendingRequest();
 
   // Called by the PeerConnection instance when data channel states change.
-  void OnSctpDataChannelStateChanged(DataChannelInterface* channel,
+  void OnSctpDataChannelStateChanged(int channel_id,
                                      DataChannelInterface::DataState state);
 
  protected:
@@ -321,12 +321,9 @@ class RTCStatsCollector : public rtc::RefCountInterface {
     // before reaching the open state does not affect these counters.
     uint32_t data_channels_opened;
     uint32_t data_channels_closed;
-    // Identifies by address channels that have been opened, which remain in the
-    // set until they have been fully closed.
-    // NOTE: There is no reference held here or any other type of guarantee that
-    // these pointers remain valid. So they MUST NOT be followed; hence, void*
-    // is used and not the explicit type.
-    webrtc::flat_set<void*> opened_data_channels;
+    // Identifies channels that have been opened, whose internal id is stored in
+    // the set until they have been fully closed.
+    webrtc::flat_set<int> opened_data_channels;
   };
   InternalRecord internal_record_;
 };

pc/rtc_stats_collector_unittest.cc

@@ -2132,10 +2132,10 @@ TEST_F(RTCStatsCollectorTest, CollectRTCPeerConnectionStats) {
       rtc::Thread::Current(), rtc::Thread::Current());
 
   stats_->stats_collector()->OnSctpDataChannelStateChanged(
-      dummy_channel_a.get(), DataChannelInterface::DataState::kOpen);
+      dummy_channel_a->internal_id(), DataChannelInterface::DataState::kOpen);
   // Closing a channel that is not opened should not affect the counts.
   stats_->stats_collector()->OnSctpDataChannelStateChanged(
-      dummy_channel_b.get(), DataChannelInterface::DataState::kClosed);
+      dummy_channel_b->internal_id(), DataChannelInterface::DataState::kClosed);
 
   {
     rtc::scoped_refptr<const RTCStatsReport> report =
@@ -2148,9 +2148,9 @@ TEST_F(RTCStatsCollectorTest, CollectRTCPeerConnectionStats) {
   }
 
   stats_->stats_collector()->OnSctpDataChannelStateChanged(
-      dummy_channel_b.get(), DataChannelInterface::DataState::kOpen);
+      dummy_channel_b->internal_id(), DataChannelInterface::DataState::kOpen);
   stats_->stats_collector()->OnSctpDataChannelStateChanged(
-      dummy_channel_b.get(), DataChannelInterface::DataState::kClosed);
+      dummy_channel_b->internal_id(), DataChannelInterface::DataState::kClosed);
 
   {
     rtc::scoped_refptr<const RTCStatsReport> report =
@@ -2165,7 +2165,7 @@ TEST_F(RTCStatsCollectorTest, CollectRTCPeerConnectionStats) {
   // Re-opening a data channel (or opening a new data channel that is re-using
   // the same address in memory) should increase the opened count.
   stats_->stats_collector()->OnSctpDataChannelStateChanged(
-      dummy_channel_b.get(), DataChannelInterface::DataState::kOpen);
+      dummy_channel_b->internal_id(), DataChannelInterface::DataState::kOpen);
 
   {
     rtc::scoped_refptr<const RTCStatsReport> report =
@@ -2178,9 +2178,9 @@ TEST_F(RTCStatsCollectorTest, CollectRTCPeerConnectionStats) {
   }
 
   stats_->stats_collector()->OnSctpDataChannelStateChanged(
-      dummy_channel_a.get(), DataChannelInterface::DataState::kClosed);
+      dummy_channel_a->internal_id(), DataChannelInterface::DataState::kClosed);
   stats_->stats_collector()->OnSctpDataChannelStateChanged(
-      dummy_channel_b.get(), DataChannelInterface::DataState::kClosed);
+      dummy_channel_b->internal_id(), DataChannelInterface::DataState::kClosed);
 
   {
     rtc::scoped_refptr<const RTCStatsReport> report =

pc/sctp_data_channel.h

@@ -211,6 +211,11 @@ class SctpDataChannel : public DataChannelInterface {
 
   DataChannelStats GetStats() const;
 
+  // Returns a unique identifier that's guaranteed to always be available,
+  // doesn't change throughout SctpDataChannel's lifetime and is used for
+  // stats purposes (see also `GetStats()`).
+  int internal_id() const { return internal_id_; }
+
   const StreamId& sid() const { return id_; }
 
   // Reset the allocator for internal ID values for testing, so that

pc/test/mock_peer_connection_internal.h

@@ -319,7 +319,7 @@ class MockPeerConnectionInternal : public PeerConnectionInternal {
   MOCK_METHOD(void, NoteDataAddedEvent, (), (override));
   MOCK_METHOD(void,
               OnSctpDataChannelStateChanged,
-              (DataChannelInterface * channel, DataChannelInterface::DataState),
+              (int channel_id, DataChannelInterface::DataState),
               (override));
 };