dtls-in-stun: Defer writable until Ice has become writable
This patch modifies the DtlsTransport::set_writable
to only sinal writable (true) if ice has been writable
at least once. This is the behaviour before dtls-in-stun
(since the handshake starts when ice becomes writable).
This change reduce the observable changes (from within webrtc) of
the dtls-in-stun feature, keeping the positive aspects (i.e.
faster/more-robust dtls handshake)
BUG=webrtc:367395350
Change-Id: I2a70c8ecb6fcd83734c211a6b40aef02f6c23f55
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/381940
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Jonas Oreland <jonaso@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#44171}
diff --git a/p2p/dtls/dtls_transport.cc b/p2p/dtls/dtls_transport.cc
index 5e55bdb..5d0740b 100644
--- a/p2p/dtls/dtls_transport.cc
+++ b/p2p/dtls/dtls_transport.cc
@@ -619,6 +619,8 @@
<< ": ice_transport writable state changed to "
<< ice_transport_->writable();
+ ice_has_been_writable_ = true;
+
if (!dtls_active_) {
// Not doing DTLS.
// Note: SignalWritableState fired by set_writable.
@@ -910,6 +912,16 @@
if (writable_ == writable) {
return;
}
+ if (writable && !ice_has_been_writable_) {
+ // Wait with reporting writable until ICE has become writable once,
+ // so as to not confuse other part of stack (such as sctp).
+ RTC_DCHECK(dtls_in_stun_);
+ RTC_LOG(LS_INFO)
+ << ToString()
+ << ": defer set_writable(true) until ICE has become writable once";
+ return;
+ }
+
if (event_log_) {
event_log_->Log(
std::make_unique<webrtc::RtcEventDtlsWritableState>(writable));
diff --git a/p2p/dtls/dtls_transport.h b/p2p/dtls/dtls_transport.h
index 3f0a51e..3bd5b68 100644
--- a/p2p/dtls/dtls_transport.h
+++ b/p2p/dtls/dtls_transport.h
@@ -286,6 +286,12 @@
bool receiving_ = false;
bool writable_ = false;
+ // Keep track if ICE has ever been writable.
+ // This is used to prevent "spurious" Dtls::Writable with DTLS-in-STUN,
+ // where DTLS can become writable before ICE. This can confuse other parts
+ // of the stack.
+ bool ice_has_been_writable_ = false;
+
webrtc::RtcEventLog* const event_log_;
// Initialized in constructor based on WebRTC-IceHandshakeDtls,
diff --git a/p2p/dtls/dtls_transport_unittest.cc b/p2p/dtls/dtls_transport_unittest.cc
index ae69627..de8f3f5 100644
--- a/p2p/dtls/dtls_transport_unittest.cc
+++ b/p2p/dtls/dtls_transport_unittest.cc
@@ -1457,6 +1457,67 @@
ClearPacketFilters();
}
+TEST_P(DtlsTransportDtlsInStunTest,
+ DtlsDoesNotSignalWritableUnlessIceWritableOnce) {
+ Prepare(/* rtt_estimate= */ false);
+ AddPacketLogging();
+
+ RTC_LOG(LS_INFO) << "client1: " << std::get<0>(GetParam());
+ RTC_LOG(LS_INFO) << "client2: " << std::get<1>(GetParam());
+
+ ASSERT_TRUE(client1_.ConnectIceTransport(&client2_));
+
+ client1_.SendIcePing();
+ ASSERT_TRUE(WaitUntil([&] {
+ return client2_.fake_ice_transport()->GetCountOfReceivedStunMessages(
+ STUN_BINDING_REQUEST) == 1;
+ }));
+ client2_.SendIcePingConf();
+ ASSERT_TRUE(WaitUntil([&] {
+ return client1_.fake_ice_transport()->GetCountOfReceivedStunMessages(
+ STUN_BINDING_RESPONSE) == 1;
+ }));
+ client1_.SendIcePing();
+ ASSERT_TRUE(WaitUntil([&] {
+ return client2_.fake_ice_transport()->GetCountOfReceivedStunMessages(
+ STUN_BINDING_REQUEST) == 2;
+ }));
+ client2_.SendIcePingConf();
+ ASSERT_TRUE(WaitUntil([&] {
+ return client1_.fake_ice_transport()->GetCountOfReceivedStunMessages(
+ STUN_BINDING_RESPONSE) == 2;
+ }));
+
+ bool dtls_in_stun = std::get<0>(GetParam()).dtls_in_stun &&
+ std::get<1>(GetParam()).dtls_in_stun;
+ if (dtls_in_stun) {
+ ASSERT_TRUE(client1_.dtls_transport()->writable());
+ }
+ // Ice has never been writable on client2.
+ ASSERT_FALSE(client2_.dtls_transport()->writable());
+
+ client2_.SendIcePing();
+ ASSERT_TRUE(WaitUntil([&] {
+ return client1_.fake_ice_transport()->GetCountOfReceivedStunMessages(
+ STUN_BINDING_REQUEST) == 1;
+ }));
+ client1_.SendIcePingConf();
+ ASSERT_TRUE(WaitUntil([&] {
+ return client2_.fake_ice_transport()->GetCountOfReceivedStunMessages(
+ STUN_BINDING_RESPONSE) == 1;
+ }));
+
+ EXPECT_TRUE(WaitUntil([&] {
+ return client1_.dtls_transport()->writable() &&
+ client2_.dtls_transport()->writable();
+ }));
+
+ EXPECT_TRUE(client1_.dtls_transport()->writable());
+ EXPECT_TRUE(client2_.dtls_transport()->writable());
+
+ ClearPacketFilters();
+}
+
INSTANTIATE_TEST_SUITE_P(DtlsTransportDtlsInStunTest,
DtlsTransportDtlsInStunTest,
testing::ValuesIn(AllEndpointVariants()));
