diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h index 09317b828b..3ada740f18 100644 --- a/api/peer_connection_interface.h +++ b/api/peer_connection_interface.h @@ -1003,6 +1003,16 @@ class RTC_EXPORT PeerConnectionInterface : public rtc::RefCountInterface { virtual void SetRemoteDescription(SetSessionDescriptionObserver* observer, SessionDescriptionInterface* desc) {} + // According to spec, we must only fire "negotiationneeded" if the Operations + // Chain is empty. This method takes care of validating an event previously + // generated with PeerConnectionObserver::OnNegotiationNeededEvent() to make + // sure that even if there was a delay (e.g. due to a PostTask) between the + // event being generated and the time of firing, the Operations Chain is empty + // and the event is still valid to be fired. + virtual bool ShouldFireNegotiationNeededEvent(uint32_t event_id) { + return true; + } + virtual PeerConnectionInterface::RTCConfiguration GetConfiguration() = 0; // Sets the PeerConnection's global configuration to |config|. @@ -1176,7 +1186,17 @@ class PeerConnectionObserver { // Triggered when renegotiation is needed. For example, an ICE restart // has begun. - virtual void OnRenegotiationNeeded() = 0; + // TODO(hbos): Delete in favor of OnNegotiationNeededEvent() when downstream + // projects have migrated. + virtual void OnRenegotiationNeeded() {} + // Used to fire spec-compliant onnegotiationneeded events, which should only + // fire when the Operations Chain is empty. The observer is responsible for + // queuing a task (e.g. Chromium: jump to main thread) to maybe fire the + // event. The event identified using |event_id| must only fire if + // PeerConnection::ShouldFireNegotiationNeededEvent() returns true since it is + // possible for the event to become invalidated by operations subsequently + // chained. + virtual void OnNegotiationNeededEvent(uint32_t event_id) {} // Called any time the legacy IceConnectionState changes. // diff --git a/api/peer_connection_proxy.h b/api/peer_connection_proxy.h index 0cc3b3b8e7..2d4cb5cad0 100644 --- a/api/peer_connection_proxy.h +++ b/api/peer_connection_proxy.h @@ -116,6 +116,7 @@ PROXY_METHOD2(void, SetRemoteDescription, SetSessionDescriptionObserver*, SessionDescriptionInterface*) +PROXY_METHOD1(bool, ShouldFireNegotiationNeededEvent, uint32_t) PROXY_METHOD0(PeerConnectionInterface::RTCConfiguration, GetConfiguration) PROXY_METHOD1(RTCError, SetConfiguration, diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 6a5ad95291..34e638f3cf 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -1038,7 +1038,14 @@ PeerConnection::PeerConnection(PeerConnectionFactory* factory, call_ptr_(call_.get()), local_ice_credentials_to_replace_(new LocalIceCredentialsToReplace()), data_channel_controller_(this), - weak_ptr_factory_(this) {} + weak_ptr_factory_(this) { + operations_chain_->SetOnChainEmptyCallback( + [this_weak_ptr = weak_ptr_factory_.GetWeakPtr()]() { + if (!this_weak_ptr) + return; + this_weak_ptr->OnOperationsChainEmpty(); + }); +} PeerConnection::~PeerConnection() { TRACE_EVENT0("webrtc", "PeerConnection::~PeerConnection"); @@ -2638,18 +2645,24 @@ void PeerConnection::DoSetLocalDescription( ReportNegotiatedSdpSemantics(*local_description()); } + observer->OnSetLocalDescriptionComplete(RTCError::OK()); + NoteUsageEvent(UsageEvent::SET_LOCAL_DESCRIPTION_SUCCEEDED); + + // Check if negotiation is needed. We must do this after informing the + // observer that SetLocalDescription() has completed to ensure negotiation is + // not needed prior to the promise resolving. if (IsUnifiedPlan()) { bool was_negotiation_needed = is_negotiation_needed_; UpdateNegotiationNeeded(); if (signaling_state() == kStable && was_negotiation_needed && is_negotiation_needed_) { + // Legacy version. Observer()->OnRenegotiationNeeded(); + // Spec-compliant version; the event may get invalidated before firing. + GenerateNegotiationNeededEvent(); } } - observer->OnSetLocalDescriptionComplete(RTCError::OK()); - NoteUsageEvent(UsageEvent::SET_LOCAL_DESCRIPTION_SUCCEEDED); - // MaybeStartGathering needs to be called after informing the observer so that // we don't signal any candidates before signaling that SetLocalDescription // completed. @@ -3098,17 +3111,23 @@ void PeerConnection::DoSetRemoteDescription( ReportNegotiatedSdpSemantics(*remote_description()); } + observer->OnSetRemoteDescriptionComplete(RTCError::OK()); + NoteUsageEvent(UsageEvent::SET_REMOTE_DESCRIPTION_SUCCEEDED); + + // Check if negotiation is needed. We must do this after informing the + // observer that SetRemoteDescription() has completed to ensure negotiation is + // not needed prior to the promise resolving. if (IsUnifiedPlan()) { bool was_negotiation_needed = is_negotiation_needed_; UpdateNegotiationNeeded(); if (signaling_state() == kStable && was_negotiation_needed && is_negotiation_needed_) { + // Legacy version. Observer()->OnRenegotiationNeeded(); + // Spec-compliant version; the event may get invalidated before firing. + GenerateNegotiationNeededEvent(); } } - - observer->OnSetRemoteDescriptionComplete(RTCError::OK()); - NoteUsageEvent(UsageEvent::SET_REMOTE_DESCRIPTION_SUCCEEDED); } RTCError PeerConnection::ApplyRemoteDescription( @@ -7310,9 +7329,22 @@ void PeerConnection::UpdateNegotiationNeeded() { RTC_DCHECK_RUN_ON(signaling_thread()); if (!IsUnifiedPlan()) { Observer()->OnRenegotiationNeeded(); + GenerateNegotiationNeededEvent(); return; } + // In the spec, a task is queued here to run the following steps - this is + // meant to ensure we do not fire onnegotiationneeded prematurely if multiple + // changes are being made at once. In order to support Chromium's + // implementation where the JavaScript representation of the PeerConnection + // lives on a separate thread though, the queuing of a task is instead + // performed by the PeerConnectionObserver posting from the signaling thread + // to the JavaScript main thread that negotiation is needed. And because the + // Operations Chain lives on the WebRTC signaling thread, + // ShouldFireNegotiationNeededEvent() must be called before firing the event + // to ensure the Operations Chain is still empty and the event has not been + // invalidated. + // If connection's [[IsClosed]] slot is true, abort these steps. if (IsClosed()) return; @@ -7331,6 +7363,9 @@ void PeerConnection::UpdateNegotiationNeeded() { bool is_negotiation_needed = CheckIfNegotiationIsNeeded(); if (!is_negotiation_needed) { is_negotiation_needed_ = false; + // Invalidate any negotiation needed event that may previosuly have been + // generated. + ++negotiation_needed_event_id_; return; } @@ -7347,6 +7382,10 @@ void PeerConnection::UpdateNegotiationNeeded() { // If connection's [[NegotiationNeeded]] slot is false, abort these steps. // Fire an event named negotiationneeded at connection. Observer()->OnRenegotiationNeeded(); + // Fire the spec-compliant version; when ShouldFireNegotiationNeededEvent() is + // used in the task queued by the observer, this event will only fire when the + // chain is empty. + GenerateNegotiationNeededEvent(); } bool PeerConnection::CheckIfNegotiationIsNeeded() { @@ -7488,6 +7527,59 @@ bool PeerConnection::CheckIfNegotiationIsNeeded() { return false; } +void PeerConnection::OnOperationsChainEmpty() { + RTC_DCHECK_RUN_ON(signaling_thread()); + if (IsClosed() || !update_negotiation_needed_on_empty_chain_) + return; + update_negotiation_needed_on_empty_chain_ = false; + // Firing when chain is empty is only supported in Unified Plan to avoid Plan + // B regressions. (In Plan B, onnegotiationneeded is already broken anyway, so + // firing it even more might just be confusing.) + if (IsUnifiedPlan()) { + UpdateNegotiationNeeded(); + } +} + +bool PeerConnection::ShouldFireNegotiationNeededEvent(uint32_t event_id) { + RTC_DCHECK_RUN_ON(signaling_thread()); + // Plan B? Always fire to conform with useless legacy behavior. + if (!IsUnifiedPlan()) { + return true; + } + // The event ID has been invalidated. Either negotiation is no longer needed + // or a newer negotiation needed event has been generated. + if (event_id != negotiation_needed_event_id_) { + return false; + } + // The chain is no longer empty, update negotiation needed when it becomes + // empty. This should generate a newer negotiation needed event, making this + // one obsolete. + if (!operations_chain_->IsEmpty()) { + // Since we just suppressed an event that would have been fired, if + // negotiation is still needed by the time the chain becomes empty again, we + // must make sure to generate another event if negotiation is needed then. + // This happens when |is_negotiation_needed_| goes from false to true, so we + // set it to false until UpdateNegotiationNeeded() is called. + is_negotiation_needed_ = false; + update_negotiation_needed_on_empty_chain_ = true; + return false; + } + // We must not fire if the signaling state is no longer "stable". If + // negotiation is still needed when we return to "stable", a new negotiation + // needed event will be generated, so this one can safely be suppressed. + if (signaling_state_ != PeerConnectionInterface::kStable) { + return false; + } + // All checks have passed - please fire "negotiationneeded" now! + return true; +} + +void PeerConnection::GenerateNegotiationNeededEvent() { + RTC_DCHECK_RUN_ON(signaling_thread()); + ++negotiation_needed_event_id_; + Observer()->OnNegotiationNeededEvent(negotiation_needed_event_id_); +} + RTCError PeerConnection::Rollback(SdpType sdp_type) { auto state = signaling_state(); if (state != PeerConnectionInterface::kHaveLocalOffer && @@ -7574,7 +7666,10 @@ RTCError PeerConnection::Rollback(SdpType sdp_type) { if (sdp_type == SdpType::kRollback) { UpdateNegotiationNeeded(); if (is_negotiation_needed_) { + // Legacy version. Observer()->OnRenegotiationNeeded(); + // Spec-compliant version; the event may get invalidated before firing. + GenerateNegotiationNeededEvent(); } } return RTCError::OK(); diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 41cb68c645..d33c658e9d 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -331,6 +331,8 @@ class PeerConnection : public PeerConnectionInternal, // Handler for the "channel closed" signal void OnSctpDataChannelClosed(DataChannelInterface* channel); + bool ShouldFireNegotiationNeededEvent(uint32_t event_id) override; + // Functions made public for testing. void ReturnHistogramVeryQuicklyForTesting() { RTC_DCHECK_RUN_ON(signaling_thread()); @@ -1134,6 +1136,8 @@ class PeerConnection : public PeerConnectionInternal, void UpdateNegotiationNeeded(); bool CheckIfNegotiationIsNeeded(); + void OnOperationsChainEmpty(); + void GenerateNegotiationNeededEvent(); // | sdp_type | is the type of the SDP that caused the rollback. RTCError Rollback(SdpType sdp_type); @@ -1332,6 +1336,9 @@ class PeerConnection : public PeerConnectionInternal, std::unique_ptr local_ice_credentials_to_replace_ RTC_GUARDED_BY(signaling_thread()); bool is_negotiation_needed_ RTC_GUARDED_BY(signaling_thread()) = false; + bool update_negotiation_needed_on_empty_chain_ + RTC_GUARDED_BY(signaling_thread()) = false; + uint32_t negotiation_needed_event_id_ = 0; DataChannelController data_channel_controller_; rtc::WeakPtrFactory weak_ptr_factory_ diff --git a/pc/peer_connection_ice_unittest.cc b/pc/peer_connection_ice_unittest.cc index 8502dd427a..8c1a764398 100644 --- a/pc/peer_connection_ice_unittest.cc +++ b/pc/peer_connection_ice_unittest.cc @@ -1041,9 +1041,11 @@ TEST_P(PeerConnectionIceTest, RestartIceCausesNegotiationNeeded) { auto callee = CreatePeerConnectionWithAudioVideo(); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); - caller->observer()->clear_negotiation_needed(); + caller->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); caller->pc()->RestartIce(); - EXPECT_TRUE(caller->observer()->negotiation_needed()); + EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); } // In Unified Plan, "onnegotiationneeded" is spec-compliant, including not @@ -1064,14 +1066,17 @@ TEST_F(PeerConnectionIceTestUnifiedPlan, 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->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); 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()); + EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_FALSE(caller->observer()->has_negotiation_needed_event()); ASSERT_TRUE( caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); - EXPECT_TRUE(caller->observer()->negotiation_needed()); + EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); } TEST_F(PeerConnectionIceTestUnifiedPlan, @@ -1084,14 +1089,17 @@ TEST_F(PeerConnectionIceTestUnifiedPlan, 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->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); 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()); + EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_FALSE(caller->observer()->has_negotiation_needed_event()); ASSERT_TRUE( callee->SetRemoteDescription(caller->CreateAnswerAndSetAsLocal())); - EXPECT_TRUE(caller->observer()->negotiation_needed()); + EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); } TEST_F(PeerConnectionIceTestUnifiedPlan, @@ -1102,14 +1110,16 @@ TEST_F(PeerConnectionIceTestUnifiedPlan, ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); // Local restart. caller->pc()->RestartIce(); - caller->observer()->clear_negotiation_needed(); + caller->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); // 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()); + EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_FALSE(caller->observer()->has_negotiation_needed_event()); } TEST_F(PeerConnectionIceTestUnifiedPlan, @@ -1119,10 +1129,13 @@ TEST_F(PeerConnectionIceTestUnifiedPlan, ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); caller->pc()->RestartIce(); - EXPECT_TRUE(caller->observer()->negotiation_needed()); - caller->observer()->clear_negotiation_needed(); + EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); + caller->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); caller->pc()->RestartIce(); - EXPECT_FALSE(caller->observer()->negotiation_needed()); + EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_FALSE(caller->observer()->has_negotiation_needed_event()); } // In Plan B, "onnegotiationneeded" is not spec-compliant, firing based on if @@ -1140,15 +1153,19 @@ TEST_F(PeerConnectionIceTestPlanB, auto callee = CreatePeerConnectionWithAudioVideo(); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - caller->observer()->clear_negotiation_needed(); + caller->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); caller->pc()->RestartIce(); - EXPECT_TRUE(caller->observer()->negotiation_needed()); - caller->observer()->clear_negotiation_needed(); + EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); + caller->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); 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()); + EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_FALSE(caller->observer()->has_negotiation_needed_event()); } TEST_F(PeerConnectionIceTestPlanB, @@ -1157,15 +1174,19 @@ TEST_F(PeerConnectionIceTestPlanB, auto callee = CreatePeerConnectionWithAudioVideo(); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); - caller->observer()->clear_negotiation_needed(); + caller->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); caller->pc()->RestartIce(); - EXPECT_TRUE(caller->observer()->negotiation_needed()); - caller->observer()->clear_negotiation_needed(); + EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); + caller->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); 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()); + EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); } // The following parameterized test verifies that if an offer is sent with a diff --git a/pc/peer_connection_jsep_unittest.cc b/pc/peer_connection_jsep_unittest.cc index c4b7de10f7..b7c07598cf 100644 --- a/pc/peer_connection_jsep_unittest.cc +++ b/pc/peer_connection_jsep_unittest.cc @@ -1791,7 +1791,8 @@ TEST_F(PeerConnectionJsepTest, RollbackImplicitly) { EXPECT_EQ(callee->signaling_state(), PeerConnectionInterface::kHaveRemoteOffer); EXPECT_TRUE(callee->CreateAnswerAndSetAsLocal()); - EXPECT_FALSE(callee->observer()->negotiation_needed()); + EXPECT_FALSE(callee->observer()->legacy_renegotiation_needed()); + EXPECT_FALSE(callee->observer()->has_negotiation_needed_event()); } TEST_F(PeerConnectionJsepTest, RollbackImplicitlyNegotatiationNotNeeded) { @@ -1803,13 +1804,15 @@ TEST_F(PeerConnectionJsepTest, RollbackImplicitlyNegotatiationNotNeeded) { caller->AddAudioTrack("a"); callee->AddAudioTrack("b"); EXPECT_TRUE(callee->CreateOfferAndSetAsLocal()); - callee->observer()->clear_negotiation_needed(); + callee->observer()->clear_legacy_renegotiation_needed(); + callee->observer()->clear_latest_negotiation_needed_event(); EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); EXPECT_EQ(callee->signaling_state(), PeerConnectionInterface::kHaveRemoteOffer); EXPECT_TRUE(callee->CreateAnswerAndSetAsLocal()); // No negotiation needed as track got attached in the answer. - EXPECT_FALSE(callee->observer()->negotiation_needed()); + EXPECT_FALSE(callee->observer()->legacy_renegotiation_needed()); + EXPECT_FALSE(callee->observer()->has_negotiation_needed_event()); EXPECT_EQ(callee->observer()->remove_track_events_.size(), 0u); } @@ -1821,13 +1824,16 @@ TEST_F(PeerConnectionJsepTest, RollbackImplicitlyAndNegotiationNeeded) { auto callee = CreatePeerConnection(config); callee->AddAudioTrack("a"); EXPECT_TRUE(callee->CreateOfferAndSetAsLocal()); - callee->observer()->clear_negotiation_needed(); + callee->observer()->clear_legacy_renegotiation_needed(); + callee->observer()->clear_latest_negotiation_needed_event(); EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); EXPECT_EQ(callee->signaling_state(), PeerConnectionInterface::kHaveRemoteOffer); - EXPECT_FALSE(callee->observer()->negotiation_needed()); + EXPECT_FALSE(callee->observer()->legacy_renegotiation_needed()); + EXPECT_FALSE(callee->observer()->has_negotiation_needed_event()); EXPECT_TRUE(callee->CreateAnswerAndSetAsLocal()); - EXPECT_TRUE(callee->observer()->negotiation_needed()); + EXPECT_TRUE(callee->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(callee->observer()->has_negotiation_needed_event()); EXPECT_EQ(callee->observer()->remove_track_events_.size(), 0u); } @@ -1938,7 +1944,8 @@ TEST_F(PeerConnectionJsepTest, RollbackHasNoEffectOnStableTransceivers) { EXPECT_TRUE( caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); // In stable don't add or remove anything. - callee->observer()->clear_negotiation_needed(); + callee->observer()->clear_legacy_renegotiation_needed(); + callee->observer()->clear_latest_negotiation_needed_event(); size_t transceiver_count = callee->pc()->GetTransceivers().size(); auto mid_0 = callee->pc()->GetTransceivers()[0]->mid(); auto mid_1 = callee->pc()->GetTransceivers()[1]->mid(); @@ -1948,7 +1955,8 @@ TEST_F(PeerConnectionJsepTest, RollbackHasNoEffectOnStableTransceivers) { EXPECT_EQ(callee->pc()->GetTransceivers()[0]->mid(), mid_0); EXPECT_EQ(callee->pc()->GetTransceivers()[1]->mid(), mid_1); EXPECT_EQ(callee->observer()->remove_track_events_.size(), 0u); - EXPECT_FALSE(callee->observer()->negotiation_needed()); + EXPECT_FALSE(callee->observer()->legacy_renegotiation_needed()); + EXPECT_FALSE(callee->observer()->has_negotiation_needed_event()); } TEST_F(PeerConnectionJsepTest, ImplicitlyRollbackTransceiversWithSameMids) { @@ -2083,9 +2091,11 @@ TEST_F(PeerConnectionJsepTest, RollbackAfterMultipleSLD) { EXPECT_TRUE(callee->CreateOfferAndSetAsLocal()); callee->AddTransceiver(cricket::MEDIA_TYPE_VIDEO); EXPECT_TRUE(callee->CreateOfferAndSetAsLocal()); - callee->observer()->clear_negotiation_needed(); + callee->observer()->clear_legacy_renegotiation_needed(); + callee->observer()->clear_latest_negotiation_needed_event(); EXPECT_TRUE(callee->SetRemoteDescription(callee->CreateRollback())); - EXPECT_TRUE(callee->observer()->negotiation_needed()); + EXPECT_TRUE(callee->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(callee->observer()->has_negotiation_needed_event()); EXPECT_EQ(callee->pc()->GetTransceivers().size(), 2u); EXPECT_EQ(callee->pc()->GetTransceivers()[0]->mid(), absl::nullopt); EXPECT_EQ(callee->pc()->GetTransceivers()[1]->mid(), absl::nullopt); @@ -2134,7 +2144,8 @@ TEST_F(PeerConnectionJsepTest, DataChannelImplicitRollback) { EXPECT_TRUE(callee->CreateOfferAndSetAsLocal()); EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); EXPECT_TRUE(callee->CreateAnswerAndSetAsLocal()); - EXPECT_TRUE(callee->observer()->negotiation_needed()); + EXPECT_TRUE(callee->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(callee->observer()->has_negotiation_needed_event()); EXPECT_TRUE(callee->CreateOfferAndSetAsLocal()); } diff --git a/pc/peer_connection_rtp_unittest.cc b/pc/peer_connection_rtp_unittest.cc index e69a0882ae..152a12ead7 100644 --- a/pc/peer_connection_rtp_unittest.cc +++ b/pc/peer_connection_rtp_unittest.cc @@ -1145,12 +1145,15 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, RtpTransceiverInit init; init.direction = RtpTransceiverDirection::kInactive; auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, init); - EXPECT_TRUE(caller->observer()->negotiation_needed()); + EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); - caller->observer()->clear_negotiation_needed(); + caller->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); ASSERT_TRUE(caller->AddAudioTrack("a")); - EXPECT_TRUE(caller->observer()->negotiation_needed()); + EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); EXPECT_EQ(RtpTransceiverDirection::kSendOnly, transceiver->direction()); } @@ -1165,12 +1168,15 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, RtpTransceiverInit init; init.direction = RtpTransceiverDirection::kRecvOnly; auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, init); - EXPECT_TRUE(caller->observer()->negotiation_needed()); + EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); - caller->observer()->clear_negotiation_needed(); + caller->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); ASSERT_TRUE(caller->AddAudioTrack("a")); - EXPECT_TRUE(caller->observer()->negotiation_needed()); + EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); EXPECT_EQ(RtpTransceiverDirection::kSendRecv, transceiver->direction()); } @@ -1194,10 +1200,12 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, AddTrackErrorIfClosed) { auto audio_track = caller->CreateAudioTrack("a"); caller->pc()->Close(); - caller->observer()->clear_negotiation_needed(); + caller->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); auto result = caller->pc()->AddTrack(audio_track, std::vector()); EXPECT_EQ(RTCErrorType::INVALID_STATE, result.error().type()); - EXPECT_FALSE(caller->observer()->negotiation_needed()); + EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_FALSE(caller->observer()->has_negotiation_needed_event()); } TEST_F(PeerConnectionRtpTestUnifiedPlan, AddTrackErrorIfTrackAlreadyHasSender) { @@ -1206,10 +1214,12 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, AddTrackErrorIfTrackAlreadyHasSender) { auto audio_track = caller->CreateAudioTrack("a"); ASSERT_TRUE(caller->AddTrack(audio_track)); - caller->observer()->clear_negotiation_needed(); + caller->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); auto result = caller->pc()->AddTrack(audio_track, std::vector()); EXPECT_EQ(RTCErrorType::INVALID_PARAMETER, result.error().type()); - EXPECT_FALSE(caller->observer()->negotiation_needed()); + EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_FALSE(caller->observer()->has_negotiation_needed_event()); } // Unified Plan RemoveTrack tests. @@ -1236,13 +1246,16 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, init.direction = RtpTransceiverDirection::kSendRecv; auto transceiver = caller->AddTransceiver(caller->CreateAudioTrack("a"), init); - EXPECT_TRUE(caller->observer()->negotiation_needed()); + EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); - caller->observer()->clear_negotiation_needed(); + caller->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); ASSERT_TRUE(caller->pc()->RemoveTrack(transceiver->sender())); - EXPECT_TRUE(caller->observer()->negotiation_needed()); + EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); EXPECT_EQ(RtpTransceiverDirection::kRecvOnly, transceiver->direction()); } @@ -1258,13 +1271,16 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, init.direction = RtpTransceiverDirection::kSendOnly; auto transceiver = caller->AddTransceiver(caller->CreateAudioTrack("a"), init); - EXPECT_TRUE(caller->observer()->negotiation_needed()); + EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); - caller->observer()->clear_negotiation_needed(); + caller->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); ASSERT_TRUE(caller->pc()->RemoveTrack(transceiver->sender())); - EXPECT_TRUE(caller->observer()->negotiation_needed()); + EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); EXPECT_EQ(RtpTransceiverDirection::kInactive, transceiver->direction()); } @@ -1278,9 +1294,11 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, RemoveTrackWithNullSenderTrackIsNoOp) { auto transceiver = caller->pc()->GetTransceivers()[0]; ASSERT_TRUE(sender->SetTrack(nullptr)); - caller->observer()->clear_negotiation_needed(); + caller->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); ASSERT_TRUE(caller->pc()->RemoveTrack(sender)); - EXPECT_FALSE(caller->observer()->negotiation_needed()); + EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_FALSE(caller->observer()->has_negotiation_needed_event()); EXPECT_EQ(RtpTransceiverDirection::kSendRecv, transceiver->direction()); } @@ -1293,9 +1311,11 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, RemoveTrackErrorIfClosed) { auto sender = caller->AddAudioTrack("a"); caller->pc()->Close(); - caller->observer()->clear_negotiation_needed(); + caller->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); EXPECT_FALSE(caller->pc()->RemoveTrack(sender)); - EXPECT_FALSE(caller->observer()->negotiation_needed()); + EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_FALSE(caller->observer()->has_negotiation_needed_event()); } TEST_F(PeerConnectionRtpTestUnifiedPlan, @@ -1305,9 +1325,11 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, auto sender = caller->AddAudioTrack("a"); ASSERT_TRUE(caller->pc()->RemoveTrack(sender)); - caller->observer()->clear_negotiation_needed(); + caller->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); EXPECT_TRUE(caller->pc()->RemoveTrack(sender)); - EXPECT_FALSE(caller->observer()->negotiation_needed()); + EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_FALSE(caller->observer()->has_negotiation_needed_event()); } // Test that setting offers that add/remove/add a track repeatedly without @@ -1413,16 +1435,20 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, RenegotiationNeededAfterTransceiverSetDirection) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); - EXPECT_FALSE(caller->observer()->negotiation_needed()); + EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_FALSE(caller->observer()->has_negotiation_needed_event()); auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); - EXPECT_TRUE(caller->observer()->negotiation_needed()); + EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); - caller->observer()->clear_negotiation_needed(); + caller->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); transceiver->SetDirectionWithError(RtpTransceiverDirection::kInactive); - EXPECT_TRUE(caller->observer()->negotiation_needed()); + EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); } // Test that OnRenegotiationNeeded is not fired if SetDirection is called on an @@ -1433,9 +1459,11 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); - caller->observer()->clear_negotiation_needed(); + caller->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); transceiver->SetDirectionWithError(transceiver->direction()); - EXPECT_FALSE(caller->observer()->negotiation_needed()); + EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_FALSE(caller->observer()->has_negotiation_needed_event()); } // Test that OnRenegotiationNeeded is not fired if SetDirection is called on a @@ -1447,9 +1475,11 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); transceiver->StopInternal(); - caller->observer()->clear_negotiation_needed(); + caller->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); transceiver->SetDirectionWithError(RtpTransceiverDirection::kInactive); - EXPECT_FALSE(caller->observer()->negotiation_needed()); + EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_FALSE(caller->observer()->has_negotiation_needed_event()); } // Test that currentDirection returnes "stopped" if the transceiver was stopped. @@ -1829,13 +1859,16 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, init.direction = RtpTransceiverDirection::kSendRecv; auto transceiver = caller->AddTransceiver(caller->CreateAudioTrack("a"), init); - EXPECT_TRUE(caller->observer()->negotiation_needed()); + EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); - caller->observer()->clear_negotiation_needed(); + caller->observer()->clear_legacy_renegotiation_needed(); + caller->observer()->clear_latest_negotiation_needed_event(); transceiver->sender()->SetStreams({"stream3", "stream4", "stream5"}); - EXPECT_TRUE(caller->observer()->negotiation_needed()); + EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); auto callee_streams = callee->pc()->GetReceivers()[0]->streams(); diff --git a/pc/peer_connection_signaling_unittest.cc b/pc/peer_connection_signaling_unittest.cc index 72cbffbbc5..9a89fceab2 100644 --- a/pc/peer_connection_signaling_unittest.cc +++ b/pc/peer_connection_signaling_unittest.cc @@ -976,4 +976,68 @@ TEST_F(PeerConnectionSignalingUnifiedPlanTest, ASSERT_EQ(SignalingState::kStable, caller->signaling_state()); } +TEST_F(PeerConnectionSignalingUnifiedPlanTest, + ShouldFireNegotiationNeededWhenNoChangesArePending) { + auto caller = CreatePeerConnection(); + EXPECT_FALSE(caller->observer()->has_negotiation_needed_event()); + auto transceiver = + caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, RtpTransceiverInit()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); + EXPECT_TRUE(caller->pc()->ShouldFireNegotiationNeededEvent( + caller->observer()->latest_negotiation_needed_event())); +} + +TEST_F(PeerConnectionSignalingUnifiedPlanTest, + SuppressNegotiationNeededWhenOperationChainIsNotEmpty) { + auto caller = CreatePeerConnection(); + EXPECT_FALSE(caller->observer()->has_negotiation_needed_event()); + auto transceiver = + caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, RtpTransceiverInit()); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); + + rtc::scoped_refptr observer = + new rtc::RefCountedObject(); + caller->pc()->CreateOffer(observer, RTCOfferAnswerOptions()); + // For this test to work, the operation has to be pending, i.e. the observer + // has not yet been invoked. + EXPECT_FALSE(observer->called()); + // Because the Operations Chain is not empty, the event is now suppressed. + EXPECT_FALSE(caller->pc()->ShouldFireNegotiationNeededEvent( + caller->observer()->latest_negotiation_needed_event())); + caller->observer()->clear_latest_negotiation_needed_event(); + + // When the Operations Chain becomes empty again, a new negotiation needed + // event will be generated that is not suppressed. + EXPECT_TRUE_WAIT(observer->called(), kWaitTimeout); + EXPECT_TRUE(caller->observer()->has_negotiation_needed_event()); + EXPECT_TRUE(caller->pc()->ShouldFireNegotiationNeededEvent( + caller->observer()->latest_negotiation_needed_event())); +} + +TEST_F(PeerConnectionSignalingUnifiedPlanTest, + SuppressNegotiationNeededWhenSignalingStateIsNotStable) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + auto offer = caller->CreateOffer(RTCOfferAnswerOptions()); + + EXPECT_FALSE(caller->observer()->has_negotiation_needed_event()); + auto transceiver = + callee->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, RtpTransceiverInit()); + EXPECT_TRUE(callee->observer()->has_negotiation_needed_event()); + + // Change signaling state (to "have-remote-offer") by setting a remote offer. + callee->SetRemoteDescription(std::move(offer)); + // Because the signaling state is not "stable", the event is now suppressed. + EXPECT_FALSE(callee->pc()->ShouldFireNegotiationNeededEvent( + callee->observer()->latest_negotiation_needed_event())); + callee->observer()->clear_latest_negotiation_needed_event(); + + // Upon rolling back to "stable", a new negotiation needed event will be + // generated that is not suppressed. + callee->SetLocalDescription(CreateSessionDescription(SdpType::kRollback, "")); + EXPECT_TRUE(callee->observer()->has_negotiation_needed_event()); + EXPECT_TRUE(callee->pc()->ShouldFireNegotiationNeededEvent( + callee->observer()->latest_negotiation_needed_event())); +} + } // namespace webrtc diff --git a/pc/test/mock_peer_connection_observers.h b/pc/test/mock_peer_connection_observers.h index 0a835713a9..7766297843 100644 --- a/pc/test/mock_peer_connection_observers.h +++ b/pc/test/mock_peer_connection_observers.h @@ -85,6 +85,9 @@ class MockPeerConnectionObserver : public PeerConnectionObserver { remote_streams_->RemoveStream(stream); } void OnRenegotiationNeeded() override { renegotiation_needed_ = true; } + void OnNegotiationNeededEvent(uint32_t event_id) override { + latest_negotiation_needed_event_ = event_id; + } void OnDataChannel( rtc::scoped_refptr data_channel) override { last_datachannel_ = data_channel; @@ -214,8 +217,18 @@ class MockPeerConnectionObserver : public PeerConnectionObserver { return candidates; } - bool negotiation_needed() const { return renegotiation_needed_; } - void clear_negotiation_needed() { renegotiation_needed_ = false; } + bool legacy_renegotiation_needed() const { return renegotiation_needed_; } + void clear_legacy_renegotiation_needed() { renegotiation_needed_ = false; } + + bool has_negotiation_needed_event() { + return latest_negotiation_needed_event_.has_value(); + } + uint32_t latest_negotiation_needed_event() { + return latest_negotiation_needed_event_.value_or(0u); + } + void clear_latest_negotiation_needed_event() { + latest_negotiation_needed_event_ = absl::nullopt; + } rtc::scoped_refptr pc_; PeerConnectionInterface::SignalingState state_; @@ -223,6 +236,7 @@ class MockPeerConnectionObserver : public PeerConnectionObserver { rtc::scoped_refptr last_datachannel_; rtc::scoped_refptr remote_streams_; bool renegotiation_needed_ = false; + absl::optional latest_negotiation_needed_event_; bool ice_gathering_complete_ = false; bool ice_connected_ = false; bool callback_triggered_ = false; diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn index ffc19debc7..4eaedfd066 100644 --- a/rtc_base/BUILD.gn +++ b/rtc_base/BUILD.gn @@ -461,6 +461,7 @@ rtc_source_set("rtc_operations_chain") { "../api:scoped_refptr", "synchronization:sequence_checker", ] + absl_deps = [ "//third_party/abseil-cpp/absl/types:optional" ] } if (rtc_enable_libevent) { diff --git a/rtc_base/operations_chain.cc b/rtc_base/operations_chain.cc index 68ee20babc..189b2373fb 100644 --- a/rtc_base/operations_chain.cc +++ b/rtc_base/operations_chain.cc @@ -49,6 +49,17 @@ OperationsChain::~OperationsChain() { RTC_DCHECK(chained_operations_.empty()); } +void OperationsChain::SetOnChainEmptyCallback( + std::function on_chain_empty_callback) { + RTC_DCHECK_RUN_ON(&sequence_checker_); + on_chain_empty_callback_ = std::move(on_chain_empty_callback); +} + +bool OperationsChain::IsEmpty() const { + RTC_DCHECK_RUN_ON(&sequence_checker_); + return chained_operations_.empty(); +} + std::function OperationsChain::CreateOperationsChainCallback() { return [handle = rtc::scoped_refptr( new CallbackHandle(this))]() { handle->OnOperationComplete(); }; @@ -59,9 +70,12 @@ void OperationsChain::OnOperationComplete() { // The front element is the operation that just completed, remove it. RTC_DCHECK(!chained_operations_.empty()); chained_operations_.pop(); - // If there are any other operations chained, execute the next one. + // If there are any other operations chained, execute the next one. Otherwise, + // invoke the "on chain empty" callback if it has been set. if (!chained_operations_.empty()) { chained_operations_.front()->Run(); + } else if (on_chain_empty_callback_.has_value()) { + on_chain_empty_callback_.value()(); } } diff --git a/rtc_base/operations_chain.h b/rtc_base/operations_chain.h index b6ec46e04a..47fe132998 100644 --- a/rtc_base/operations_chain.h +++ b/rtc_base/operations_chain.h @@ -18,6 +18,7 @@ #include #include +#include "absl/types/optional.h" #include "api/scoped_refptr.h" #include "rtc_base/checks.h" #include "rtc_base/constructor_magic.h" @@ -112,6 +113,9 @@ class OperationsChain final : public RefCountedObject { static scoped_refptr Create(); ~OperationsChain(); + void SetOnChainEmptyCallback(std::function on_chain_empty_callback); + bool IsEmpty() const; + // Chains an operation. Chained operations are executed in FIFO order. The // operation starts when |functor| is executed by the OperationsChain and is // contractually obligated to invoke the callback passed to it when the @@ -181,6 +185,8 @@ class OperationsChain final : public RefCountedObject { // to it. std::queue> chained_operations_ RTC_GUARDED_BY(sequence_checker_); + absl::optional> on_chain_empty_callback_ + RTC_GUARDED_BY(sequence_checker_); RTC_DISALLOW_COPY_AND_ASSIGN(OperationsChain); }; diff --git a/rtc_base/operations_chain_unittest.cc b/rtc_base/operations_chain_unittest.cc index ed3c924998..988ad346af 100644 --- a/rtc_base/operations_chain_unittest.cc +++ b/rtc_base/operations_chain_unittest.cc @@ -10,6 +10,7 @@ #include "rtc_base/operations_chain.h" +#include #include #include #include @@ -120,6 +121,31 @@ class OperationTrackerProxy { return event; } + void SetOnChainEmptyCallback(std::function on_chain_empty_callback) { + Event event; + operations_chain_thread_->PostTask( + RTC_FROM_HERE, + [this, &event, + on_chain_empty_callback = std::move(on_chain_empty_callback)]() { + operations_chain_->SetOnChainEmptyCallback( + std::move(on_chain_empty_callback)); + event.Set(); + }); + event.Wait(Event::kForever); + } + + bool IsEmpty() { + Event event; + bool is_empty = false; + operations_chain_thread_->PostTask( + RTC_FROM_HERE, [this, &event, &is_empty]() { + is_empty = operations_chain_->IsEmpty(); + event.Set(); + }); + event.Wait(Event::kForever); + return is_empty; + } + std::unique_ptr ReleaseOperationChain() { std::unique_ptr event = std::make_unique(); operations_chain_thread_->PostTask(RTC_FROM_HERE, @@ -326,6 +352,87 @@ TEST(OperationsChainTest, OperationsAreExecutedInOrder) { operation6_completed_event.get())); } +TEST(OperationsChainTest, IsEmpty) { + OperationTrackerProxy operation_tracker_proxy; + operation_tracker_proxy.Initialize()->Wait(Event::kForever); + + // The chain is initially empty. + EXPECT_TRUE(operation_tracker_proxy.IsEmpty()); + // Chain a single event. + Event unblock_async_operation_event0; + auto async_operation_completed_event0 = + operation_tracker_proxy.PostAsynchronousOperation( + &unblock_async_operation_event0); + // The chain is not empty while an event is pending. + EXPECT_FALSE(operation_tracker_proxy.IsEmpty()); + // Completing the operation empties the chain. + unblock_async_operation_event0.Set(); + async_operation_completed_event0->Wait(Event::kForever); + EXPECT_TRUE(operation_tracker_proxy.IsEmpty()); + + // Chain multiple events. + Event unblock_async_operation_event1; + auto async_operation_completed_event1 = + operation_tracker_proxy.PostAsynchronousOperation( + &unblock_async_operation_event1); + Event unblock_async_operation_event2; + auto async_operation_completed_event2 = + operation_tracker_proxy.PostAsynchronousOperation( + &unblock_async_operation_event2); + // Again, the chain is not empty while an event is pending. + EXPECT_FALSE(operation_tracker_proxy.IsEmpty()); + // Upon completing the first event, the chain is still not empty. + unblock_async_operation_event1.Set(); + async_operation_completed_event1->Wait(Event::kForever); + EXPECT_FALSE(operation_tracker_proxy.IsEmpty()); + // Completing the last evenet empties the chain. + unblock_async_operation_event2.Set(); + async_operation_completed_event2->Wait(Event::kForever); + EXPECT_TRUE(operation_tracker_proxy.IsEmpty()); +} + +TEST(OperationsChainTest, OnChainEmptyCallback) { + OperationTrackerProxy operation_tracker_proxy; + operation_tracker_proxy.Initialize()->Wait(Event::kForever); + + std::atomic on_empty_callback_counter(0u); + operation_tracker_proxy.SetOnChainEmptyCallback( + [&on_empty_callback_counter] { ++on_empty_callback_counter; }); + + // Chain a single event. + Event unblock_async_operation_event0; + auto async_operation_completed_event0 = + operation_tracker_proxy.PostAsynchronousOperation( + &unblock_async_operation_event0); + // The callback is not invoked until the operation has completed. + EXPECT_EQ(0u, on_empty_callback_counter); + // Completing the operation empties the chain, invoking the callback. + unblock_async_operation_event0.Set(); + async_operation_completed_event0->Wait(Event::kForever); + EXPECT_EQ(1u, on_empty_callback_counter); + + // Chain multiple events. + Event unblock_async_operation_event1; + auto async_operation_completed_event1 = + operation_tracker_proxy.PostAsynchronousOperation( + &unblock_async_operation_event1); + Event unblock_async_operation_event2; + auto async_operation_completed_event2 = + operation_tracker_proxy.PostAsynchronousOperation( + &unblock_async_operation_event2); + // Again, the callback is not invoked until the operation has completed. + EXPECT_EQ(1u, on_empty_callback_counter); + // Upon completing the first event, the chain is still not empty, so the + // callback must not be invoked yet. + unblock_async_operation_event1.Set(); + async_operation_completed_event1->Wait(Event::kForever); + EXPECT_EQ(1u, on_empty_callback_counter); + // Completing the last evenet empties the chain, invoking the callback. + unblock_async_operation_event2.Set(); + async_operation_completed_event2->Wait(Event::kForever); + EXPECT_EQ(2u, on_empty_callback_counter); +} + TEST(OperationsChainTest, SafeToReleaseReferenceToOperationChainWhileOperationIsPending) { OperationTrackerProxy operation_tracker_proxy;