Reland "[PeerConnection] Use an OperationsChain in PeerConnection for async ops."
This is a reland of 1dddaa1a84330091ca083c950ef2e24a85a48fc8
The regression that caused the original CL to be reverted was the fact that
invoking SetLocalDescription() inside of the CreateOffer() callback was no
longer executing synchronously and immediately.
In this CL, the original CL is patched so that the CreateOffer() operation
is marked as completed just before invoking the CreateOffer() callback
(versus doing it just afterwards). This ensures that the OperationsChain is
popped before the callback runs. The same applies for CreateAnswer().
See diff between Patch Set 1 (Original CL) and the latest Patch Set.
Original change's description:
> [PeerConnection] Use an OperationsChain in PeerConnection for async ops.
>
> For background, motivation, requirements and implementation notes, see
> https://docs.google.com/document/d/1XLwNN2kUIGGTwz9LQ0NwJNkcybi9oKnynUEZB1jGA14/edit?usp=sharing
>
> Using the OperationsChain will unblock future CLs from chaining multiple
> operations together such as implementing parameterless
> setLocalDescription().
>
> In this CL, the OperationsChain is used in existing signaling operations
> with little intended side-effects. An operation that is chained onto an
> empty OperationsChain will for instance execute immediately, and
> SetLocalDescription() and SetRemoteDescription() are implemented as
> "synchronous operations".
>
> The lifetime of the PeerConnection is not indended to change as a result
> of this CL: All chained operations use a WeakPtr to the PC to ensure
> use-after-free does not happen.
>
> There is one notable change though: CreateOffer() and CreateAnswer() will
> asynchronously delay other signaling methods from executing until they
> have completed.
>
> Drive-by fix: This CL also ensures that early failing
> CreateOffer/CreateAnswer operation's observers are invoked if the
> PeerConnection is destroyed while a PostCreateSessionDescriptionFailure
> is pending.
>
> Bug: webrtc:11019
> Change-Id: I521333e41d20d9bbfb1e721609f2c9db2a5f93a9
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/157305
> Reviewed-by: Steve Anton <steveanton@webrtc.org>
> Commit-Queue: Henrik Boström <hbos@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#29605}
TBR=steveanton@webrtc.org
Bug: webrtc:11019
Change-Id: I57b4496e63378c91c24679ee496e21f5cb6a0e59
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/158524
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#29630}
diff --git a/pc/peer_connection_signaling_unittest.cc b/pc/peer_connection_signaling_unittest.cc
index 9916539..e79ee3d 100644
--- a/pc/peer_connection_signaling_unittest.cc
+++ b/pc/peer_connection_signaling_unittest.cc
@@ -41,6 +41,10 @@
using ::testing::Combine;
using ::testing::Values;
+namespace {
+const int64_t kWaitTimeout = 10000;
+} // namespace
+
class PeerConnectionWrapperForSignalingTest : public PeerConnectionWrapper {
public:
using PeerConnectionWrapper::PeerConnectionWrapper;
@@ -57,6 +61,31 @@
}
};
+class ExecuteFunctionOnCreateSessionDescriptionObserver
+ : public CreateSessionDescriptionObserver {
+ public:
+ ExecuteFunctionOnCreateSessionDescriptionObserver(
+ std::function<void(SessionDescriptionInterface*)> function)
+ : function_(std::move(function)) {}
+ ~ExecuteFunctionOnCreateSessionDescriptionObserver() override {
+ RTC_DCHECK(was_called_);
+ }
+
+ bool was_called() const { return was_called_; }
+
+ void OnSuccess(SessionDescriptionInterface* desc) override {
+ RTC_DCHECK(!was_called_);
+ was_called_ = true;
+ function_(desc);
+ }
+
+ void OnFailure(RTCError error) override { RTC_NOTREACHED(); }
+
+ private:
+ bool was_called_ = false;
+ std::function<void(SessionDescriptionInterface*)> function_;
+};
+
class PeerConnectionSignalingBaseTest : public ::testing::Test {
protected:
typedef std::unique_ptr<PeerConnectionWrapperForSignalingTest> WrapperPtr;
@@ -522,6 +551,63 @@
}
}
+// Similar to the above test, but by closing the PC first the CreateOffer() will
+// fail "early", which triggers a codepath where the PeerConnection is
+// reponsible for invoking the observer, instead of the normal codepath where
+// the WebRtcSessionDescriptionFactory is responsible for it.
+TEST_P(PeerConnectionSignalingTest, CloseCreateOfferAndShutdown) {
+ auto caller = CreatePeerConnection();
+ rtc::scoped_refptr<MockCreateSessionDescriptionObserver> observer =
+ new rtc::RefCountedObject<MockCreateSessionDescriptionObserver>();
+ caller->pc()->Close();
+ caller->pc()->CreateOffer(observer, RTCOfferAnswerOptions());
+ caller.reset(nullptr);
+ EXPECT_TRUE(observer->called());
+}
+
+TEST_P(PeerConnectionSignalingTest, SetRemoteDescriptionExecutesImmediately) {
+ auto caller = CreatePeerConnectionWithAudioVideo();
+ auto callee = CreatePeerConnection();
+
+ // This offer will cause receivers to be created.
+ auto offer = caller->CreateOffer(RTCOfferAnswerOptions());
+
+ // By not waiting for the observer's callback we can verify that the operation
+ // executed immediately.
+ callee->pc()->SetRemoteDescription(std::move(offer),
+ new MockSetRemoteDescriptionObserver());
+ EXPECT_EQ(2u, callee->pc()->GetReceivers().size());
+}
+
+TEST_P(PeerConnectionSignalingTest, CreateOfferBlocksSetRemoteDescription) {
+ auto caller = CreatePeerConnectionWithAudioVideo();
+ auto callee = CreatePeerConnection();
+
+ // This offer will cause receivers to be created.
+ auto offer = caller->CreateOffer(RTCOfferAnswerOptions());
+
+ EXPECT_EQ(0u, callee->pc()->GetReceivers().size());
+ rtc::scoped_refptr<MockCreateSessionDescriptionObserver> offer_observer(
+ new rtc::RefCountedObject<MockCreateSessionDescriptionObserver>());
+ // Synchronously invoke CreateOffer() and SetRemoteDescription(). The
+ // SetRemoteDescription() operation should be chained to be executed
+ // asynchronously, when CreateOffer() completes.
+ callee->pc()->CreateOffer(offer_observer, RTCOfferAnswerOptions());
+ callee->pc()->SetRemoteDescription(std::move(offer),
+ new MockSetRemoteDescriptionObserver());
+ // CreateOffer() is asynchronous; without message processing this operation
+ // should not have completed.
+ EXPECT_FALSE(offer_observer->called());
+ // Due to chaining, the receivers should not have been created by the offer
+ // yet.
+ EXPECT_EQ(0u, callee->pc()->GetReceivers().size());
+ // EXPECT_TRUE_WAIT causes messages to be processed...
+ EXPECT_TRUE_WAIT(offer_observer->called(), kWaitTimeout);
+ // Now that the offer has been completed, SetRemoteDescription() will have
+ // been executed next in the chain.
+ EXPECT_EQ(2u, callee->pc()->GetReceivers().size());
+}
+
INSTANTIATE_TEST_SUITE_P(PeerConnectionSignalingTest,
PeerConnectionSignalingTest,
Values(SdpSemantics::kPlanB,
@@ -534,6 +620,49 @@
: PeerConnectionSignalingBaseTest(SdpSemantics::kUnifiedPlan) {}
};
+// We verify that SetLocalDescription() executed immediately by verifying that
+// the transceiver mid values got assigned. SLD executing immeditately is not
+// unique to Unified Plan, but the transceivers used to verify this are only
+// available in Unified Plan.
+TEST_F(PeerConnectionSignalingUnifiedPlanTest,
+ SetLocalDescriptionExecutesImmediately) {
+ 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.
+ RTC_DCHECK(!caller->pc()->GetTransceivers()[0]->mid().has_value());
+ caller->pc()->SetLocalDescription(
+ new rtc::RefCountedObject<MockSetSessionDescriptionObserver>(),
+ offer.release());
+ EXPECT_TRUE(caller->pc()->GetTransceivers()[0]->mid().has_value());
+}
+
+TEST_F(PeerConnectionSignalingUnifiedPlanTest,
+ SetLocalDescriptionExecutesImmediatelyInsideCreateOfferCallback) {
+ auto caller = CreatePeerConnectionWithAudioVideo();
+
+ // This offer will cause transceiver mids to get assigned.
+ auto offer = caller->CreateOffer(RTCOfferAnswerOptions());
+
+ rtc::scoped_refptr<ExecuteFunctionOnCreateSessionDescriptionObserver>
+ offer_observer(new rtc::RefCountedObject<
+ ExecuteFunctionOnCreateSessionDescriptionObserver>(
+ [pc = caller->pc()](SessionDescriptionInterface* desc) {
+ // By not waiting for the observer's callback we can verify that the
+ // operation executed immediately.
+ RTC_DCHECK(!pc->GetTransceivers()[0]->mid().has_value());
+ pc->SetLocalDescription(
+ new rtc::RefCountedObject<MockSetSessionDescriptionObserver>(),
+ desc);
+ EXPECT_TRUE(pc->GetTransceivers()[0]->mid().has_value());
+ }));
+ caller->pc()->CreateOffer(offer_observer, RTCOfferAnswerOptions());
+ EXPECT_TRUE_WAIT(offer_observer->was_called(), kWaitTimeout);
+}
+
// Test that transports are shown in the sender/receiver API after offer/answer.
// This only works in Unified Plan.
TEST_F(PeerConnectionSignalingUnifiedPlanTest,