Change who gets the clone when creating SDPs in SdpOfferAnswerHandler
Previously, `CreateDescriptionObserverWrapperWithCreationCallback` and
associated callbacks, created a clone of the new sdp for internal
storage (last_created_offer_ and last_created_answer_) passed the
original session description to the wrapped observer.
This patch inverts the logic and gives the cloned object to the wrapped
observer instead. The reason for that is that in order to create a
clone, the state of the original object needs to be accessed and doing
so will (once thread checkers are in place) pin that state to the
calling thread. The calling thread is the signaling thread, which is
suitable for SdpOfferAnswerHandler but the wrapped observer should get
the pristine object which is less bound.
A side effect of this CL may be that we'll get less flake on tsan bots
since a test observer implementation will now get a newly constructed
object rather than one who's state may be accessed on different threads
in parallel. However, the local_description() and remote_description()
accessors still need to be fixed (wip).
Along the way use AnyInvocable&& instead of std::function.
Bug: webrtc:442220720
Change-Id: Idfef673b59e1f50bc2a6e82fb142d5f1aa528d8b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/409280
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#45646}
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc
index 93f7c70..506ec61 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -1301,26 +1301,38 @@
// Wraps a session description observer so a Clone of the last created
// offer/answer can be stored.
+// This object can be used only once.
class CreateDescriptionObserverWrapperWithCreationCallback
: public CreateSessionDescriptionObserver {
public:
CreateDescriptionObserverWrapperWithCreationCallback(
- std::function<void(const SessionDescriptionInterface* desc)> callback,
+ absl::AnyInvocable<void(std::unique_ptr<SessionDescriptionInterface>) &&>
+ callback,
scoped_refptr<CreateSessionDescriptionObserver> observer)
- : callback_(callback), observer_(observer) {
+ : callback_(std::move(callback)), observer_(observer) {
RTC_DCHECK(observer_);
}
void OnSuccess(SessionDescriptionInterface* desc) override {
- callback_(desc);
- observer_->OnSuccess(desc);
+ // `OnSuccess` will be called on the signaling thread. That's what we want
+ // for SdpOfferAnswerHandler since that's where public methods are called.
+ // The wrapped observer might have different threading needs. Calling the
+ // `Clone()` method will require accessing internal state of `desc` and
+ // therefore will (at least semantically) attach its state to the current
+ // thread. So, since we need to call Clone(), keep `desc` as the copy that
+ // SdpOfferAnswerHandler will own, and give a pristine clone to the
+ // observer.
+ auto clone = desc->Clone();
+ std::move(callback_)(absl::WrapUnique(desc));
+ observer_->OnSuccess(clone.release());
}
void OnFailure(RTCError error) override {
- callback_(nullptr);
+ std::move(callback_)(nullptr);
observer_->OnFailure(std::move(error));
}
private:
- std::function<void(const SessionDescriptionInterface* desc)> callback_;
+ absl::AnyInvocable<void(std::unique_ptr<SessionDescriptionInterface>) &&>
+ callback_;
scoped_refptr<CreateSessionDescriptionObserver> observer_;
};
@@ -2665,13 +2677,9 @@
GetOptionsForOffer(options, &session_options);
auto observer_wrapper =
make_ref_counted<CreateDescriptionObserverWrapperWithCreationCallback>(
- [this](const SessionDescriptionInterface* desc) {
+ [this](std::unique_ptr<SessionDescriptionInterface> desc) {
RTC_DCHECK_RUN_ON(signaling_thread());
- if (desc) {
- last_created_offer_ = desc->Clone();
- } else {
- last_created_offer_.reset(nullptr);
- }
+ last_created_offer_ = std::move(desc);
},
std::move(observer));
webrtc_session_desc_factory_->CreateOffer(observer_wrapper.get(), options,
@@ -2763,13 +2771,9 @@
GetOptionsForAnswer(options, &session_options);
auto observer_wrapper =
make_ref_counted<CreateDescriptionObserverWrapperWithCreationCallback>(
- [this](const SessionDescriptionInterface* desc) {
+ [this](std::unique_ptr<SessionDescriptionInterface> desc) {
RTC_DCHECK_RUN_ON(signaling_thread());
- if (desc) {
- last_created_answer_ = desc->Clone();
- } else {
- last_created_answer_.reset(nullptr);
- }
+ last_created_answer_ = std::move(desc);
},
std::move(observer));
webrtc_session_desc_factory_->CreateAnswer(observer_wrapper.get(),
