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}

api/peer_connection_interface.h

@@ -909,6 +909,10 @@ class RTC_EXPORT PeerConnectionInterface : public rtc::RefCountInterface {
       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 @@ class RTC_EXPORT PeerConnectionInterface : public rtc::RefCountInterface {
   // 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;

api/peer_connection_proxy.h

@@ -147,6 +147,7 @@ PROXY_METHOD2(bool,
 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

api/proxy.h

@@ -402,7 +402,9 @@ class ConstMethodCall : public rtc::Message, public rtc::MessageHandler {
 // 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_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();                                                    \
   }

api/test/dummy_peer_connection.h

@@ -237,7 +237,11 @@ class DummyPeerConnection : public PeerConnectionInterface {
 
   void StopRtcEventLog() { FATAL() << "Not implemented"; }
 
-  void Close() {}
+  void Close() override {}
+
+  rtc::Thread* signaling_thread() const override {
+    return rtc::Thread::Current();
+  }
 };
 
 static_assert(

pc/BUILD.gn

@@ -270,6 +270,7 @@ rtc_library("peerconnection") {
     "../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",

pc/peer_connection.h

@@ -260,14 +260,15 @@ class PeerConnection : public PeerConnectionInternal,
 
   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());

pc/peer_connection_internal.h

@@ -30,7 +30,6 @@ class PeerConnectionInternal : public PeerConnectionInterface {
  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;

sdk/android/src/jni/pc/peer_connection.cc

@@ -478,17 +478,39 @@ static jlong JNI_PeerConnection_GetNativePeerConnection(
 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(

sdk/android/src/jni/pc/sdp_observer.cc

@@ -31,8 +31,11 @@ CreateSdpObserverJni::~CreateSdpObserverJni() = default;
 
 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;

sdk/android/src/jni/pc/session_description.cc

@@ -37,12 +37,10 @@ std::unique_ptr<SessionDescriptionInterface> JavaToNativeSessionDescription(
 
 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));
 }
 

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 @@ std::unique_ptr<SessionDescriptionInterface> JavaToNativeSessionDescription(
 
 ScopedJavaLocalRef<jobject> NativeToJavaSessionDescription(
     JNIEnv* jni,
-    const SessionDescriptionInterface* desc);
+    const std::string& sdp,
+    const std::string& type);
 
 }  // namespace jni
 }  // namespace webrtc

sdk/objc/api/peerconnection/RTCPeerConnection.mm

@@ -358,19 +358,27 @@ void PeerConnectionDelegateAdapter::OnRemoveTrack(
 }
 
 - (RTC_OBJC_TYPE(RTCSessionDescription) *)localDescription {
+  // 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 {
+  // 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 {