diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h index 69935d2fa9..8ffb8ae7a9 100644 --- a/api/peer_connection_interface.h +++ b/api/peer_connection_interface.h @@ -935,6 +935,14 @@ class RTC_EXPORT PeerConnectionInterface : public rtc::RefCountInterface { virtual const SessionDescriptionInterface* pending_local_description() const; virtual const SessionDescriptionInterface* pending_remote_description() const; + // Tells the PeerConnection that ICE should be restarted. This triggers a need + // for negotiation and subsequent CreateOffer() calls will act as if + // RTCOfferAnswerOptions::ice_restart is true. + // https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-restartice + // TODO(hbos): Remove default implementation when downstream projects + // implement this. + virtual void RestartIce() {} + // Create a new offer. // The CreateSessionDescriptionObserver callback will be called when done. virtual void CreateOffer(CreateSessionDescriptionObserver* observer, diff --git a/api/peer_connection_proxy.h b/api/peer_connection_proxy.h index 61ac6a1ca3..88e0d71749 100644 --- a/api/peer_connection_proxy.h +++ b/api/peer_connection_proxy.h @@ -86,6 +86,7 @@ PROXY_CONSTMETHOD0(const SessionDescriptionInterface*, pending_local_description) PROXY_CONSTMETHOD0(const SessionDescriptionInterface*, pending_remote_description) +PROXY_METHOD0(void, RestartIce) PROXY_METHOD2(void, CreateOffer, CreateSessionDescriptionObserver*, diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index e677e5d4df..3c03e392e6 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -690,6 +690,55 @@ const ContentInfo* FindTransceiverMSection( } // namespace +class PeerConnection::LocalIceCredentialsToReplace { + public: + // Sets the ICE credentials that need restarting to the ICE credentials of + // the current and pending descriptions. + void SetIceCredentialsFromLocalDescriptions( + const SessionDescriptionInterface* current_local_description, + const SessionDescriptionInterface* pending_local_description) { + ice_credentials_.clear(); + if (current_local_description) { + AppendIceCredentialsFromSessionDescription(*current_local_description); + } + if (pending_local_description) { + AppendIceCredentialsFromSessionDescription(*pending_local_description); + } + } + + void ClearIceCredentials() { ice_credentials_.clear(); } + + // Returns true if we have ICE credentials that need restarting. + bool HasIceCredentials() const { return !ice_credentials_.empty(); } + + // Returns true if |local_description| shares no ICE credentials with the + // ICE credentials that need restarting. + bool SatisfiesIceRestart( + const SessionDescriptionInterface& local_description) const { + for (const auto& transport_info : + local_description.description()->transport_infos()) { + if (ice_credentials_.find(std::make_pair( + transport_info.description.ice_ufrag, + transport_info.description.ice_pwd)) != ice_credentials_.end()) { + return false; + } + } + return true; + } + + private: + void AppendIceCredentialsFromSessionDescription( + const SessionDescriptionInterface& desc) { + for (const auto& transport_info : desc.description()->transport_infos()) { + ice_credentials_.insert( + std::make_pair(transport_info.description.ice_ufrag, + transport_info.description.ice_pwd)); + } + } + + std::set> ice_credentials_; +}; + // Upon completion, posts a task to execute the callback of the // SetSessionDescriptionObserver asynchronously on the same thread. At this // point, the state of the peer connection might no longer reflect the effects @@ -878,7 +927,8 @@ PeerConnection::PeerConnection(PeerConnectionFactory* factory, local_streams_(StreamCollection::Create()), remote_streams_(StreamCollection::Create()), call_(std::move(call)), - call_ptr_(call_.get()) {} + call_ptr_(call_.get()), + local_ice_credentials_to_replace_(new LocalIceCredentialsToReplace()) {} PeerConnection::~PeerConnection() { TRACE_EVENT0("webrtc", "PeerConnection::~PeerConnection"); @@ -1963,6 +2013,13 @@ rtc::scoped_refptr PeerConnection::CreateDataChannel( return DataChannelProxy::Create(signaling_thread(), channel.get()); } +void PeerConnection::RestartIce() { + RTC_DCHECK_RUN_ON(signaling_thread()); + local_ice_credentials_to_replace_->SetIceCredentialsFromLocalDescriptions( + current_local_description_.get(), pending_local_description_.get()); + UpdateNegotiationNeeded(); +} + void PeerConnection::CreateOffer(CreateSessionDescriptionObserver* observer, const RTCOfferAnswerOptions& options) { RTC_DCHECK_RUN_ON(signaling_thread()); @@ -2448,6 +2505,12 @@ RTCError PeerConnection::ApplyLocalDescription( } } + if (type == SdpType::kAnswer && + local_ice_credentials_to_replace_->SatisfiesIceRestart( + *current_local_description_)) { + local_ice_credentials_to_replace_->ClearIceCredentials(); + } + return RTCError::OK(); } @@ -2923,6 +2986,12 @@ RTCError PeerConnection::ApplyRemoteDescription( UpdateEndedRemoteMediaStreams(); } + if (type == SdpType::kAnswer && + local_ice_credentials_to_replace_->SatisfiesIceRestart( + *current_local_description_)) { + local_ice_credentials_to_replace_->ClearIceCredentials(); + } + return RTCError::OK(); } @@ -4313,8 +4382,10 @@ void PeerConnection::GetOptionsForOffer( } // Apply ICE restart flag and renomination flag. + bool ice_restart = offer_answer_options.ice_restart || + local_ice_credentials_to_replace_->HasIceCredentials(); for (auto& options : session_options->media_description_options) { - options.transport_options.ice_restart = offer_answer_options.ice_restart; + options.transport_options.ice_restart = ice_restart; options.transport_options.enable_ice_renomination = configuration_.enable_ice_renomination; } @@ -7311,19 +7382,24 @@ bool PeerConnection::CheckIfNegotiationIsNeeded() { // 1. If any implementation-specific negotiation is required, as described at // the start of this section, return true. - // 2. Let description be connection.[[CurrentLocalDescription]]. + // 2. If connection's [[RestartIce]] internal slot is true, return true. + if (local_ice_credentials_to_replace_->HasIceCredentials()) { + return true; + } + + // 3. Let description be connection.[[CurrentLocalDescription]]. const SessionDescriptionInterface* description = current_local_description(); if (!description) return true; - // 3. If connection has created any RTCDataChannels, and no m= section in + // 4. If connection has created any RTCDataChannels, and no m= section in // description has been negotiated yet for data, return true. if (!sctp_data_channels_.empty()) { if (!cricket::GetFirstDataContent(description->description()->contents())) return true; } - // 4. For each transceiver in connection's set of transceivers, perform the + // 5. For each transceiver in connection's set of transceivers, perform the // following checks: for (const auto& transceiver : transceivers_) { const ContentInfo* current_local_msection = @@ -7332,7 +7408,7 @@ bool PeerConnection::CheckIfNegotiationIsNeeded() { const ContentInfo* current_remote_msection = FindTransceiverMSection( transceiver.get(), current_remote_description()); - // 4.3 If transceiver is stopped and is associated with an m= section, + // 5.3 If transceiver is stopped and is associated with an m= section, // but the associated m= section is not yet rejected in // connection.[[CurrentLocalDescription]] or // connection.[[CurrentRemoteDescription]], return true. @@ -7345,17 +7421,17 @@ bool PeerConnection::CheckIfNegotiationIsNeeded() { continue; } - // 4.1 If transceiver isn't stopped and isn't yet associated with an m= + // 5.1 If transceiver isn't stopped and isn't yet associated with an m= // section in description, return true. if (!current_local_msection) return true; const MediaContentDescription* current_local_media_description = current_local_msection->media_description(); - // 4.2 If transceiver isn't stopped and is associated with an m= section + // 5.2 If transceiver isn't stopped and is associated with an m= section // in description then perform the following checks: - // 4.2.1 If transceiver.[[Direction]] is "sendrecv" or "sendonly", and the + // 5.2.1 If transceiver.[[Direction]] is "sendrecv" or "sendonly", and the // associated m= section in description either doesn't contain a single // "a=msid" line, or the number of MSIDs from the "a=msid" lines in this // m= section, or the MSID values themselves, differ from what is in @@ -7381,7 +7457,7 @@ bool PeerConnection::CheckIfNegotiationIsNeeded() { return true; } - // 4.2.2 If description is of type "offer", and the direction of the + // 5.2.2 If description is of type "offer", and the direction of the // associated m= section in neither connection.[[CurrentLocalDescription]] // nor connection.[[CurrentRemoteDescription]] matches // transceiver.[[Direction]], return true. @@ -7403,7 +7479,7 @@ bool PeerConnection::CheckIfNegotiationIsNeeded() { } } - // 4.2.3 If description is of type "answer", and the direction of the + // 5.2.3 If description is of type "answer", and the direction of the // associated m= section in the description does not match // transceiver.[[Direction]] intersected with the offered direction (as // described in [JSEP] (section 5.3.1.)), return true. diff --git a/pc/peer_connection.h b/pc/peer_connection.h index ed8c54cd6b..4e84b977d4 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -15,6 +15,7 @@ #include #include #include +#include #include #include "api/media_transport_interface.h" @@ -169,6 +170,8 @@ class PeerConnection : public PeerConnectionInternal, const SessionDescriptionInterface* pending_remote_description() const override; + void RestartIce() override; + // JSEP01 void CreateOffer(CreateSessionDescriptionObserver* observer, const RTCOfferAnswerOptions& options) override; @@ -287,6 +290,13 @@ class PeerConnection : public PeerConnectionInternal, private: class SetRemoteDescriptionObserverAdapter; friend class SetRemoteDescriptionObserverAdapter; + // Represents the [[LocalIceCredentialsToReplace]] internal slot in the spec. + // It makes the next CreateOffer() produce new ICE credentials even if + // RTCOfferAnswerOptions::ice_restart is false. + // https://w3c.github.io/webrtc-pc/#dfn-localufragstoreplace + // TODO(hbos): When JsepTransportController/JsepTransport supports rollback, + // move this type of logic to JsepTransportController/JsepTransport. + class LocalIceCredentialsToReplace; struct RtpSenderInfo { RtpSenderInfo() : first_ssrc(0) {} @@ -1366,6 +1376,8 @@ class PeerConnection : public PeerConnectionInternal, std::unique_ptr video_bitrate_allocator_factory_; + std::unique_ptr + local_ice_credentials_to_replace_ RTC_GUARDED_BY(signaling_thread()); bool is_negotiation_needed_ RTC_GUARDED_BY(signaling_thread()) = false; }; diff --git a/pc/peer_connection_ice_unittest.cc b/pc/peer_connection_ice_unittest.cc index 3b8b4db951..855cdf5cce 100644 --- a/pc/peer_connection_ice_unittest.cc +++ b/pc/peer_connection_ice_unittest.cc @@ -225,6 +225,26 @@ class PeerConnectionIceBaseTest : public ::testing::Test { return cricket::ICEROLE_UNKNOWN; } + // Returns a list of (ufrag, pwd) pairs in the order that they appear in + // |description|, or the empty list if |description| is null. + std::vector> GetIceCredentials( + const SessionDescriptionInterface* description) { + std::vector> ice_credentials; + if (!description) + return ice_credentials; + const auto* desc = description->description(); + for (const auto& content_info : desc->contents()) { + const auto* transport_info = + desc->GetTransportInfoByName(content_info.name); + if (transport_info) { + ice_credentials.push_back( + std::make_pair(transport_info->description.ice_ufrag, + transport_info->description.ice_pwd)); + } + } + return ice_credentials; + } + bool AddCandidateToFirstTransport(cricket::Candidate* candidate, SessionDescriptionInterface* sdesc) { auto* desc = sdesc->description(); @@ -813,6 +833,209 @@ TEST_P(PeerConnectionIceTest, LaterAnswerHasSameIceCredentialsIfNoIceRestart) { EXPECT_EQ(answer_transport_desc->ice_pwd, local_transport_desc->ice_pwd); } +TEST_P(PeerConnectionIceTest, RestartIceGeneratesNewCredentials) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + auto initial_ice_credentials = + GetIceCredentials(caller->pc()->local_description()); + caller->pc()->RestartIce(); + ASSERT_TRUE(caller->CreateOfferAndSetAsLocal()); + auto restarted_ice_credentials = + GetIceCredentials(caller->pc()->local_description()); + EXPECT_NE(initial_ice_credentials, restarted_ice_credentials); +} + +TEST_P(PeerConnectionIceTest, + RestartIceWhileLocalOfferIsPendingGeneratesNewCredentialsInNextOffer) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + auto initial_ice_credentials = + GetIceCredentials(caller->pc()->local_description()); + // ICE restart becomes needed while an O/A is pending and |caller| is the + // offerer. + caller->pc()->RestartIce(); + ASSERT_TRUE( + caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); + ASSERT_TRUE(caller->CreateOfferAndSetAsLocal()); + auto restarted_ice_credentials = + GetIceCredentials(caller->pc()->local_description()); + EXPECT_NE(initial_ice_credentials, restarted_ice_credentials); +} + +TEST_P(PeerConnectionIceTest, + RestartIceWhileRemoteOfferIsPendingGeneratesNewCredentialsInNextOffer) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + auto initial_ice_credentials = + GetIceCredentials(caller->pc()->local_description()); + ASSERT_TRUE(caller->SetRemoteDescription(callee->CreateOfferAndSetAsLocal())); + // ICE restart becomes needed while an O/A is pending and |caller| is the + // answerer. + caller->pc()->RestartIce(); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateAnswerAndSetAsLocal())); + ASSERT_TRUE(caller->CreateOfferAndSetAsLocal()); + auto restarted_ice_credentials = + GetIceCredentials(caller->pc()->local_description()); + EXPECT_NE(initial_ice_credentials, restarted_ice_credentials); +} + +TEST_P(PeerConnectionIceTest, RestartIceTriggeredByRemoteSide) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + auto initial_ice_credentials = + GetIceCredentials(caller->pc()->local_description()); + + // Remote restart and O/A exchange with |caller| as the answerer should + // restart ICE locally as well. + callee->pc()->RestartIce(); + ASSERT_TRUE(callee->ExchangeOfferAnswerWith(caller.get())); + + auto restarted_ice_credentials = + GetIceCredentials(caller->pc()->local_description()); + EXPECT_NE(initial_ice_credentials, restarted_ice_credentials); +} + +TEST_P(PeerConnectionIceTest, RestartIceCausesNegotiationNeeded) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + caller->observer()->clear_negotiation_needed(); + caller->pc()->RestartIce(); + EXPECT_TRUE(caller->observer()->negotiation_needed()); +} + +// In Unified Plan, "onnegotiationneeded" is spec-compliant, including not +// firing multipe times in a row, or firing when returning to the stable +// signaling state if negotiation is still needed. In Plan B it fires any time +// something changes. As such, some tests are SdpSemantics-specific. +class PeerConnectionIceTestUnifiedPlan : public PeerConnectionIceBaseTest { + protected: + PeerConnectionIceTestUnifiedPlan() + : PeerConnectionIceBaseTest(SdpSemantics::kUnifiedPlan) {} +}; + +TEST_F(PeerConnectionIceTestUnifiedPlan, + RestartIceWhileLocalOfferIsPendingCausesNegotiationNeededWhenStable) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + // ICE restart becomes needed while an O/A is pending and |caller| is the + // offerer. + caller->observer()->clear_negotiation_needed(); + caller->pc()->RestartIce(); + // In Unified Plan, the event should not fire until we are back in the stable + // signaling state. + EXPECT_FALSE(caller->observer()->negotiation_needed()); + ASSERT_TRUE( + caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); + EXPECT_TRUE(caller->observer()->negotiation_needed()); +} + +TEST_F(PeerConnectionIceTestUnifiedPlan, + RestartIceWhileRemoteOfferIsPendingCausesNegotiationNeededWhenStable) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + + // Establish initial credentials as the caller. + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + ASSERT_TRUE(caller->SetRemoteDescription(callee->CreateOfferAndSetAsLocal())); + // ICE restart becomes needed while an O/A is pending and |caller| is the + // answerer. + caller->observer()->clear_negotiation_needed(); + caller->pc()->RestartIce(); + // In Unified Plan, the event should not fire until we are back in the stable + // signaling state. + EXPECT_FALSE(caller->observer()->negotiation_needed()); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateAnswerAndSetAsLocal())); + EXPECT_TRUE(caller->observer()->negotiation_needed()); +} + +TEST_F(PeerConnectionIceTestUnifiedPlan, + RestartIceTriggeredByRemoteSideCauseNegotiationNotNeeded) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + // Local restart. + caller->pc()->RestartIce(); + caller->observer()->clear_negotiation_needed(); + // Remote restart and O/A exchange with |caller| as the answerer should + // restart ICE locally as well. + callee->pc()->RestartIce(); + ASSERT_TRUE(callee->ExchangeOfferAnswerWith(caller.get())); + // Having restarted ICE by the remote offer, we do not need to renegotiate ICE + // credentials when back in the stable signaling state. + EXPECT_FALSE(caller->observer()->negotiation_needed()); +} + +TEST_F(PeerConnectionIceTestUnifiedPlan, + RestartIceTwiceDoesNotFireNegotiationNeededTwice) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + caller->pc()->RestartIce(); + EXPECT_TRUE(caller->observer()->negotiation_needed()); + caller->observer()->clear_negotiation_needed(); + caller->pc()->RestartIce(); + EXPECT_FALSE(caller->observer()->negotiation_needed()); +} + +// In Plan B, "onnegotiationneeded" is not spec-compliant, firing based on if +// something changed rather than if negotiation is needed. In Unified Plan it +// fires according to spec. As such, some tests are SdpSemantics-specific. +class PeerConnectionIceTestPlanB : public PeerConnectionIceBaseTest { + protected: + PeerConnectionIceTestPlanB() + : PeerConnectionIceBaseTest(SdpSemantics::kPlanB) {} +}; + +TEST_F(PeerConnectionIceTestPlanB, + RestartIceWhileOfferIsPendingCausesNegotiationNeededImmediately) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + caller->observer()->clear_negotiation_needed(); + caller->pc()->RestartIce(); + EXPECT_TRUE(caller->observer()->negotiation_needed()); + caller->observer()->clear_negotiation_needed(); + ASSERT_TRUE( + caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); + // In Plan B, the event fired early so we don't expect it to fire now. This is + // not spec-compliant but follows the pattern of existing Plan B behavior. + EXPECT_FALSE(caller->observer()->negotiation_needed()); +} + +TEST_F(PeerConnectionIceTestPlanB, + RestartIceTwiceDoesFireNegotiationNeededTwice) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + caller->observer()->clear_negotiation_needed(); + caller->pc()->RestartIce(); + EXPECT_TRUE(caller->observer()->negotiation_needed()); + caller->observer()->clear_negotiation_needed(); + caller->pc()->RestartIce(); + // In Plan B, the event fires every time something changed, even if we have + // already fired the event. This is not spec-compliant but follows the same + // pattern of existing Plan B behavior. + EXPECT_TRUE(caller->observer()->negotiation_needed()); +} + // The following parameterized test verifies that if an offer is sent with a // modified ICE ufrag and/or ICE pwd, then the answer should identify that the // other side has initiated an ICE restart and generate a new ufrag and pwd. diff --git a/pc/test/fake_peer_connection_base.h b/pc/test/fake_peer_connection_base.h index f88eb1ebfc..e6ca0c9a9f 100644 --- a/pc/test/fake_peer_connection_base.h +++ b/pc/test/fake_peer_connection_base.h @@ -144,6 +144,8 @@ class FakePeerConnectionBase : public PeerConnectionInternal { return nullptr; } + void RestartIce() override {} + void CreateOffer(CreateSessionDescriptionObserver* observer, const RTCOfferAnswerOptions& options) override {}