[Perfect Negotiation] Implement non-racy version of SetLocalDescription.
BACKGROUND
When SLD is invoked with SetSessionDescriptionObserver, the observer is
called by posting a message back to the execution thread, delaying the
call. This delay is "artificial" - it's not necessary; the operation is
already complete. It's a post from the signaling thread to the signaling
thread. The rationale for the post was to avoid the observer making
recursive calls back into the PeerConnection. The problem with this is
that by the time the observer is called, the PeerConnection could
already have executed other operations and modified its states.
This causes the referenced bug: one can have a race where SLD is
resolved "too late" (after a pending SRD is executed) and the signaling
state observed when SLD resolves doesn't make sense.
When implementing Unified Plan, we fixed similar issues for SRD by
adding a version that takes SetRemoteDescriptionObserverInterface as
argument instead of SetSessionDescriptionObserver. The new version did
not have the delay. The old version had to be kept around not to break
downstream projects that had dependencies both on he delay and on
allowing the PC to be destroyed midst-operation without informing its
observers.
THIS CL
This does the old SRD fix for SLD as well: A new observer interface is
added, SetLocalDescriptionObserverInterface, and
PeerConnection::SetLocalDescription() is overloaded. If you call it with
the old observer, you get the delay, but if you call it with the new
observer, you don't get a delay.
- SetLocalDescriptionObserverInterface is added.
- SetLocalDescription is overloaded.
- The adapter for SetSessionDescriptionObserver that causes the delay
previously only used for SRD is updated to handle both SLD and SRD.
- FakeSetLocalDescriptionObserver is added and
MockSetRemoteDescriptionObserver is renamed "Fake...".
Bug: chromium:1071733
Change-Id: I920368e648bede481058ac22f5b8794752a220b3
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/179100
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31798}
diff --git a/pc/peer_connection_signaling_unittest.cc b/pc/peer_connection_signaling_unittest.cc
index 30b11ce..72cbffb 100644
--- a/pc/peer_connection_signaling_unittest.cc
+++ b/pc/peer_connection_signaling_unittest.cc
@@ -565,30 +565,102 @@
EXPECT_TRUE(observer->called());
}
-TEST_P(PeerConnectionSignalingTest, ImplicitCreateOfferAndShutdown) {
+TEST_P(PeerConnectionSignalingTest,
+ ImplicitCreateOfferAndShutdownWithOldObserver) {
auto caller = CreatePeerConnection();
auto observer = MockSetSessionDescriptionObserver::Create();
+ caller->pc()->SetLocalDescription(observer.get());
+ caller.reset(nullptr);
+ // The old observer does not get invoked because posted messages are lost.
+ EXPECT_FALSE(observer->called());
+}
+
+TEST_P(PeerConnectionSignalingTest, ImplicitCreateOfferAndShutdown) {
+ auto caller = CreatePeerConnection();
+ rtc::scoped_refptr<FakeSetLocalDescriptionObserver> observer(
+ new FakeSetLocalDescriptionObserver());
caller->pc()->SetLocalDescription(observer);
caller.reset(nullptr);
+ // The new observer gets invoked because it is called immediately.
+ EXPECT_TRUE(observer->called());
+ EXPECT_FALSE(observer->error().ok());
+}
+
+TEST_P(PeerConnectionSignalingTest,
+ CloseBeforeImplicitCreateOfferAndShutdownWithOldObserver) {
+ auto caller = CreatePeerConnection();
+ auto observer = MockSetSessionDescriptionObserver::Create();
+ caller->pc()->Close();
+ caller->pc()->SetLocalDescription(observer.get());
+ caller.reset(nullptr);
+ // The old observer does not get invoked because posted messages are lost.
EXPECT_FALSE(observer->called());
}
TEST_P(PeerConnectionSignalingTest, CloseBeforeImplicitCreateOfferAndShutdown) {
auto caller = CreatePeerConnection();
- auto observer = MockSetSessionDescriptionObserver::Create();
+ rtc::scoped_refptr<FakeSetLocalDescriptionObserver> observer(
+ new FakeSetLocalDescriptionObserver());
caller->pc()->Close();
caller->pc()->SetLocalDescription(observer);
caller.reset(nullptr);
+ // The new observer gets invoked because it is called immediately.
+ EXPECT_TRUE(observer->called());
+ EXPECT_FALSE(observer->error().ok());
+}
+
+TEST_P(PeerConnectionSignalingTest,
+ CloseAfterImplicitCreateOfferAndShutdownWithOldObserver) {
+ auto caller = CreatePeerConnection();
+ auto observer = MockSetSessionDescriptionObserver::Create();
+ caller->pc()->SetLocalDescription(observer.get());
+ caller->pc()->Close();
+ caller.reset(nullptr);
+ // The old observer does not get invoked because posted messages are lost.
EXPECT_FALSE(observer->called());
}
TEST_P(PeerConnectionSignalingTest, CloseAfterImplicitCreateOfferAndShutdown) {
auto caller = CreatePeerConnection();
- auto observer = MockSetSessionDescriptionObserver::Create();
+ rtc::scoped_refptr<FakeSetLocalDescriptionObserver> observer(
+ new FakeSetLocalDescriptionObserver());
caller->pc()->SetLocalDescription(observer);
caller->pc()->Close();
caller.reset(nullptr);
+ // The new observer gets invoked because it is called immediately.
+ EXPECT_TRUE(observer->called());
+ EXPECT_FALSE(observer->error().ok());
+}
+
+TEST_P(PeerConnectionSignalingTest,
+ SetLocalDescriptionNewObserverIsInvokedImmediately) {
+ auto caller = CreatePeerConnection();
+ auto offer = caller->CreateOffer(RTCOfferAnswerOptions());
+
+ rtc::scoped_refptr<FakeSetLocalDescriptionObserver> observer(
+ new FakeSetLocalDescriptionObserver());
+ caller->pc()->SetLocalDescription(std::move(offer), observer);
+ // The new observer is invoked immediately.
+ EXPECT_TRUE(observer->called());
+ EXPECT_TRUE(observer->error().ok());
+}
+
+TEST_P(PeerConnectionSignalingTest,
+ SetLocalDescriptionOldObserverIsInvokedInAPostedMessage) {
+ auto caller = CreatePeerConnection();
+ auto offer = caller->CreateOffer(RTCOfferAnswerOptions());
+
+ auto observer = MockSetSessionDescriptionObserver::Create();
+ caller->pc()->SetLocalDescription(observer, offer.release());
+ // The old observer is not invoked immediately.
EXPECT_FALSE(observer->called());
+ // Process all currently pending messages by waiting for a posted task to run.
+ bool checkpoint_reached = false;
+ rtc::Thread::Current()->PostTask(
+ RTC_FROM_HERE, [&checkpoint_reached] { checkpoint_reached = true; });
+ EXPECT_TRUE_WAIT(checkpoint_reached, kWaitTimeout);
+ // If resolving the observer was pending, it must now have been called.
+ EXPECT_TRUE(observer->called());
}
TEST_P(PeerConnectionSignalingTest, SetRemoteDescriptionExecutesImmediately) {
@@ -601,7 +673,7 @@
// By not waiting for the observer's callback we can verify that the operation
// executed immediately.
callee->pc()->SetRemoteDescription(std::move(offer),
- new MockSetRemoteDescriptionObserver());
+ new FakeSetRemoteDescriptionObserver());
EXPECT_EQ(2u, callee->pc()->GetReceivers().size());
}
@@ -620,7 +692,7 @@
// asynchronously, when CreateOffer() completes.
callee->pc()->CreateOffer(offer_observer, RTCOfferAnswerOptions());
callee->pc()->SetRemoteDescription(std::move(offer),
- new MockSetRemoteDescriptionObserver());
+ new FakeSetRemoteDescriptionObserver());
// CreateOffer() is asynchronous; without message processing this operation
// should not have completed.
EXPECT_FALSE(offer_observer->called());
@@ -639,7 +711,7 @@
auto caller = CreatePeerConnectionWithAudioVideo();
auto observer = MockSetSessionDescriptionObserver::Create();
- caller->pc()->SetLocalDescription(observer);
+ caller->pc()->SetLocalDescription(observer.get());
// The offer is created asynchronously; message processing is needed for it to
// complete.
@@ -665,7 +737,7 @@
EXPECT_EQ(PeerConnection::kHaveRemoteOffer, callee->signaling_state());
auto observer = MockSetSessionDescriptionObserver::Create();
- callee->pc()->SetLocalDescription(observer);
+ callee->pc()->SetLocalDescription(observer.get());
// The answer is created asynchronously; message processing is needed for it
// to complete.
@@ -687,28 +759,27 @@
auto callee = CreatePeerConnectionWithAudioVideo();
// SetLocalDescription(), implicitly creating an offer.
- rtc::scoped_refptr<MockSetSessionDescriptionObserver>
- caller_set_local_description_observer(
- new rtc::RefCountedObject<MockSetSessionDescriptionObserver>());
- caller->pc()->SetLocalDescription(caller_set_local_description_observer);
+ auto caller_set_local_description_observer =
+ MockSetSessionDescriptionObserver::Create();
+ caller->pc()->SetLocalDescription(
+ caller_set_local_description_observer.get());
EXPECT_TRUE_WAIT(caller_set_local_description_observer->called(),
kWaitTimeout);
ASSERT_TRUE(caller->pc()->pending_local_description());
// SetRemoteDescription(offer)
- rtc::scoped_refptr<MockSetSessionDescriptionObserver>
- callee_set_remote_description_observer(
- new rtc::RefCountedObject<MockSetSessionDescriptionObserver>());
+ auto callee_set_remote_description_observer =
+ MockSetSessionDescriptionObserver::Create();
callee->pc()->SetRemoteDescription(
- callee_set_remote_description_observer.get(),
+ callee_set_remote_description_observer,
CloneSessionDescription(caller->pc()->pending_local_description())
.release());
// SetLocalDescription(), implicitly creating an answer.
- rtc::scoped_refptr<MockSetSessionDescriptionObserver>
- callee_set_local_description_observer(
- new rtc::RefCountedObject<MockSetSessionDescriptionObserver>());
- callee->pc()->SetLocalDescription(callee_set_local_description_observer);
+ auto callee_set_local_description_observer =
+ MockSetSessionDescriptionObserver::Create();
+ callee->pc()->SetLocalDescription(
+ callee_set_local_description_observer.get());
EXPECT_TRUE_WAIT(callee_set_local_description_observer->called(),
kWaitTimeout);
// Chaining guarantees SetRemoteDescription() happened before
@@ -717,9 +788,8 @@
EXPECT_TRUE(callee->pc()->current_local_description());
// SetRemoteDescription(answer)
- rtc::scoped_refptr<MockSetSessionDescriptionObserver>
- caller_set_remote_description_observer(
- new rtc::RefCountedObject<MockSetSessionDescriptionObserver>());
+ auto caller_set_remote_description_observer =
+ MockSetSessionDescriptionObserver::Create();
caller->pc()->SetRemoteDescription(
caller_set_remote_description_observer,
CloneSessionDescription(callee->pc()->current_local_description())
@@ -737,7 +807,7 @@
auto observer = MockSetSessionDescriptionObserver::Create();
caller->pc()->Close();
- caller->pc()->SetLocalDescription(observer);
+ caller->pc()->SetLocalDescription(observer.get());
// The operation should fail asynchronously.
EXPECT_FALSE(observer->called());
@@ -756,7 +826,7 @@
auto caller = CreatePeerConnectionWithAudioVideo();
auto observer = MockSetSessionDescriptionObserver::Create();
- caller->pc()->SetLocalDescription(observer);
+ caller->pc()->SetLocalDescription(observer.get());
caller->pc()->Close();
// The operation should fail asynchronously.
@@ -788,14 +858,15 @@
// unique to Unified Plan, but the transceivers used to verify this are only
// available in Unified Plan.
TEST_F(PeerConnectionSignalingUnifiedPlanTest,
- SetLocalDescriptionExecutesImmediately) {
+ SetLocalDescriptionExecutesImmediatelyUsingOldObserver) {
auto caller = CreatePeerConnectionWithAudioVideo();
// This offer will cause transceiver mids to get assigned.
auto offer = caller->CreateOffer(RTCOfferAnswerOptions());
// By not waiting for the observer's callback we can verify that the operation
- // executed immediately.
+ // executed immediately. The old observer is invoked in a posted message, so
+ // waiting for it would not ensure synchronicity.
RTC_DCHECK(!caller->pc()->GetTransceivers()[0]->mid().has_value());
caller->pc()->SetLocalDescription(
new rtc::RefCountedObject<MockSetSessionDescriptionObserver>(),
@@ -804,6 +875,22 @@
}
TEST_F(PeerConnectionSignalingUnifiedPlanTest,
+ SetLocalDescriptionExecutesImmediatelyUsingNewObserver) {
+ auto caller = CreatePeerConnectionWithAudioVideo();
+
+ // This offer will cause transceiver mids to get assigned.
+ auto offer = caller->CreateOffer(RTCOfferAnswerOptions());
+
+ // Verify that mids were assigned without waiting for the observer. (However,
+ // the new observer should also be invoked synchronously - as is ensured by
+ // other tests.)
+ RTC_DCHECK(!caller->pc()->GetTransceivers()[0]->mid().has_value());
+ caller->pc()->SetLocalDescription(std::move(offer),
+ new FakeSetLocalDescriptionObserver());
+ EXPECT_TRUE(caller->pc()->GetTransceivers()[0]->mid().has_value());
+}
+
+TEST_F(PeerConnectionSignalingUnifiedPlanTest,
SetLocalDescriptionExecutesImmediatelyInsideCreateOfferCallback) {
auto caller = CreatePeerConnectionWithAudioVideo();
