Revert "SetRemoteDescriptionObserverInterface added."
This reverts commit 6c7ec32bd63ab2b45d4d83ae1de817ee946b4d72.
Reason for revert: Third party project breaks due to use-after-free
in the callback. I suspect this is because the adapter is processing
the async callback instead of the pc, i.e. callback is called from
SetRemoteDescriptionObserverAdapter::OnMessage instead of from
PeerConnection::OnMessage. This makes it possible for the callback to
be invoked after the PC is destroyed.
I argue this is how it should be done, and that if you're using a raw
pointer in an async callback you're doing it wrong, but I will reland
this CL with the callback processed in PeerConnection::OnMessage
instead as to not change the behavior of the old SRD signature.
Original change's description:
> SetRemoteDescriptionObserverInterface added.
>
> The new observer replaced SetSessionDescriptionObserver for
> SetRemoteDescription. Unlike SetSessionDescriptionObserver,
> SetRemoteDescriptionObserverInterface is invoked synchronously so
> that the you can rely on the state of the PeerConnection to represent
> the result of the SetRemoteDescription call in the callback.
>
> The new observer succeeds or fails with an RTCError.
>
> This deprecates the need for PeerConnectionObserver::OnAdd/RemoveTrack
> and SetSessionDescriptionObserver, with the benefit that all media
> object changes can be processed in a single callback by the application
> in a synchronous callback. This will help Chromium keep objects in-sync
> across layers and threads in a non-racy and straight-forward way, see
> design doc (Proposal 2):
> https://docs.google.com/a/google.com/document/d/1-cDDC82mgU5zrHacfFz720p3xwRtuBkOPSRchh07Ho0/edit?usp=sharing
>
> An adapter for SetSessionDescriptionObserver is added to allow calling
> the old SetRemoteDescription signature and get the old behavior
> (OnSuccess/OnFailure callback in a Post) until third parties switch.
>
> Bug: webrtc:8473
> Change-Id: I3d4eb60da6dd34615f2c9f384aeaf4634e648c99
> Reviewed-on: https://webrtc-review.googlesource.com/17523
> Commit-Queue: Henrik Boström <hbos@webrtc.org>
> Reviewed-by: Peter Thatcher <pthatcher@webrtc.org>
> Reviewed-by: Guido Urdaneta <guidou@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#20841}
TBR=hbos@webrtc.org,hta@webrtc.org,pthatcher@webrtc.org,guidou@webrtc.org
Change-Id: I715555e084d9aae49ee2a8831c70dc006dbdb74c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: webrtc:8473
Reviewed-on: https://webrtc-review.googlesource.com/25580
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20850}diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc
index 08370d9..62e0ef8 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -1463,7 +1463,7 @@
std::string error;
// Takes the ownership of |desc_temp|. On success, local_description() is
// updated to reflect the description that was passed in.
- if (!SetCurrentOrPendingLocalDescription(std::move(desc_temp), &error)) {
+ if (!SetLocalDescription(std::move(desc_temp), &error)) {
PostSetSessionDescriptionFailure(observer, error);
return;
}
@@ -1541,25 +1541,26 @@
}
void PeerConnection::SetRemoteDescription(
- std::unique_ptr<SessionDescriptionInterface> desc,
- rtc::scoped_refptr<SetRemoteDescriptionObserverInterface> observer) {
+ SetSessionDescriptionObserver* observer,
+ SessionDescriptionInterface* desc) {
TRACE_EVENT0("webrtc", "PeerConnection::SetRemoteDescription");
if (!observer) {
RTC_LOG(LS_ERROR) << "SetRemoteDescription - observer is NULL.";
return;
}
if (!desc) {
- observer->OnSetRemoteDescriptionComplete(RTCError(
- RTCErrorType::UNSUPPORTED_PARAMETER, "SessionDescription is NULL."));
+ PostSetSessionDescriptionFailure(observer, "SessionDescription is NULL.");
return;
}
+ // Takes the ownership of |desc| regardless of the result.
+ std::unique_ptr<SessionDescriptionInterface> desc_temp(desc);
+
if (IsClosed()) {
- std::string error = "Failed to set remote " + desc->type() +
+ std::string error = "Failed to set remote " + desc_temp->type() +
" sdp: Called in wrong state: STATE_CLOSED";
RTC_LOG(LS_ERROR) << error;
- observer->OnSetRemoteDescriptionComplete(
- RTCError(RTCErrorType::INVALID_STATE, std::move(error)));
+ PostSetSessionDescriptionFailure(observer, error);
return;
}
@@ -1567,11 +1568,10 @@
// streams that might be removed by updating the session description.
stats_->UpdateStats(kStatsOutputLevelStandard);
std::string error;
- // Takes the ownership of |desc|. On success, remote_description() is updated
- // to reflect the description that was passed in.
- if (!SetCurrentOrPendingRemoteDescription(std::move(desc), &error)) {
- observer->OnSetRemoteDescriptionComplete(
- RTCError(RTCErrorType::UNSUPPORTED_PARAMETER, std::move(error)));
+ // Takes the ownership of |desc_temp|. On success, remote_description() is
+ // updated to reflect the description that was passed in.
+ if (!SetRemoteDescription(std::move(desc_temp), &error)) {
+ PostSetSessionDescriptionFailure(observer, error);
return;
}
RTC_DCHECK(remote_description());
@@ -1661,7 +1661,9 @@
UpdateEndedRemoteMediaStreams();
- observer->OnSetRemoteDescriptionComplete(RTCError::OK());
+ SetSessionDescriptionMsg* msg = new SetSessionDescriptionMsg(observer);
+ signaling_thread()->Post(RTC_FROM_HERE, this,
+ MSG_SET_SESSIONDESCRIPTION_SUCCESS, msg);
if (remote_description()->type() == SessionDescriptionInterface::kAnswer) {
// TODO(deadbeef): We already had to hop to the network thread for
@@ -3258,7 +3260,7 @@
role);
}
-bool PeerConnection::SetCurrentOrPendingLocalDescription(
+bool PeerConnection::SetLocalDescription(
std::unique_ptr<SessionDescriptionInterface> desc,
std::string* err_desc) {
RTC_DCHECK(signaling_thread()->IsCurrent());
@@ -3314,7 +3316,7 @@
return true;
}
-bool PeerConnection::SetCurrentOrPendingRemoteDescription(
+bool PeerConnection::SetRemoteDescription(
std::unique_ptr<SessionDescriptionInterface> desc,
std::string* err_desc) {
RTC_DCHECK(signaling_thread()->IsCurrent());
