Reland "Add param to DCC::SetupDataChannelTransport_n, simplify DCC* setup code."
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 <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}
Bug: webrtc:11547
Change-Id: I8ebbc3d3a12786dff2096350a77e03e98466ff00
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/301702
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39884}
diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc
index a4c8d88..0a0d93e 100644
--- a/pc/data_channel_controller.cc
+++ b/pc/data_channel_controller.cc
@@ -159,13 +159,11 @@
}
}
-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::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 @@
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::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 9e6af3f..ac47c8a 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();
+ void SetupDataChannelTransport_n(DataChannelTransportInterface* transport);
// Called from PeerConnection::TeardownDataChannelTransport_n
void TeardownDataChannelTransport_n(RTCError error);
@@ -93,9 +93,6 @@
// 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 @@
// (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 a2a9f00..ea72e85 100644
--- a/pc/data_channel_controller_unittest.cc
+++ b/pc/data_channel_controller_unittest.cc
@@ -51,8 +51,15 @@
// 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 @@
: 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 @@
});
NiceMock<MockDataChannelTransport> 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 f76ce63..d9602ea 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -2113,12 +2113,14 @@
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 @@
return absl::nullopt;
}
-bool PeerConnection::SetupDataChannelTransport_n(const std::string& mid) {
+absl::optional<std::string> 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<std::string> 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 e7cfb9a..32d304e 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -402,9 +402,10 @@
// channels are configured this will return nullopt.
absl::optional<std::string> 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 @@
// this session.
bool SrtpRequired() const override;
- bool SetupDataChannelTransport_n(const std::string& mid) override
- RTC_RUN_ON(network_thread());
+ absl::optional<std::string> 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 ab9789d..c91a44a 100644
--- a/pc/peer_connection_internal.h
+++ b/pc/peer_connection_internal.h
@@ -117,10 +117,16 @@
// 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<std::string> 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 8a53dee..3fed03c 100644
--- a/pc/sctp_data_channel.cc
+++ b/pc/sctp_data_channel.cc
@@ -725,15 +725,6 @@
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 38b1ae0..13bebd4 100644
--- a/pc/sctp_data_channel.h
+++ b/pc/sctp_data_channel.h
@@ -182,11 +182,6 @@
// 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 4a86ba8..59f0b01 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -5093,18 +5093,15 @@
RTC_LOG(LS_INFO) << "Creating data channel, mid=" << mid;
- if (!context_->network_thread()->BlockingCall([this, &mid] {
+ absl::optional<std::string> 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 @@
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 3178770..743c181 100644
--- a/pc/test/fake_peer_connection_base.h
+++ b/pc/test/fake_peer_connection_base.h
@@ -358,12 +358,14 @@
Call* call_ptr() override { return nullptr; }
bool SrtpRequired() const override { return false; }
- bool SetupDataChannelTransport_n(const std::string& mid) override {
- return false;
+ absl::optional<std::string> 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 b8107a3..58d13ed 100644
--- a/pc/test/mock_peer_connection_internal.h
+++ b/pc/test/mock_peer_connection_internal.h
@@ -263,13 +263,16 @@
(override));
MOCK_METHOD(Call*, call_ptr, (), (override));
MOCK_METHOD(bool, SrtpRequired, (), (const, override));
- MOCK_METHOD(bool,
+ MOCK_METHOD(absl::optional<std::string>,
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
