diff --git a/media/sctp/sctptransport.cc b/media/sctp/sctptransport.cc index e893c03e3d..bcfb4f4c2f 100644 --- a/media/sctp/sctptransport.cc +++ b/media/sctp/sctptransport.cc @@ -361,8 +361,8 @@ class SctpTransport::UsrSctpWrapper { // CopyOnWriteBuffer is the most convenient way to do this. transport->invoker_.AsyncInvoke( RTC_FROM_HERE, transport->network_thread_, - rtc::Bind(&SctpTransport::OnInboundPacketFromSctpToChannel, transport, - buffer, params, flags)); + rtc::Bind(&SctpTransport::OnInboundPacketFromSctpToTransport, + transport, buffer, params, flags)); } free(data); return 1; @@ -405,14 +405,14 @@ class SctpTransport::UsrSctpWrapper { }; SctpTransport::SctpTransport(rtc::Thread* network_thread, - rtc::PacketTransportInternal* channel) + rtc::PacketTransportInternal* transport) : network_thread_(network_thread), - transport_channel_(channel), - was_ever_writable_(channel->writable()) { + transport_(transport), + was_ever_writable_(transport->writable()) { RTC_DCHECK(network_thread_); - RTC_DCHECK(transport_channel_); + RTC_DCHECK(transport_); RTC_DCHECK_RUN_ON(network_thread_); - ConnectTransportChannelSignals(); + ConnectTransportSignals(); } SctpTransport::~SctpTransport() { @@ -420,15 +420,14 @@ SctpTransport::~SctpTransport() { CloseSctpSocket(); } -void SctpTransport::SetTransportChannel(rtc::PacketTransportInternal* channel) { +void SctpTransport::SetDtlsTransport(rtc::PacketTransportInternal* transport) { RTC_DCHECK_RUN_ON(network_thread_); - RTC_DCHECK(channel); - DisconnectTransportChannelSignals(); - transport_channel_ = channel; - ConnectTransportChannelSignals(); - if (!was_ever_writable_ && channel->writable()) { + DisconnectTransportSignals(); + transport_ = transport; + ConnectTransportSignals(); + if (!was_ever_writable_ && transport && transport->writable()) { was_ever_writable_ = true; - // New channel is writable, now we can start the SCTP connection if Start + // New transport is writable, now we can start the SCTP connection if Start // was called already. if (started_) { RTC_DCHECK(!sock_); @@ -457,7 +456,7 @@ bool SctpTransport::Start(int local_sctp_port, int remote_sctp_port) { remote_port_ = remote_sctp_port; started_ = true; RTC_DCHECK(!sock_); - // Only try to connect if the DTLS channel has been writable before + // Only try to connect if the DTLS transport has been writable before // (indicating that the DTLS handshake is complete). if (was_ever_writable_) { return Connect(); @@ -591,18 +590,23 @@ bool SctpTransport::ReadyToSendData() { return ready_to_send_data_; } -void SctpTransport::ConnectTransportChannelSignals() { +void SctpTransport::ConnectTransportSignals() { RTC_DCHECK_RUN_ON(network_thread_); - transport_channel_->SignalWritableState.connect( - this, &SctpTransport::OnWritableState); - transport_channel_->SignalReadPacket.connect(this, - &SctpTransport::OnPacketRead); + if (!transport_) { + return; + } + transport_->SignalWritableState.connect(this, + &SctpTransport::OnWritableState); + transport_->SignalReadPacket.connect(this, &SctpTransport::OnPacketRead); } -void SctpTransport::DisconnectTransportChannelSignals() { +void SctpTransport::DisconnectTransportSignals() { RTC_DCHECK_RUN_ON(network_thread_); - transport_channel_->SignalWritableState.disconnect(this); - transport_channel_->SignalReadPacket.disconnect(this); + if (!transport_) { + return; + } + transport_->SignalWritableState.disconnect(this); + transport_->SignalReadPacket.disconnect(this); } bool SctpTransport::Connect() { @@ -836,7 +840,7 @@ void SctpTransport::SetReadyToSendData() { void SctpTransport::OnWritableState(rtc::PacketTransportInternal* transport) { RTC_DCHECK_RUN_ON(network_thread_); - RTC_DCHECK_EQ(transport_channel_, transport); + RTC_DCHECK_EQ(transport_, transport); if (!was_ever_writable_ && transport->writable()) { was_ever_writable_ = true; if (started_) { @@ -852,7 +856,7 @@ void SctpTransport::OnPacketRead(rtc::PacketTransportInternal* transport, const rtc::PacketTime& packet_time, int flags) { RTC_DCHECK_RUN_ON(network_thread_); - RTC_DCHECK_EQ(transport_channel_, transport); + RTC_DCHECK_EQ(transport_, transport); TRACE_EVENT0("webrtc", "SctpTransport::OnPacketRead"); if (flags & PF_SRTP_BYPASS) { @@ -906,24 +910,24 @@ void SctpTransport::OnPacketFromSctpToNetwork( } TRACE_EVENT0("webrtc", "SctpTransport::OnPacketFromSctpToNetwork"); - // Don't create noise by trying to send a packet when the DTLS channel isn't + // Don't create noise by trying to send a packet when the DTLS transport isn't // even writable. - if (!transport_channel_->writable()) { + if (!transport_ || !transport_->writable()) { return; } // Bon voyage. - transport_channel_->SendPacket(buffer.data(), buffer.size(), - rtc::PacketOptions(), PF_NORMAL); + transport_->SendPacket(buffer.data(), buffer.size(), + rtc::PacketOptions(), PF_NORMAL); } -void SctpTransport::OnInboundPacketFromSctpToChannel( +void SctpTransport::OnInboundPacketFromSctpToTransport( const rtc::CopyOnWriteBuffer& buffer, ReceiveDataParams params, int flags) { RTC_DCHECK_RUN_ON(network_thread_); RTC_LOG(LS_VERBOSE) << debug_name_ - << "->OnInboundPacketFromSctpToChannel(...): " + << "->OnInboundPacketFromSctpToTransport(...): " << "Received SCTP data:" << " sid=" << params.sid << " notification: " << (flags & MSG_NOTIFICATION) @@ -932,22 +936,22 @@ void SctpTransport::OnInboundPacketFromSctpToChannel( // connection" message. This sets sock_ = NULL; if (!buffer.size() || !buffer.data()) { RTC_LOG(LS_INFO) << debug_name_ - << "->OnInboundPacketFromSctpToChannel(...): " + << "->OnInboundPacketFromSctpToTransport(...): " "No data, closing."; return; } if (flags & MSG_NOTIFICATION) { OnNotificationFromSctp(buffer); } else { - OnDataFromSctpToChannel(params, buffer); + OnDataFromSctpToTransport(params, buffer); } } -void SctpTransport::OnDataFromSctpToChannel( +void SctpTransport::OnDataFromSctpToTransport( const ReceiveDataParams& params, const rtc::CopyOnWriteBuffer& buffer) { RTC_DCHECK_RUN_ON(network_thread_); - RTC_LOG(LS_VERBOSE) << debug_name_ << "->OnDataFromSctpToChannel(...): " + RTC_LOG(LS_VERBOSE) << debug_name_ << "->OnDataFromSctpToTransport(...): " << "Posting with length: " << buffer.size() << " on stream " << params.sid; // Reports all received messages to upper layers, no matter whether the sid diff --git a/media/sctp/sctptransport.h b/media/sctp/sctptransport.h index e99784dbe3..25f2f567e7 100644 --- a/media/sctp/sctptransport.h +++ b/media/sctp/sctptransport.h @@ -46,15 +46,15 @@ struct SctpInboundPacket; // 3. OnSctpOutboundPacket(wrapped_data) // [sctp thread returns having async invoked on the network thread] // 4. SctpTransport::OnPacketFromSctpToNetwork(wrapped_data) -// 5. TransportChannel::SendPacket(wrapped_data) +// 5. DtlsTransport::SendPacket(wrapped_data) // 6. ... across network ... a packet is sent back ... // 7. SctpTransport::OnPacketReceived(wrapped_data) // 8. usrsctp_conninput(wrapped_data) // [network thread returns; sctp thread then calls the following] // 9. OnSctpInboundData(data) // [sctp thread returns having async invoked on the network thread] -// 10. SctpTransport::OnInboundPacketFromSctpToChannel(inboundpacket) -// 11. SctpTransport::OnDataFromSctpToChannel(data) +// 10. SctpTransport::OnInboundPacketFromSctpToTransport(inboundpacket) +// 11. SctpTransport::OnDataFromSctpToTransport(data) // 12. SctpTransport::SignalDataReceived(data) // [from the same thread, methods registered/connected to // SctpTransport are called with the recieved data] @@ -71,7 +71,7 @@ class SctpTransport : public SctpTransportInternal, ~SctpTransport() override; // SctpTransportInternal overrides (see sctptransportinternal.h for comments). - void SetTransportChannel(rtc::PacketTransportInternal* channel) override; + void SetDtlsTransport(rtc::PacketTransportInternal* transport) override; bool Start(int local_port, int remote_port) override; bool OpenStream(int sid) override; bool ResetStream(int sid) override; @@ -88,8 +88,8 @@ class SctpTransport : public SctpTransportInternal, rtc::Thread* network_thread() const { return network_thread_; } private: - void ConnectTransportChannelSignals(); - void DisconnectTransportChannelSignals(); + void ConnectTransportSignals(); + void DisconnectTransportSignals(); // Creates the socket and connects. bool Connect(); @@ -124,11 +124,11 @@ class SctpTransport : public SctpTransportInternal, // Called using |invoker_| to decide what to do with the packet. // The |flags| parameter is used by SCTP to distinguish notification packets // from other types of packets. - void OnInboundPacketFromSctpToChannel(const rtc::CopyOnWriteBuffer& buffer, - ReceiveDataParams params, - int flags); - void OnDataFromSctpToChannel(const ReceiveDataParams& params, - const rtc::CopyOnWriteBuffer& buffer); + void OnInboundPacketFromSctpToTransport(const rtc::CopyOnWriteBuffer& buffer, + ReceiveDataParams params, + int flags); + void OnDataFromSctpToTransport(const ReceiveDataParams& params, + const rtc::CopyOnWriteBuffer& buffer); void OnNotificationFromSctp(const rtc::CopyOnWriteBuffer& buffer); void OnNotificationAssocChange(const sctp_assoc_change& change); @@ -140,7 +140,7 @@ class SctpTransport : public SctpTransportInternal, // Helps pass inbound/outbound packets asynchronously to the network thread. rtc::AsyncInvoker invoker_; // Underlying DTLS channel. - rtc::PacketTransportInternal* transport_channel_; + rtc::PacketTransportInternal* transport_; bool was_ever_writable_ = false; int local_port_ = kSctpDefaultPort; int remote_port_ = kSctpDefaultPort; @@ -149,8 +149,7 @@ class SctpTransport : public SctpTransportInternal, // Has Start been called? Don't create SCTP socket until it has. bool started_ = false; // Are we ready to queue data (SCTP socket created, and not blocked due to - // congestion control)? Different than |transport_channel_|'s "ready to - // send". + // congestion control)? Different than |transport_|'s "ready to send". bool ready_to_send_data_ = false; typedef std::set StreamSet; @@ -179,9 +178,9 @@ class SctpTransportFactory : public SctpTransportInternalFactory { : network_thread_(network_thread) {} std::unique_ptr CreateSctpTransport( - rtc::PacketTransportInternal* channel) override { + rtc::PacketTransportInternal* transport) override { return std::unique_ptr( - new SctpTransport(network_thread_, channel)); + new SctpTransport(network_thread_, transport)); } private: diff --git a/media/sctp/sctptransport_unittest.cc b/media/sctp/sctptransport_unittest.cc index 32e439a188..9864d7af0f 100644 --- a/media/sctp/sctptransport_unittest.cc +++ b/media/sctp/sctptransport_unittest.cc @@ -238,16 +238,16 @@ class SctpTransportTest : public testing::Test, public sigslot::has_slots<> { }; // Test that data can be sent end-to-end when an SCTP transport starts with one -// transport channel (which is unwritable), and then switches to another -// channel. A common scenario due to how BUNDLE works. -TEST_F(SctpTransportTest, SwitchTransportChannel) { +// transport (which is unwritable), and then switches to another transport. A +// common scenario due to how BUNDLE works. +TEST_F(SctpTransportTest, SwitchDtlsTransport) { FakeDtlsTransport black_hole("black hole", 0); FakeDtlsTransport fake_dtls1("fake dtls 1", 0); FakeDtlsTransport fake_dtls2("fake dtls 2", 0); SctpFakeDataReceiver recv1; SctpFakeDataReceiver recv2; - // Construct transport1 with the "black hole" channel. + // Construct transport1 with the "black hole" transport. std::unique_ptr transport1( CreateTransport(&black_hole, &recv1)); std::unique_ptr transport2( @@ -261,10 +261,10 @@ TEST_F(SctpTransportTest, SwitchTransportChannel) { transport1->Start(kTransport1Port, kTransport2Port); transport2->Start(kTransport2Port, kTransport1Port); - // Switch transport1_ to the normal fake_dtls1_ channel. - transport1->SetTransportChannel(&fake_dtls1); + // Switch transport1_ to the normal fake_dtls1_ transport. + transport1->SetDtlsTransport(&fake_dtls1); - // Connect the two fake DTLS channels. + // Connect the two fake DTLS transports. bool asymmetric = false; fake_dtls1.SetDestination(&fake_dtls2, asymmetric); @@ -274,6 +274,10 @@ TEST_F(SctpTransportTest, SwitchTransportChannel) { ASSERT_TRUE(SendData(transport2.get(), 1, "bar", &result)); EXPECT_TRUE_WAIT(ReceivedData(&recv2, 1, "foo"), kDefaultTimeout); EXPECT_TRUE_WAIT(ReceivedData(&recv1, 1, "bar"), kDefaultTimeout); + + // Setting a null DtlsTransport should work. This could happen when an SCTP + // data section is rejected. + transport1->SetDtlsTransport(nullptr); } // Calling Start twice shouldn't do anything bad, if with the same parameters. @@ -317,7 +321,7 @@ TEST_F(SctpTransportTest, NegativeOnePortTreatedAsDefault) { transport1->Start(kSctpDefaultPort, kSctpDefaultPort); transport2->Start(-1, -1); - // Connect the two fake DTLS channels. + // Connect the two fake DTLS transports. bool asymmetric = false; fake_dtls1.SetDestination(&fake_dtls2, asymmetric); @@ -347,7 +351,7 @@ TEST_F(SctpTransportTest, ResetStreamWithAlreadyResetStreamFails) { } // Test that SignalReadyToSendData is fired after Start has been called and the -// DTLS channel is writable. +// DTLS transport is writable. TEST_F(SctpTransportTest, SignalReadyToSendDataAfterDtlsWritable) { FakeDtlsTransport fake_dtls("fake dtls", 0); SctpFakeDataReceiver recv; diff --git a/media/sctp/sctptransportinternal.h b/media/sctp/sctptransportinternal.h index c8c606ed11..9168320b2b 100644 --- a/media/sctp/sctptransportinternal.h +++ b/media/sctp/sctptransportinternal.h @@ -48,17 +48,15 @@ constexpr uint16_t kMinSctpSid = 0; // usrsctp.h) const int kSctpDefaultPort = 5000; -// Abstract SctpTransport interface for use internally (by -// PeerConnection/WebRtcSession/etc.). Exists to allow mock/fake SctpTransports -// to be created. +// Abstract SctpTransport interface for use internally (by PeerConnection etc.). +// Exists to allow mock/fake SctpTransports to be created. class SctpTransportInternal { public: virtual ~SctpTransportInternal() {} - // Changes what underlying DTLS channel is uses. Used when switching which + // Changes what underlying DTLS transport is uses. Used when switching which // bundled transport the SctpTransport uses. - // Assumes |channel| is non-null. - virtual void SetTransportChannel(rtc::PacketTransportInternal* channel) = 0; + virtual void SetDtlsTransport(rtc::PacketTransportInternal* transport) = 0; // When Start is called, connects as soon as possible; this can be called // before DTLS completes, in which case the connection will begin when DTLS diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index f1e5ac2baa..74f8872de6 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -5533,10 +5533,10 @@ Call::Stats PeerConnection::GetCallStats() { bool PeerConnection::CreateSctpTransport_n(const std::string& mid) { RTC_DCHECK(network_thread()->IsCurrent()); RTC_DCHECK(sctp_factory_); - cricket::DtlsTransportInternal* tc = + cricket::DtlsTransportInternal* dtls_transport = transport_controller_->GetDtlsTransport(mid); - RTC_DCHECK(tc); - sctp_transport_ = sctp_factory_->CreateSctpTransport(tc); + RTC_DCHECK(dtls_transport); + sctp_transport_ = sctp_factory_->CreateSctpTransport(dtls_transport); RTC_DCHECK(sctp_transport_); sctp_invoker_.reset(new rtc::AsyncInvoker()); sctp_transport_->SignalReadyToSendData.connect( @@ -5546,7 +5546,7 @@ bool PeerConnection::CreateSctpTransport_n(const std::string& mid) { sctp_transport_->SignalStreamClosedRemotely.connect( this, &PeerConnection::OnSctpStreamClosedRemotely_n); sctp_mid_ = mid; - sctp_transport_->SetTransportChannel(tc); + sctp_transport_->SetDtlsTransport(dtls_transport); return true; } @@ -6122,7 +6122,7 @@ void PeerConnection::OnDtlsTransportChanged( cricket::DtlsTransportInternal* dtls_transport) { if (sctp_transport_) { RTC_DCHECK(mid == sctp_mid_); - sctp_transport_->SetTransportChannel(dtls_transport); + sctp_transport_->SetDtlsTransport(dtls_transport); } } diff --git a/pc/peerconnectioninterface_unittest.cc b/pc/peerconnectioninterface_unittest.cc index 9242eeaff5..c040353383 100644 --- a/pc/peerconnectioninterface_unittest.cc +++ b/pc/peerconnectioninterface_unittest.cc @@ -2258,8 +2258,8 @@ TEST_P(PeerConnectionInterfaceTest, DataChannelCloseWhenPeerConnectionClose) { EXPECT_EQ(DataChannelInterface::kClosed, data2->state()); } -// This test that data channels can be rejected in an answer. -TEST_P(PeerConnectionInterfaceTest, TestRejectDataChannelInAnswer) { +// This tests that RTP data channels can be rejected in an answer. +TEST_P(PeerConnectionInterfaceTest, TestRejectRtpDataChannelInAnswer) { FakeConstraints constraints; constraints.SetAllowRtpDataChannels(); CreatePeerConnection(&constraints); @@ -2283,6 +2283,35 @@ TEST_P(PeerConnectionInterfaceTest, TestRejectDataChannelInAnswer) { EXPECT_EQ(DataChannelInterface::kClosed, offer_channel->state()); } +#ifdef HAVE_SCTP +// This tests that SCTP data channels can be rejected in an answer. +TEST_P(PeerConnectionInterfaceTest, TestRejectSctpDataChannelInAnswer) +#else +TEST_P(PeerConnectionInterfaceTest, DISABLED_TestRejectSctpDataChannelInAnswer) +#endif +{ + FakeConstraints constraints; + CreatePeerConnection(&constraints); + + rtc::scoped_refptr offer_channel( + pc_->CreateDataChannel("offer_channel", NULL)); + + CreateOfferAsLocalDescription(); + + // Create an answer where the m-line for data channels are rejected. + std::string sdp; + EXPECT_TRUE(pc_->local_description()->ToString(&sdp)); + std::unique_ptr answer( + webrtc::CreateSessionDescription(SdpType::kAnswer, sdp)); + ASSERT_TRUE(answer); + cricket::ContentInfo* data_info = + cricket::GetFirstDataContent(answer->description()); + data_info->rejected = true; + + DoSetRemoteDescription(std::move(answer)); + EXPECT_EQ(DataChannelInterface::kClosed, offer_channel->state()); +} + // Test that we can create a session description from an SDP string from // FireFox, use it as a remote session description, generate an answer and use // the answer as a local description. diff --git a/pc/test/fakesctptransport.h b/pc/test/fakesctptransport.h index 17143ed66c..002caa6213 100644 --- a/pc/test/fakesctptransport.h +++ b/pc/test/fakesctptransport.h @@ -20,7 +20,7 @@ // local/remote ports. class FakeSctpTransport : public cricket::SctpTransportInternal { public: - void SetTransportChannel(rtc::PacketTransportInternal* channel) override {} + void SetDtlsTransport(rtc::PacketTransportInternal* transport) override {} bool Start(int local_port, int remote_port) override { local_port_.emplace(local_port); remote_port_.emplace(remote_port);