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 <tommi@webrtc.org>
> Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
> 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 <mbonadei@webrtc.org>
Owners-Override: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#39864}
diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc
index 3059fdd..a4c8d88 100644
--- a/pc/data_channel_controller.cc
+++ b/pc/data_channel_controller.cc
@@ -159,11 +159,13 @@
}
}
-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::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 @@
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::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 ac47c8a..9e6af3f 100644
--- a/pc/data_channel_controller.h
+++ b/pc/data_channel_controller.h
@@ -68,7 +68,7 @@
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 @@
// 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 @@
// (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 ea72e85..a2a9f00 100644
--- a/pc/data_channel_controller_unittest.cc
+++ b/pc/data_channel_controller_unittest.cc
@@ -51,15 +51,8 @@
// 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 @@
: 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 @@
});
NiceMock<MockDataChannelTransport> 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 d9602ea..f76ce63 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -2113,14 +2113,12 @@
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 @@
return absl::nullopt;
}
-absl::optional<std::string> 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<std::string> 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 32d304e..e7cfb9a 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -402,10 +402,9 @@
// channels are configured this will return nullopt.
absl::optional<std::string> 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 @@
// this session.
bool SrtpRequired() const override;
- absl::optional<std::string> 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 c91a44a..ab9789d 100644
--- a/pc/peer_connection_internal.h
+++ b/pc/peer_connection_internal.h
@@ -117,16 +117,10 @@
// 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<std::string> 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 3fed03c..8a53dee 100644
--- a/pc/sctp_data_channel.cc
+++ b/pc/sctp_data_channel.cc
@@ -725,6 +725,15 @@
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 13bebd4..38b1ae0 100644
--- a/pc/sctp_data_channel.h
+++ b/pc/sctp_data_channel.h
@@ -182,6 +182,11 @@
// 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 59f0b01..4a86ba8 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -5093,15 +5093,18 @@
RTC_LOG(LS_INFO) << "Creating data channel, mid=" << mid;
- absl::optional<std::string> 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 @@
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 743c181..3178770 100644
--- a/pc/test/fake_peer_connection_base.h
+++ b/pc/test/fake_peer_connection_base.h
@@ -358,14 +358,12 @@
Call* call_ptr() override { return nullptr; }
bool SrtpRequired() const override { return false; }
- absl::optional<std::string> 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 58d13ed..b8107a3 100644
--- a/pc/test/mock_peer_connection_internal.h
+++ b/pc/test/mock_peer_connection_internal.h
@@ -263,16 +263,13 @@
(override));
MOCK_METHOD(Call*, call_ptr, (), (override));
MOCK_METHOD(bool, SrtpRequired, (), (const, override));
- MOCK_METHOD(absl::optional<std::string>,
+ 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
