From aa3c9f297219fd9ff7d75be281cc61b2e4d67bb0 Mon Sep 17 00:00:00 2001 From: Tommi Date: Tue, 18 Apr 2023 12:19:19 +0200 Subject: [PATCH] Reland "Add param to DCC::SetupDataChannelTransport_n, simplify DCC* setup code." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 298313534df2420e079ffc6fc9c6019d01d29a88. Changes from the original commit: * Call OnTransportClosed() from TeardownDataChannelTransport_n() (same as before the original commit) * Not call OnTransportClosed() from OnTransportChanged() when its called with nullptr (also preserving the behaviour from before the original commit). Original change's description: > Revert "Add param to DCC::SetupDataChannelTransport_n, simplify DCC* setup code." > > This reverts commit 2ec6a6c57830e06f601607c1b9473ad821b57e07. > > Reason for revert: It breaks WPT tests (e.g. https://ci.chromium.org/ui/p/chromium/builders/try/linux-rel/1361972/overview) blocking the roll into Chromium. > > Original change's description: > > Add param to DCC::SetupDataChannelTransport_n, simplify DCC* setup code. > > > > * DCC = DataChannelController. > > > > * Consolidate steps to set the mid and transport name. They're now > > set at the same time and without a separate PostTask. > > * Transport sink is now consistently set in DCC > > * Order of notifications for setting up the transport is now the same > > regardless of the first time the transport is being set or if it's > > being replaced. > > * Made set_data_channel_transport() private. > > > > Bug: webrtc:11547 > > Change-Id: I39e89c6e269e6f06d55981d7944678bf23c8817a > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/300562 > > Commit-Queue: Tomas Gunnarsson > > Reviewed-by: Danil Chapovalov > > Cr-Commit-Position: refs/heads/main@{#39859} > > Bug: webrtc:11547 > Change-Id: I0d8d7453b71be80fbf1b7eba7d161336e29de091 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/301360 > Auto-Submit: Mirko Bonadei > Owners-Override: Mirko Bonadei > Commit-Queue: Mirko Bonadei > Bot-Commit: rubber-stamper@appspot.gserviceaccount.com > Cr-Commit-Position: refs/heads/main@{#39864} Bug: webrtc:11547 Change-Id: I8ebbc3d3a12786dff2096350a77e03e98466ff00 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/301702 Reviewed-by: Henrik Boström Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#39884} --- pc/data_channel_controller.cc | 39 ++++++++++------------ pc/data_channel_controller.h | 7 ++-- pc/data_channel_controller_unittest.cc | 19 ++++++----- pc/peer_connection.cc | 43 ++++++++++++------------- pc/peer_connection.h | 9 +++--- pc/peer_connection_internal.h | 12 +++++-- pc/sctp_data_channel.cc | 9 ------ pc/sctp_data_channel.h | 5 --- pc/sdp_offer_answer.cc | 17 ++++------ pc/test/fake_peer_connection_base.h | 10 +++--- pc/test/mock_peer_connection_internal.h | 11 ++++--- 11 files changed, 85 insertions(+), 96 deletions(-) diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc index a4c8d8852b..0a0d93e236 100644 --- a/pc/data_channel_controller.cc +++ b/pc/data_channel_controller.cc @@ -159,13 +159,11 @@ void DataChannelController::OnTransportClosed(RTCError error) { } } -void DataChannelController::SetupDataChannelTransport_n() { +void DataChannelController::SetupDataChannelTransport_n( + DataChannelTransportInterface* transport) { RTC_DCHECK_RUN_ON(network_thread()); - - // 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(); + RTC_DCHECK(transport); + set_data_channel_transport(transport); } void DataChannelController::PrepareForShutdown() { @@ -175,14 +173,8 @@ void DataChannelController::PrepareForShutdown() { void DataChannelController::TeardownDataChannelTransport_n(RTCError error) { RTC_DCHECK_RUN_ON(network_thread()); - OnTransportClosed(error); - - if (data_channel_transport_) { - data_channel_transport_->SetDataSink(nullptr); - set_data_channel_transport(nullptr); - } - + set_data_channel_transport(nullptr); RTC_DCHECK(sctp_data_channels_n_.empty()); weak_factory_.InvalidateWeakPtrs(); } @@ -194,16 +186,7 @@ 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(); - } } } @@ -416,7 +399,19 @@ 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); + } } void DataChannelController::NotifyDataChannelsOfTransportCreated() { diff --git a/pc/data_channel_controller.h b/pc/data_channel_controller.h index 9e6af3fa5b..ac47c8aa28 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(); + void SetupDataChannelTransport_n(DataChannelTransportInterface* transport); // Called from PeerConnection::TeardownDataChannelTransport_n void TeardownDataChannelTransport_n(RTCError error); @@ -93,9 +93,6 @@ 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: @@ -135,6 +132,8 @@ 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 a2a9f00cd3..ea72e85218 100644 --- a/pc/data_channel_controller_unittest.cc +++ b/pc/data_channel_controller_unittest.cc @@ -51,8 +51,15 @@ class MockDataChannelTransport : public webrtc::DataChannelTransportInterface { // behavior by calling those methods from within its destructor. class DataChannelControllerForTest : public DataChannelController { public: - explicit DataChannelControllerForTest(PeerConnectionInternal* pc) - : DataChannelController(pc) {} + explicit DataChannelControllerForTest( + PeerConnectionInternal* pc, + DataChannelTransportInterface* transport = nullptr) + : DataChannelController(pc) { + if (transport) { + network_thread()->BlockingCall( + [&] { SetupDataChannelTransport_n(transport); }); + } + } ~DataChannelControllerForTest() override { network_thread()->BlockingCall( @@ -143,9 +150,7 @@ TEST_F(DataChannelControllerTest, MaxChannels) { : rtc::SSL_CLIENT); }); - DataChannelControllerForTest dcc(pc_.get()); - pc_->network_thread()->BlockingCall( - [&] { dcc.set_data_channel_transport(&transport); }); + DataChannelControllerForTest dcc(pc_.get(), &transport); // Allocate the maximum number of channels + 1. Inside the loop, the creation // process will allocate a stream id for each channel. @@ -171,9 +176,7 @@ TEST_F(DataChannelControllerTest, NoStreamIdReuseWhileClosing) { }); NiceMock transport; // Wider scope than `dcc`. - DataChannelControllerForTest dcc(pc_.get()); - pc_->network_thread()->BlockingCall( - [&] { dcc.set_data_channel_transport(&transport); }); + DataChannelControllerForTest dcc(pc_.get(), &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 f76ce6396d..d9602ea479 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -2113,12 +2113,14 @@ absl::optional PeerConnection::GetDataMid() const { return sctp_mid_s_; } -void PeerConnection::SetSctpDataMid(const std::string& mid) { +void PeerConnection::SetSctpDataInfo(absl::string_view mid, + absl::string_view transport_name) { RTC_DCHECK_RUN_ON(signaling_thread()); - sctp_mid_s_ = mid; + sctp_mid_s_ = std::string(mid); + SetSctpTransportName(std::string(transport_name)); } -void PeerConnection::ResetSctpDataMid() { +void PeerConnection::ResetSctpDataInfo() { RTC_DCHECK_RUN_ON(signaling_thread()); sctp_mid_s_.reset(); SetSctpTransportName(""); @@ -2511,37 +2513,32 @@ absl::optional PeerConnection::GetAudioDeviceStats() { return absl::nullopt; } -bool PeerConnection::SetupDataChannelTransport_n(const std::string& mid) { +absl::optional PeerConnection::SetupDataChannelTransport_n( + absl::string_view mid) { + sctp_mid_n_ = std::string(mid); DataChannelTransportInterface* transport = - transport_controller_->GetDataChannelTransport(mid); + transport_controller_->GetDataChannelTransport(*sctp_mid_n_); if (!transport) { RTC_LOG(LS_ERROR) << "Data channel transport is not available for data channels, mid=" << mid; - return false; + sctp_mid_n_ = absl::nullopt; + return absl::nullopt; } - RTC_LOG(LS_INFO) << "Setting up data channel transport for mid=" << mid; - data_channel_controller_.set_data_channel_transport(transport); - data_channel_controller_.SetupDataChannelTransport_n(); - sctp_mid_n_ = mid; + absl::optional transport_name; cricket::DtlsTransportInternal* dtls_transport = - transport_controller_->GetDtlsTransport(mid); + transport_controller_->GetDtlsTransport(*sctp_mid_n_); if (dtls_transport) { - signaling_thread()->PostTask( - SafeTask(signaling_thread_safety_.flag(), - [this, name = dtls_transport->transport_name()] { - RTC_DCHECK_RUN_ON(signaling_thread()); - SetSctpTransportName(std::move(name)); - })); + transport_name = dtls_transport->transport_name(); + } else { + // Make sure we still set a valid string. + transport_name = std::string(""); } - // 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; + data_channel_controller_.SetupDataChannelTransport_n(transport); + + return transport_name; } void PeerConnection::TeardownDataChannelTransport_n(RTCError error) { diff --git a/pc/peer_connection.h b/pc/peer_connection.h index e7cfb9a5c1..32d304e6e7 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -402,9 +402,10 @@ class PeerConnection : public PeerConnectionInternal, // channels are configured this will return nullopt. absl::optional GetDataMid() const override; - void SetSctpDataMid(const std::string& mid) override; + void SetSctpDataInfo(absl::string_view mid, + absl::string_view transport_name) override; - void ResetSctpDataMid() override; + void ResetSctpDataInfo() override; // Asynchronously calls SctpTransport::Start() on the network thread for // `sctp_mid()` if set. Called as part of setting the local description. @@ -432,8 +433,8 @@ class PeerConnection : public PeerConnectionInternal, // this session. bool SrtpRequired() const override; - bool SetupDataChannelTransport_n(const std::string& mid) override - RTC_RUN_ON(network_thread()); + absl::optional SetupDataChannelTransport_n( + absl::string_view 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 ab9789d3ab..c91a44a148 100644 --- a/pc/peer_connection_internal.h +++ b/pc/peer_connection_internal.h @@ -117,10 +117,16 @@ class PeerConnectionSdpMethods { // Returns true if SRTP (either using DTLS-SRTP or SDES) is required by // this session. virtual bool SrtpRequired() const = 0; - virtual bool SetupDataChannelTransport_n(const std::string& mid) = 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 void TeardownDataChannelTransport_n(RTCError error) = 0; - virtual void SetSctpDataMid(const std::string& mid) = 0; - virtual void ResetSctpDataMid() = 0; + virtual void SetSctpDataInfo(absl::string_view mid, + absl::string_view transport_name) = 0; + virtual void ResetSctpDataInfo() = 0; virtual const FieldTrialsView& trials() const = 0; diff --git a/pc/sctp_data_channel.cc b/pc/sctp_data_channel.cc index 8a53dee0a3..3fed03ca11 100644 --- a/pc/sctp_data_channel.cc +++ b/pc/sctp_data_channel.cc @@ -725,15 +725,6 @@ 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 38b1ae02a7..13bebd4612 100644 --- a/pc/sctp_data_channel.h +++ b/pc/sctp_data_channel.h @@ -182,11 +182,6 @@ 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 4a86ba80b7..59f0b01dfd 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -5093,18 +5093,15 @@ bool SdpOfferAnswerHandler::CreateDataChannel(const std::string& mid) { RTC_LOG(LS_INFO) << "Creating data channel, mid=" << mid; - if (!context_->network_thread()->BlockingCall([this, &mid] { + absl::optional transport_name = + context_->network_thread()->BlockingCall([&] { RTC_DCHECK_RUN_ON(context_->network_thread()); return pc_->SetupDataChannelTransport_n(mid); - })) { + }); + if (!transport_name) return false; - } - // 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); + + pc_->SetSctpDataInfo(mid, *transport_name); return true; } @@ -5115,7 +5112,7 @@ void SdpOfferAnswerHandler::DestroyDataChannelTransport(RTCError error) { RTC_DCHECK_RUN_ON(context_->network_thread()); pc_->TeardownDataChannelTransport_n(error); }); - pc_->ResetSctpDataMid(); + pc_->ResetSctpDataInfo(); } void SdpOfferAnswerHandler::DestroyAllChannels() { diff --git a/pc/test/fake_peer_connection_base.h b/pc/test/fake_peer_connection_base.h index 3178770f7c..743c18122a 100644 --- a/pc/test/fake_peer_connection_base.h +++ b/pc/test/fake_peer_connection_base.h @@ -358,12 +358,14 @@ class FakePeerConnectionBase : public PeerConnectionInternal { Call* call_ptr() override { return nullptr; } bool SrtpRequired() const override { return false; } - bool SetupDataChannelTransport_n(const std::string& mid) override { - return false; + absl::optional SetupDataChannelTransport_n( + absl::string_view mid) override { + return absl::nullopt; } void TeardownDataChannelTransport_n(RTCError error) override {} - void SetSctpDataMid(const std::string& mid) override {} - void ResetSctpDataMid() override {} + void SetSctpDataInfo(absl::string_view mid, + absl::string_view transport_name) override {} + void ResetSctpDataInfo() 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 b8107a3d5b..58d13ede2f 100644 --- a/pc/test/mock_peer_connection_internal.h +++ b/pc/test/mock_peer_connection_internal.h @@ -263,13 +263,16 @@ class MockPeerConnectionInternal : public PeerConnectionInternal { (override)); MOCK_METHOD(Call*, call_ptr, (), (override)); MOCK_METHOD(bool, SrtpRequired, (), (const, override)); - MOCK_METHOD(bool, + MOCK_METHOD(absl::optional, SetupDataChannelTransport_n, - (const std::string&), + (absl::string_view mid), (override)); MOCK_METHOD(void, TeardownDataChannelTransport_n, (RTCError), (override)); - MOCK_METHOD(void, SetSctpDataMid, (const std::string&), (override)); - MOCK_METHOD(void, ResetSctpDataMid, (), (override)); + MOCK_METHOD(void, + SetSctpDataInfo, + (absl::string_view, absl::string_view), + (override)); + MOCK_METHOD(void, ResetSctpDataInfo, (), (override)); MOCK_METHOD(const FieldTrialsView&, trials, (), (const, override)); // PeerConnectionInternal