Make Android/iOS local/remote description accessors thread safe.
Since the descriptions can be modified on the signaling thread,
ToString can only be safely called on that thread.
Bug: webrtc:11791
Change-Id: Icf6aada8aa66d00be94c6bda7b22e41b5d3bbc17
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/180541
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Sami Kalliomäki <sakal@webrtc.org>
Reviewed-by: Anders Carlsson <andersc@webrtc.org>
Commit-Queue: Taylor <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31862}
diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h
index b1beb84..09317b8 100644
--- a/api/peer_connection_interface.h
+++ b/api/peer_connection_interface.h
@@ -909,6 +909,10 @@
const std::string& label,
const DataChannelInit* config) = 0;
+ // NOTE: For the following 6 methods, it's only safe to dereference the
+ // SessionDescriptionInterface on signaling_thread() (for example, calling
+ // ToString).
+
// Returns the more recently applied description; "pending" if it exists, and
// otherwise "current". See below.
virtual const SessionDescriptionInterface* local_description() const = 0;
@@ -1136,6 +1140,14 @@
// thus the observer object can be safely destroyed.
virtual void Close() = 0;
+ // The thread on which all PeerConnectionObserver callbacks will be invoked,
+ // as well as callbacks for other classes such as DataChannelObserver.
+ //
+ // Also the only thread on which it's safe to use SessionDescriptionInterface
+ // pointers.
+ // TODO(deadbeef): Make pure virtual when all subclasses implement it.
+ virtual rtc::Thread* signaling_thread() const { return nullptr; }
+
protected:
// Dtor protected as objects shouldn't be deleted via this interface.
~PeerConnectionInterface() override = default;
diff --git a/api/peer_connection_proxy.h b/api/peer_connection_proxy.h
index aa72d7c..0cc3b3b 100644
--- a/api/peer_connection_proxy.h
+++ b/api/peer_connection_proxy.h
@@ -147,6 +147,7 @@
PROXY_METHOD1(bool, StartRtcEventLog, std::unique_ptr<RtcEventLogOutput>)
PROXY_METHOD0(void, StopRtcEventLog)
PROXY_METHOD0(void, Close)
+BYPASS_PROXY_CONSTMETHOD0(rtc::Thread*, signaling_thread)
END_PROXY_MAP()
} // namespace webrtc
diff --git a/api/proxy.h b/api/proxy.h
index b1ebe31..0e5d622 100644
--- a/api/proxy.h
+++ b/api/proxy.h
@@ -400,11 +400,13 @@
// For use when returning purely const state (set during construction).
// Use with caution. This method should only be used when the return value will
// always be the same.
-#define BYPASS_PROXY_CONSTMETHOD0(r, method) \
- r method() const override { \
- static_assert(!std::is_pointer<r>::value, "Type is a pointer"); \
- static_assert(!std::is_reference<r>::value, "Type is a reference"); \
- return c_->method(); \
+#define BYPASS_PROXY_CONSTMETHOD0(r, method) \
+ r method() const override { \
+ static_assert( \
+ std::is_same<r, rtc::Thread*>::value || !std::is_pointer<r>::value, \
+ "Type is a pointer"); \
+ static_assert(!std::is_reference<r>::value, "Type is a reference"); \
+ return c_->method(); \
}
} // namespace webrtc
diff --git a/api/test/dummy_peer_connection.h b/api/test/dummy_peer_connection.h
index 97a97d0..0ca7d3f 100644
--- a/api/test/dummy_peer_connection.h
+++ b/api/test/dummy_peer_connection.h
@@ -237,7 +237,11 @@
void StopRtcEventLog() { FATAL() << "Not implemented"; }
- void Close() {}
+ void Close() override {}
+
+ rtc::Thread* signaling_thread() const override {
+ return rtc::Thread::Current();
+ }
};
static_assert(
diff --git a/pc/BUILD.gn b/pc/BUILD.gn
index 6b07bbe..9d04b4c 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -270,6 +270,7 @@
"../rtc_base:weak_ptr",
"../rtc_base/experiments:field_trial_parser",
"../rtc_base/synchronization:mutex",
+ "../rtc_base/synchronization:sequence_checker",
"../rtc_base/system:file_wrapper",
"../rtc_base/system:rtc_export",
"../rtc_base/third_party/base64",
diff --git a/pc/peer_connection.h b/pc/peer_connection.h
index 1cb5752..41cb68c 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -260,14 +260,15 @@
void Close() override;
+ rtc::Thread* signaling_thread() const final {
+ return factory_->signaling_thread();
+ }
+
// PeerConnectionInternal implementation.
rtc::Thread* network_thread() const final {
return factory_->network_thread();
}
rtc::Thread* worker_thread() const final { return factory_->worker_thread(); }
- rtc::Thread* signaling_thread() const final {
- return factory_->signaling_thread();
- }
std::string session_id() const override {
RTC_DCHECK_RUN_ON(signaling_thread());
diff --git a/pc/peer_connection_internal.h b/pc/peer_connection_internal.h
index 1a78ed2..029feba 100644
--- a/pc/peer_connection_internal.h
+++ b/pc/peer_connection_internal.h
@@ -30,7 +30,6 @@
public:
virtual rtc::Thread* network_thread() const = 0;
virtual rtc::Thread* worker_thread() const = 0;
- virtual rtc::Thread* signaling_thread() const = 0;
// The SDP session ID as defined by RFC 3264.
virtual std::string session_id() const = 0;
diff --git a/sdk/android/src/jni/pc/peer_connection.cc b/sdk/android/src/jni/pc/peer_connection.cc
index 9cebda3..1165333 100644
--- a/sdk/android/src/jni/pc/peer_connection.cc
+++ b/sdk/android/src/jni/pc/peer_connection.cc
@@ -478,17 +478,39 @@
static ScopedJavaLocalRef<jobject> JNI_PeerConnection_GetLocalDescription(
JNIEnv* jni,
const JavaParamRef<jobject>& j_pc) {
- const SessionDescriptionInterface* sdp =
- ExtractNativePC(jni, j_pc)->local_description();
- return sdp ? NativeToJavaSessionDescription(jni, sdp) : nullptr;
+ PeerConnectionInterface* pc = ExtractNativePC(jni, j_pc);
+ // It's only safe to operate on SessionDescriptionInterface on the
+ // signaling thread, but |jni| may only be used on the current thread, so we
+ // must do this odd dance.
+ std::string sdp;
+ std::string type;
+ pc->signaling_thread()->Invoke<void>(RTC_FROM_HERE, [pc, &sdp, &type] {
+ const SessionDescriptionInterface* desc = pc->local_description();
+ if (desc) {
+ RTC_CHECK(desc->ToString(&sdp)) << "got so far: " << sdp;
+ type = desc->type();
+ }
+ });
+ return sdp.empty() ? nullptr : NativeToJavaSessionDescription(jni, sdp, type);
}
static ScopedJavaLocalRef<jobject> JNI_PeerConnection_GetRemoteDescription(
JNIEnv* jni,
const JavaParamRef<jobject>& j_pc) {
- const SessionDescriptionInterface* sdp =
- ExtractNativePC(jni, j_pc)->remote_description();
- return sdp ? NativeToJavaSessionDescription(jni, sdp) : nullptr;
+ PeerConnectionInterface* pc = ExtractNativePC(jni, j_pc);
+ // It's only safe to operate on SessionDescriptionInterface on the
+ // signaling thread, but |jni| may only be used on the current thread, so we
+ // must do this odd dance.
+ std::string sdp;
+ std::string type;
+ pc->signaling_thread()->Invoke<void>(RTC_FROM_HERE, [pc, &sdp, &type] {
+ const SessionDescriptionInterface* desc = pc->remote_description();
+ if (desc) {
+ RTC_CHECK(desc->ToString(&sdp)) << "got so far: " << sdp;
+ type = desc->type();
+ }
+ });
+ return sdp.empty() ? nullptr : NativeToJavaSessionDescription(jni, sdp, type);
}
static ScopedJavaLocalRef<jobject> JNI_PeerConnection_GetCertificate(
diff --git a/sdk/android/src/jni/pc/sdp_observer.cc b/sdk/android/src/jni/pc/sdp_observer.cc
index fc59d17..d1842a3 100644
--- a/sdk/android/src/jni/pc/sdp_observer.cc
+++ b/sdk/android/src/jni/pc/sdp_observer.cc
@@ -31,8 +31,11 @@
void CreateSdpObserverJni::OnSuccess(SessionDescriptionInterface* desc) {
JNIEnv* env = AttachCurrentThreadIfNeeded();
- Java_SdpObserver_onCreateSuccess(env, j_observer_global_,
- NativeToJavaSessionDescription(env, desc));
+ std::string sdp;
+ RTC_CHECK(desc->ToString(&sdp)) << "got so far: " << sdp;
+ Java_SdpObserver_onCreateSuccess(
+ env, j_observer_global_,
+ NativeToJavaSessionDescription(env, sdp, desc->type()));
// OnSuccess transfers ownership of the description (there's a TODO to make
// it use unique_ptr...).
delete desc;
diff --git a/sdk/android/src/jni/pc/session_description.cc b/sdk/android/src/jni/pc/session_description.cc
index 1b33521..bbac721 100644
--- a/sdk/android/src/jni/pc/session_description.cc
+++ b/sdk/android/src/jni/pc/session_description.cc
@@ -37,12 +37,10 @@
ScopedJavaLocalRef<jobject> NativeToJavaSessionDescription(
JNIEnv* jni,
- const SessionDescriptionInterface* desc) {
- std::string sdp;
- RTC_CHECK(desc->ToString(&sdp)) << "got so far: " << sdp;
+ const std::string& sdp,
+ const std::string& type) {
return Java_SessionDescription_Constructor(
- jni,
- Java_Type_fromCanonicalForm(jni, NativeToJavaString(jni, desc->type())),
+ jni, Java_Type_fromCanonicalForm(jni, NativeToJavaString(jni, type)),
NativeToJavaString(jni, sdp));
}
diff --git a/sdk/android/src/jni/pc/session_description.h b/sdk/android/src/jni/pc/session_description.h
index fe30847..f0f49cb 100644
--- a/sdk/android/src/jni/pc/session_description.h
+++ b/sdk/android/src/jni/pc/session_description.h
@@ -13,6 +13,7 @@
#include <jni.h>
#include <memory>
+#include <string>
#include "api/jsep.h"
#include "sdk/android/native_api/jni/scoped_java_ref.h"
@@ -26,7 +27,8 @@
ScopedJavaLocalRef<jobject> NativeToJavaSessionDescription(
JNIEnv* jni,
- const SessionDescriptionInterface* desc);
+ const std::string& sdp,
+ const std::string& type);
} // namespace jni
} // namespace webrtc
diff --git a/sdk/objc/api/peerconnection/RTCPeerConnection.mm b/sdk/objc/api/peerconnection/RTCPeerConnection.mm
index 42a43a7..9288a13 100644
--- a/sdk/objc/api/peerconnection/RTCPeerConnection.mm
+++ b/sdk/objc/api/peerconnection/RTCPeerConnection.mm
@@ -358,19 +358,27 @@
}
- (RTC_OBJC_TYPE(RTCSessionDescription) *)localDescription {
- const webrtc::SessionDescriptionInterface *description =
- _peerConnection->local_description();
- return description ?
- [[RTC_OBJC_TYPE(RTCSessionDescription) alloc] initWithNativeDescription:description] :
- nil;
+ // It's only safe to operate on SessionDescriptionInterface on the signaling thread.
+ return _peerConnection->signaling_thread()->Invoke<RTC_OBJC_TYPE(RTCSessionDescription) *>(
+ RTC_FROM_HERE, [self] {
+ const webrtc::SessionDescriptionInterface *description =
+ _peerConnection->local_description();
+ return description ?
+ [[RTC_OBJC_TYPE(RTCSessionDescription) alloc] initWithNativeDescription:description] :
+ nil;
+ });
}
- (RTC_OBJC_TYPE(RTCSessionDescription) *)remoteDescription {
- const webrtc::SessionDescriptionInterface *description =
- _peerConnection->remote_description();
- return description ?
- [[RTC_OBJC_TYPE(RTCSessionDescription) alloc] initWithNativeDescription:description] :
- nil;
+ // It's only safe to operate on SessionDescriptionInterface on the signaling thread.
+ return _peerConnection->signaling_thread()->Invoke<RTC_OBJC_TYPE(RTCSessionDescription) *>(
+ RTC_FROM_HERE, [self] {
+ const webrtc::SessionDescriptionInterface *description =
+ _peerConnection->remote_description();
+ return description ?
+ [[RTC_OBJC_TYPE(RTCSessionDescription) alloc] initWithNativeDescription:description] :
+ nil;
+ });
}
- (RTCSignalingState)signalingState {