diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc index 3059fdd7f9..a4c8d8852b 100644 --- a/pc/data_channel_controller.cc +++ b/pc/data_channel_controller.cc @@ -159,11 +159,13 @@ void DataChannelController::OnTransportClosed(RTCError error) { } } -void DataChannelController::SetupDataChannelTransport_n( - DataChannelTransportInterface* transport) { +void DataChannelController::SetupDataChannelTransport_n() { RTC_DCHECK_RUN_ON(network_thread()); - RTC_DCHECK(transport); - set_data_channel_transport(transport); + + // There's a new data channel transport. This needs to be signaled to the + // `sctp_data_channels_n_` so that they can reopen and reconnect. This is + // necessary when bundling is applied. + NotifyDataChannelsOfTransportCreated(); } void DataChannelController::PrepareForShutdown() { @@ -173,7 +175,14 @@ void DataChannelController::PrepareForShutdown() { void DataChannelController::TeardownDataChannelTransport_n(RTCError error) { RTC_DCHECK_RUN_ON(network_thread()); - set_data_channel_transport(nullptr); + + OnTransportClosed(error); + + if (data_channel_transport_) { + data_channel_transport_->SetDataSink(nullptr); + set_data_channel_transport(nullptr); + } + RTC_DCHECK(sctp_data_channels_n_.empty()); weak_factory_.InvalidateWeakPtrs(); } @@ -185,7 +194,16 @@ void DataChannelController::OnTransportChanged( data_channel_transport_ != new_data_channel_transport) { // Changed which data channel transport is used for `sctp_mid_` (eg. now // it's bundled). + data_channel_transport_->SetDataSink(nullptr); set_data_channel_transport(new_data_channel_transport); + if (new_data_channel_transport) { + new_data_channel_transport->SetDataSink(this); + + // There's a new data channel transport. This needs to be signaled to the + // `sctp_data_channels_n_` so that they can reopen and reconnect. This is + // necessary when bundling is applied. + NotifyDataChannelsOfTransportCreated(); + } } } @@ -398,21 +416,7 @@ void DataChannelController::OnSctpDataChannelClosed(SctpDataChannel* channel) { void DataChannelController::set_data_channel_transport( DataChannelTransportInterface* transport) { RTC_DCHECK_RUN_ON(network_thread()); - - if (data_channel_transport_) - data_channel_transport_->SetDataSink(nullptr); - data_channel_transport_ = transport; - - if (data_channel_transport_) { - // There's a new data channel transport. This needs to be signaled to the - // `sctp_data_channels_n_` so that they can reopen and reconnect. This is - // necessary when bundling is applied. - NotifyDataChannelsOfTransportCreated(); - data_channel_transport_->SetDataSink(this); - } else { - OnTransportClosed(RTCError::OK()); - } } void DataChannelController::NotifyDataChannelsOfTransportCreated() { diff --git a/pc/data_channel_controller.h b/pc/data_channel_controller.h index ac47c8aa28..9e6af3fa5b 100644 --- a/pc/data_channel_controller.h +++ b/pc/data_channel_controller.h @@ -68,7 +68,7 @@ class DataChannelController : public SctpDataChannelControllerInterface, void PrepareForShutdown(); // Called from PeerConnection::SetupDataChannelTransport_n - void SetupDataChannelTransport_n(DataChannelTransportInterface* transport); + void SetupDataChannelTransport_n(); // Called from PeerConnection::TeardownDataChannelTransport_n void TeardownDataChannelTransport_n(RTCError error); @@ -93,6 +93,9 @@ class DataChannelController : public SctpDataChannelControllerInterface, // At some point in time, a data channel has existed. bool HasUsedDataChannels() const; + // Accessors + void set_data_channel_transport(DataChannelTransportInterface* transport); + void OnSctpDataChannelClosed(SctpDataChannel* channel); protected: @@ -132,8 +135,6 @@ class DataChannelController : public SctpDataChannelControllerInterface, // (calls OnTransportChannelCreated on the signaling thread). void NotifyDataChannelsOfTransportCreated(); - void set_data_channel_transport(DataChannelTransportInterface* transport); - // Plugin transport used for data channels. Pointer may be accessed and // checked from any thread, but the object may only be touched on the // network thread. diff --git a/pc/data_channel_controller_unittest.cc b/pc/data_channel_controller_unittest.cc index ea72e85218..a2a9f00cd3 100644 --- a/pc/data_channel_controller_unittest.cc +++ b/pc/data_channel_controller_unittest.cc @@ -51,15 +51,8 @@ class MockDataChannelTransport : public webrtc::DataChannelTransportInterface { // behavior by calling those methods from within its destructor. class DataChannelControllerForTest : public DataChannelController { public: - explicit DataChannelControllerForTest( - PeerConnectionInternal* pc, - DataChannelTransportInterface* transport = nullptr) - : DataChannelController(pc) { - if (transport) { - network_thread()->BlockingCall( - [&] { SetupDataChannelTransport_n(transport); }); - } - } + explicit DataChannelControllerForTest(PeerConnectionInternal* pc) + : DataChannelController(pc) {} ~DataChannelControllerForTest() override { network_thread()->BlockingCall( @@ -150,7 +143,9 @@ TEST_F(DataChannelControllerTest, MaxChannels) { : rtc::SSL_CLIENT); }); - DataChannelControllerForTest dcc(pc_.get(), &transport); + DataChannelControllerForTest dcc(pc_.get()); + pc_->network_thread()->BlockingCall( + [&] { dcc.set_data_channel_transport(&transport); }); // Allocate the maximum number of channels + 1. Inside the loop, the creation // process will allocate a stream id for each channel. @@ -176,7 +171,9 @@ TEST_F(DataChannelControllerTest, NoStreamIdReuseWhileClosing) { }); NiceMock transport; // Wider scope than `dcc`. - DataChannelControllerForTest dcc(pc_.get(), &transport); + DataChannelControllerForTest dcc(pc_.get()); + pc_->network_thread()->BlockingCall( + [&] { dcc.set_data_channel_transport(&transport); }); // Create the first channel and check that we got the expected, first sid. auto channel1 = dcc.InternalCreateDataChannelWithProxy( diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index d9602ea479..f76ce6396d 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -2113,14 +2113,12 @@ absl::optional PeerConnection::GetDataMid() const { return sctp_mid_s_; } -void PeerConnection::SetSctpDataInfo(absl::string_view mid, - absl::string_view transport_name) { +void PeerConnection::SetSctpDataMid(const std::string& mid) { RTC_DCHECK_RUN_ON(signaling_thread()); - sctp_mid_s_ = std::string(mid); - SetSctpTransportName(std::string(transport_name)); + sctp_mid_s_ = mid; } -void PeerConnection::ResetSctpDataInfo() { +void PeerConnection::ResetSctpDataMid() { RTC_DCHECK_RUN_ON(signaling_thread()); sctp_mid_s_.reset(); SetSctpTransportName(""); @@ -2513,32 +2511,37 @@ absl::optional PeerConnection::GetAudioDeviceStats() { return absl::nullopt; } -absl::optional PeerConnection::SetupDataChannelTransport_n( - absl::string_view mid) { - sctp_mid_n_ = std::string(mid); +bool PeerConnection::SetupDataChannelTransport_n(const std::string& mid) { DataChannelTransportInterface* transport = - transport_controller_->GetDataChannelTransport(*sctp_mid_n_); + transport_controller_->GetDataChannelTransport(mid); if (!transport) { RTC_LOG(LS_ERROR) << "Data channel transport is not available for data channels, mid=" << mid; - sctp_mid_n_ = absl::nullopt; - return absl::nullopt; + return false; } + RTC_LOG(LS_INFO) << "Setting up data channel transport for mid=" << mid; - absl::optional transport_name; + data_channel_controller_.set_data_channel_transport(transport); + data_channel_controller_.SetupDataChannelTransport_n(); + sctp_mid_n_ = mid; cricket::DtlsTransportInternal* dtls_transport = - transport_controller_->GetDtlsTransport(*sctp_mid_n_); + transport_controller_->GetDtlsTransport(mid); if (dtls_transport) { - transport_name = dtls_transport->transport_name(); - } else { - // Make sure we still set a valid string. - transport_name = std::string(""); + signaling_thread()->PostTask( + SafeTask(signaling_thread_safety_.flag(), + [this, name = dtls_transport->transport_name()] { + RTC_DCHECK_RUN_ON(signaling_thread()); + SetSctpTransportName(std::move(name)); + })); } - data_channel_controller_.SetupDataChannelTransport_n(transport); - - return transport_name; + // Note: setting the data sink and checking initial state must be done last, + // after setting up the data channel. Setting the data sink may trigger + // callbacks to PeerConnection which require the transport to be completely + // set up (eg. OnReadyToSend()). + transport->SetDataSink(&data_channel_controller_); + return true; } void PeerConnection::TeardownDataChannelTransport_n(RTCError error) { diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 32d304e6e7..e7cfb9a5c1 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -402,10 +402,9 @@ class PeerConnection : public PeerConnectionInternal, // channels are configured this will return nullopt. absl::optional GetDataMid() const override; - void SetSctpDataInfo(absl::string_view mid, - absl::string_view transport_name) override; + void SetSctpDataMid(const std::string& mid) override; - void ResetSctpDataInfo() override; + void ResetSctpDataMid() override; // Asynchronously calls SctpTransport::Start() on the network thread for // `sctp_mid()` if set. Called as part of setting the local description. @@ -433,8 +432,8 @@ class PeerConnection : public PeerConnectionInternal, // this session. bool SrtpRequired() const override; - absl::optional SetupDataChannelTransport_n( - absl::string_view mid) override RTC_RUN_ON(network_thread()); + bool SetupDataChannelTransport_n(const std::string& mid) override + RTC_RUN_ON(network_thread()); void TeardownDataChannelTransport_n(RTCError error) override RTC_RUN_ON(network_thread()); diff --git a/pc/peer_connection_internal.h b/pc/peer_connection_internal.h index c91a44a148..ab9789d3ab 100644 --- a/pc/peer_connection_internal.h +++ b/pc/peer_connection_internal.h @@ -117,16 +117,10 @@ class PeerConnectionSdpMethods { // Returns true if SRTP (either using DTLS-SRTP or SDES) is required by // this session. virtual bool SrtpRequired() const = 0; - // Configures the data channel transport on the network thread. - // The return value will be unset if an error occurs. If the setup succeeded - // the return value will be set and contain the name of the transport - // (empty string if a name isn't available). - virtual absl::optional SetupDataChannelTransport_n( - absl::string_view mid) = 0; + virtual bool SetupDataChannelTransport_n(const std::string& mid) = 0; virtual void TeardownDataChannelTransport_n(RTCError error) = 0; - virtual void SetSctpDataInfo(absl::string_view mid, - absl::string_view transport_name) = 0; - virtual void ResetSctpDataInfo() = 0; + virtual void SetSctpDataMid(const std::string& mid) = 0; + virtual void ResetSctpDataMid() = 0; virtual const FieldTrialsView& trials() const = 0; diff --git a/pc/sctp_data_channel.cc b/pc/sctp_data_channel.cc index 3fed03ca11..8a53dee0a3 100644 --- a/pc/sctp_data_channel.cc +++ b/pc/sctp_data_channel.cc @@ -725,6 +725,15 @@ void SctpDataChannel::OnDataReceived(DataMessageType type, void SctpDataChannel::OnTransportReady() { RTC_DCHECK_RUN_ON(network_thread_); + + // TODO(bugs.webrtc.org/11547): The transport is configured inside + // `PeerConnection::SetupDataChannelTransport_n`, which results in + // `SctpDataChannel` getting the OnTransportChannelCreated callback, and then + // that's immediately followed by calling `transport->SetDataSink` which is + // what triggers the callback to `OnTransportReady()`. + // These steps are currently accomplished via two separate PostTask calls to + // the signaling thread, but could simply be done in single method call on + // the network thread. RTC_DCHECK(connected_to_transport()); RTC_DCHECK(id_n_.HasValue()); diff --git a/pc/sctp_data_channel.h b/pc/sctp_data_channel.h index 13bebd4612..38b1ae02a7 100644 --- a/pc/sctp_data_channel.h +++ b/pc/sctp_data_channel.h @@ -182,6 +182,11 @@ class SctpDataChannel : public DataChannelInterface { // Specializations of CloseAbruptlyWithError void CloseAbruptlyWithDataChannelFailure(const std::string& message); + // Slots for controller to connect signals to. + // + // TODO(deadbeef): Make these private once we're hooking up signals ourselves, + // instead of relying on SctpDataChannelControllerInterface. + // Called when the SctpTransport's ready to use. That can happen when we've // finished negotiation, or if the channel was created after negotiation has // already finished. diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 59f0b01dfd..4a86ba80b7 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -5093,15 +5093,18 @@ bool SdpOfferAnswerHandler::CreateDataChannel(const std::string& mid) { RTC_LOG(LS_INFO) << "Creating data channel, mid=" << mid; - absl::optional transport_name = - context_->network_thread()->BlockingCall([&] { + if (!context_->network_thread()->BlockingCall([this, &mid] { RTC_DCHECK_RUN_ON(context_->network_thread()); return pc_->SetupDataChannelTransport_n(mid); - }); - if (!transport_name) + })) { return false; - - pc_->SetSctpDataInfo(mid, *transport_name); + } + // TODO(tommi): Is this necessary? SetupDataChannelTransport_n() above + // will have queued up updating the transport name on the signaling thread + // and could update the mid at the same time. This here is synchronous + // though, but it changes the state of PeerConnection and makes it be + // out of sync (transport name not set while the mid is set). + pc_->SetSctpDataMid(mid); return true; } @@ -5112,7 +5115,7 @@ void SdpOfferAnswerHandler::DestroyDataChannelTransport(RTCError error) { RTC_DCHECK_RUN_ON(context_->network_thread()); pc_->TeardownDataChannelTransport_n(error); }); - pc_->ResetSctpDataInfo(); + pc_->ResetSctpDataMid(); } void SdpOfferAnswerHandler::DestroyAllChannels() { diff --git a/pc/test/fake_peer_connection_base.h b/pc/test/fake_peer_connection_base.h index 743c18122a..3178770f7c 100644 --- a/pc/test/fake_peer_connection_base.h +++ b/pc/test/fake_peer_connection_base.h @@ -358,14 +358,12 @@ class FakePeerConnectionBase : public PeerConnectionInternal { Call* call_ptr() override { return nullptr; } bool SrtpRequired() const override { return false; } - absl::optional SetupDataChannelTransport_n( - absl::string_view mid) override { - return absl::nullopt; + bool SetupDataChannelTransport_n(const std::string& mid) override { + return false; } void TeardownDataChannelTransport_n(RTCError error) override {} - void SetSctpDataInfo(absl::string_view mid, - absl::string_view transport_name) override {} - void ResetSctpDataInfo() override {} + void SetSctpDataMid(const std::string& mid) override {} + void ResetSctpDataMid() override {} const FieldTrialsView& trials() const override { return field_trials_; } diff --git a/pc/test/mock_peer_connection_internal.h b/pc/test/mock_peer_connection_internal.h index 58d13ede2f..b8107a3d5b 100644 --- a/pc/test/mock_peer_connection_internal.h +++ b/pc/test/mock_peer_connection_internal.h @@ -263,16 +263,13 @@ class MockPeerConnectionInternal : public PeerConnectionInternal { (override)); MOCK_METHOD(Call*, call_ptr, (), (override)); MOCK_METHOD(bool, SrtpRequired, (), (const, override)); - MOCK_METHOD(absl::optional, + MOCK_METHOD(bool, SetupDataChannelTransport_n, - (absl::string_view mid), + (const std::string&), (override)); MOCK_METHOD(void, TeardownDataChannelTransport_n, (RTCError), (override)); - MOCK_METHOD(void, - SetSctpDataInfo, - (absl::string_view, absl::string_view), - (override)); - MOCK_METHOD(void, ResetSctpDataInfo, (), (override)); + MOCK_METHOD(void, SetSctpDataMid, (const std::string&), (override)); + MOCK_METHOD(void, ResetSctpDataMid, (), (override)); MOCK_METHOD(const FieldTrialsView&, trials, (), (const, override)); // PeerConnectionInternal