Reland "Add thread guards to JsepTransport"

This reverts commit caedb5db82b2bc8273910f4a0d1afb1d0e2994f3.

Reason for revert: Fixed issue (allow SetNeedsIceRestart from off-thread).

Original change's description:
> Revert "Add thread guards to JsepTransport"
>
> This reverts commit 7e1db52c93c57a180073906eda6a58919a9fd537.
>
> Reason for revert: Breaks downstream.
>
> Original change's description:
> > Add thread guards to JsepTransport
> >
> > This ensures that JsepTransport's methods are either only accessed on the thread
> > that creates it, or using methods that are marked for off-thread use
> > (using a lock to prevent simultaneous access).
> >
> > The intent is to document the existing contract, and to make it easy to find the
> > actions needed to convert the class to a pure single-threaded class.
> >
> > Bug: webrtc:10300
> > Change-Id: Ib5cdc027632c36baec55179937d6eb664bbaf6f5
> > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/121946
> > Commit-Queue: Harald Alvestrand <hta@webrtc.org>
> > Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
> > Reviewed-by: Henrik Boström <hbos@webrtc.org>
> > Cr-Commit-Position: refs/heads/master@{#27427}
>
> TBR=steveanton@webrtc.org,solenberg@webrtc.org,kwiberg@webrtc.org,hbos@webrtc.org,hta@webrtc.org
>
> Change-Id: I30c65d2161de9376ccd1172e2b261f2280fb1d75
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: webrtc:10300
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/130519
> Reviewed-by: Gustaf Ullberg <gustaf@webrtc.org>
> Commit-Queue: Gustaf Ullberg <gustaf@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#27429}

Change-Id: Ic32bfc04d96e657fc67c3d3999f77969e55ed994
Bug: webrtc:10300
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/130962
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27434}

pc/jsep_transport.cc

@@ -99,8 +99,12 @@ JsepTransport::JsepTransport(
     std::unique_ptr<DtlsTransportInternal> rtp_dtls_transport,
     std::unique_ptr<DtlsTransportInternal> rtcp_dtls_transport,
     std::unique_ptr<webrtc::MediaTransportInterface> media_transport)
-    : mid_(mid),
+    : network_thread_(rtc::Thread::Current()),
+      mid_(mid),
       local_certificate_(local_certificate),
+      unencrypted_rtp_transport_(std::move(unencrypted_rtp_transport)),
+      sdes_transport_(std::move(sdes_transport)),
+      dtls_srtp_transport_(std::move(dtls_srtp_transport)),
       rtp_dtls_transport_(
           rtp_dtls_transport ? new rtc::RefCountedObject<webrtc::DtlsTransport>(
                                    std::move(rtp_dtls_transport))
@@ -112,19 +116,17 @@ JsepTransport::JsepTransport(
               : nullptr),
       media_transport_(std::move(media_transport)) {
   RTC_DCHECK(rtp_dtls_transport_);
-  if (unencrypted_rtp_transport) {
+  // Verify the "only one out of these three can be set" invariant.
+  if (unencrypted_rtp_transport_) {
     RTC_DCHECK(!sdes_transport);
     RTC_DCHECK(!dtls_srtp_transport);
-    unencrypted_rtp_transport_ = std::move(unencrypted_rtp_transport);
+  } else if (sdes_transport_) {
-  } else if (sdes_transport) {
     RTC_DCHECK(!unencrypted_rtp_transport);
     RTC_DCHECK(!dtls_srtp_transport);
-    sdes_transport_ = std::move(sdes_transport);
   } else {
-    RTC_DCHECK(dtls_srtp_transport);
+    RTC_DCHECK(dtls_srtp_transport_);
     RTC_DCHECK(!unencrypted_rtp_transport);
     RTC_DCHECK(!sdes_transport);
-    dtls_srtp_transport_ = std::move(dtls_srtp_transport);
   }
 
   if (media_transport_) {
@@ -152,6 +154,7 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription(
     SdpType type) {
   webrtc::RTCError error;
 
+  RTC_DCHECK_RUN_ON(network_thread_);
   if (!VerifyIceParams(jsep_description)) {
     return webrtc::RTCError(webrtc::RTCErrorType::INVALID_PARAMETER,
                             "Invalid ice-ufrag or ice-pwd length.");
@@ -179,7 +182,6 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription(
     dtls_srtp_transport_->UpdateRecvEncryptedHeaderExtensionIds(
         jsep_description.encrypted_header_extension_ids);
   }
-
   bool ice_restarting =
       local_description_ != nullptr &&
       IceCredentialsChanged(local_description_->transport_desc.ice_ufrag,
@@ -194,13 +196,14 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription(
   if (!local_fp) {
     local_certificate_ = nullptr;
   } else {
-    error = VerifyCertificateFingerprint(local_certificate_.get(), local_fp);
+    error = VerifyCertificateFingerprint(local_certificate_, local_fp);
     if (!error.ok()) {
       local_description_.reset();
       return error;
     }
   }
-
+  {
+    rtc::CritScope scope(&accessor_lock_);
     RTC_DCHECK(rtp_dtls_transport_->internal());
     SetLocalIceParameters(rtp_dtls_transport_->internal()->ice_transport());
 
@@ -208,7 +211,7 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription(
       RTC_DCHECK(rtcp_dtls_transport_->internal());
       SetLocalIceParameters(rtcp_dtls_transport_->internal()->ice_transport());
     }
-
+  }
   // If PRANSWER/ANSWER is set, we should decide transport protocol type.
   if (type == SdpType::kPrAnswer || type == SdpType::kAnswer) {
     error = NegotiateAndSetDtlsParameters(type);
@@ -217,12 +220,14 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription(
     local_description_.reset();
     return error;
   }
-
+  {
+    rtc::CritScope scope(&accessor_lock_);
     if (needs_ice_restart_ && ice_restarting) {
       needs_ice_restart_ = false;
       RTC_LOG(LS_VERBOSE) << "needs-ice-restart flag cleared for transport "
                           << mid();
     }
+  }
 
   return webrtc::RTCError::OK();
 }
@@ -232,6 +237,7 @@ webrtc::RTCError JsepTransport::SetRemoteJsepTransportDescription(
     webrtc::SdpType type) {
   webrtc::RTCError error;
 
+  RTC_DCHECK_RUN_ON(network_thread_);
   if (!VerifyIceParams(jsep_description)) {
     remote_description_.reset();
     return webrtc::RTCError(webrtc::RTCErrorType::INVALID_PARAMETER,
@@ -266,12 +272,11 @@ webrtc::RTCError JsepTransport::SetRemoteJsepTransportDescription(
   }
 
   remote_description_.reset(new JsepTransportDescription(jsep_description));
-  RTC_DCHECK(rtp_dtls_transport_->internal());
+  RTC_DCHECK(rtp_dtls_transport());
-  SetRemoteIceParameters(rtp_dtls_transport_->internal()->ice_transport());
+  SetRemoteIceParameters(rtp_dtls_transport()->ice_transport());
 
-  if (rtcp_dtls_transport_) {
+  if (rtcp_dtls_transport()) {
-    RTC_DCHECK(rtcp_dtls_transport_->internal());
+    SetRemoteIceParameters(rtcp_dtls_transport()->ice_transport());
-    SetRemoteIceParameters(rtcp_dtls_transport_->internal()->ice_transport());
   }
 
   // If PRANSWER/ANSWER is set, we should decide transport protocol type.
@@ -287,6 +292,7 @@ webrtc::RTCError JsepTransport::SetRemoteJsepTransportDescription(
 
 webrtc::RTCError JsepTransport::AddRemoteCandidates(
     const Candidates& candidates) {
+  RTC_DCHECK_RUN_ON(network_thread_);
   if (!local_description_ || !remote_description_) {
     return webrtc::RTCError(webrtc::RTCErrorType::INVALID_STATE,
                             mid() +
@@ -312,6 +318,7 @@ webrtc::RTCError JsepTransport::AddRemoteCandidates(
 }
 
 void JsepTransport::SetNeedsIceRestartFlag() {
+  rtc::CritScope scope(&accessor_lock_);
   if (!needs_ice_restart_) {
     needs_ice_restart_ = true;
     RTC_LOG(LS_VERBOSE) << "needs-ice-restart flag set for transport " << mid();
@@ -319,6 +326,8 @@ void JsepTransport::SetNeedsIceRestartFlag() {
 }
 
 absl::optional<rtc::SSLRole> JsepTransport::GetDtlsRole() const {
+  RTC_DCHECK_RUN_ON(network_thread_);
+  rtc::CritScope scope(&accessor_lock_);
   RTC_DCHECK(rtp_dtls_transport_);
   RTC_DCHECK(rtp_dtls_transport_->internal());
   rtc::SSLRole dtls_role;
@@ -330,6 +339,8 @@ absl::optional<rtc::SSLRole> JsepTransport::GetDtlsRole() const {
 }
 
 bool JsepTransport::GetStats(TransportStats* stats) {
+  RTC_DCHECK_RUN_ON(network_thread_);
+  rtc::CritScope scope(&accessor_lock_);
   stats->transport_name = mid();
   stats->channel_stats.clear();
   RTC_DCHECK(rtp_dtls_transport_->internal());
@@ -344,6 +355,7 @@ bool JsepTransport::GetStats(TransportStats* stats) {
 webrtc::RTCError JsepTransport::VerifyCertificateFingerprint(
     const rtc::RTCCertificate* certificate,
     const rtc::SSLFingerprint* fingerprint) const {
+  RTC_DCHECK_RUN_ON(network_thread_);
   if (!fingerprint) {
     return webrtc::RTCError(webrtc::RTCErrorType::INVALID_PARAMETER,
                             "No fingerprint");
@@ -369,6 +381,8 @@ webrtc::RTCError JsepTransport::VerifyCertificateFingerprint(
 }
 
 void JsepTransport::SetActiveResetSrtpParams(bool active_reset_srtp_params) {
+  RTC_DCHECK_RUN_ON(network_thread_);
+  rtc::CritScope scope(&accessor_lock_);
   if (dtls_srtp_transport_) {
     RTC_LOG(INFO)
         << "Setting active_reset_srtp_params of DtlsSrtpTransport to: "
@@ -378,6 +392,7 @@ void JsepTransport::SetActiveResetSrtpParams(bool active_reset_srtp_params) {
 }
 
 void JsepTransport::SetLocalIceParameters(IceTransportInternal* ice_transport) {
+  RTC_DCHECK_RUN_ON(network_thread_);
   RTC_DCHECK(ice_transport);
   RTC_DCHECK(local_description_);
   ice_transport->SetIceParameters(
@@ -386,6 +401,7 @@ void JsepTransport::SetLocalIceParameters(IceTransportInternal* ice_transport) {
 
 void JsepTransport::SetRemoteIceParameters(
     IceTransportInternal* ice_transport) {
+  RTC_DCHECK_RUN_ON(network_thread_);
   RTC_DCHECK(ice_transport);
   RTC_DCHECK(remote_description_);
   ice_transport->SetRemoteIceParameters(
@@ -397,6 +413,7 @@ webrtc::RTCError JsepTransport::SetNegotiatedDtlsParameters(
     DtlsTransportInternal* dtls_transport,
     absl::optional<rtc::SSLRole> dtls_role,
     rtc::SSLFingerprint* remote_fingerprint) {
+  RTC_DCHECK_RUN_ON(network_thread_);
   RTC_DCHECK(dtls_transport);
   // Set SSL role. Role must be set before fingerprint is applied, which
   // initiates DTLS setup.
@@ -418,6 +435,7 @@ webrtc::RTCError JsepTransport::SetNegotiatedDtlsParameters(
 bool JsepTransport::SetRtcpMux(bool enable,
                                webrtc::SdpType type,
                                ContentSource source) {
+  RTC_DCHECK_RUN_ON(network_thread_);
   bool ret = false;
   switch (type) {
     case SdpType::kOffer:
@@ -448,6 +466,12 @@ bool JsepTransport::SetRtcpMux(bool enable,
 }
 
 void JsepTransport::ActivateRtcpMux() {
+  {
+    // Don't hold the network_thread_ lock while calling other functions,
+    // since they might call other functions that call RTC_DCHECK_RUN_ON.
+    // TODO(https://crbug.com/webrtc/10318): Simplify when possible.
+    RTC_DCHECK_RUN_ON(network_thread_);
+  }
   if (unencrypted_rtp_transport_) {
     RTC_DCHECK(!sdes_transport_);
     RTC_DCHECK(!dtls_srtp_transport_);
@@ -463,7 +487,10 @@ void JsepTransport::ActivateRtcpMux() {
     dtls_srtp_transport_->SetDtlsTransports(rtp_dtls_transport(),
                                             /*rtcp_dtls_transport=*/nullptr);
   }
+  {
+    rtc::CritScope scope(&accessor_lock_);
     rtcp_dtls_transport_ = nullptr;  // Destroy this reference.
+  }
   // Notify the JsepTransportController to update the aggregate states.
   SignalRtcpMuxActive();
 }
@@ -472,6 +499,8 @@ bool JsepTransport::SetSdes(const std::vector<CryptoParams>& cryptos,
                             const std::vector<int>& encrypted_extension_ids,
                             webrtc::SdpType type,
                             ContentSource source) {
+  RTC_DCHECK_RUN_ON(network_thread_);
+  rtc::CritScope scope(&accessor_lock_);
   bool ret = false;
   ret = sdes_negotiator_.Process(cryptos, type, source);
   if (!ret) {
@@ -514,6 +543,7 @@ bool JsepTransport::SetSdes(const std::vector<CryptoParams>& cryptos,
 
 webrtc::RTCError JsepTransport::NegotiateAndSetDtlsParameters(
     SdpType local_description_type) {
+  RTC_DCHECK_RUN_ON(network_thread_);
   if (!local_description_ || !remote_description_) {
     return webrtc::RTCError(webrtc::RTCErrorType::INVALID_STATE,
                             "Applying an answer transport description "
@@ -550,19 +580,16 @@ webrtc::RTCError JsepTransport::NegotiateAndSetDtlsParameters(
   // between future SetRemote/SetLocal invocations and new transport
   // creation, we have the negotiation state saved until a new
   // negotiation happens.
-  RTC_DCHECK(rtp_dtls_transport_->internal());
+  RTC_DCHECK(rtp_dtls_transport());
   webrtc::RTCError error = SetNegotiatedDtlsParameters(
-      rtp_dtls_transport_->internal(), negotiated_dtls_role,
+      rtp_dtls_transport(), negotiated_dtls_role, remote_fingerprint.get());
-      remote_fingerprint.get());
   if (!error.ok()) {
     return error;
   }
 
-  if (rtcp_dtls_transport_) {
+  if (rtcp_dtls_transport()) {
-    RTC_DCHECK(rtcp_dtls_transport_->internal());
+    error = SetNegotiatedDtlsParameters(
-    error = SetNegotiatedDtlsParameters(rtcp_dtls_transport_->internal(),
+        rtcp_dtls_transport(), negotiated_dtls_role, remote_fingerprint.get());
-                                        negotiated_dtls_role,
-                                        remote_fingerprint.get());
   }
   return error;
 }
@@ -657,6 +684,8 @@ webrtc::RTCError JsepTransport::NegotiateDtlsRole(
 
 bool JsepTransport::GetTransportStats(DtlsTransportInternal* dtls_transport,
                                       TransportStats* stats) {
+  RTC_DCHECK_RUN_ON(network_thread_);
+  rtc::CritScope scope(&accessor_lock_);
   RTC_DCHECK(dtls_transport);
   TransportChannelStats substats;
   if (rtcp_dtls_transport_) {
@@ -681,7 +710,11 @@ void JsepTransport::OnStateChanged(webrtc::MediaTransportState state) {
   // TODO(bugs.webrtc.org/9719) This method currently fires on the network
   // thread, but media transport does not make such guarantees. We need to make
   // sure this callback is guaranteed to be executed on the network thread.
+  RTC_DCHECK_RUN_ON(network_thread_);
+  {
+    rtc::CritScope scope(&accessor_lock_);
     media_transport_state_ = state;
+  }
   SignalMediaTransportStateChanged();
 }
 

pc/jsep_transport.h

@@ -36,6 +36,7 @@
 #include "rtc_base/rtc_certificate.h"
 #include "rtc_base/ssl_stream_adapter.h"
 #include "rtc_base/third_party/sigslot/sigslot.h"
+#include "rtc_base/thread_checker.h"
 
 namespace cricket {
 
@@ -101,11 +102,13 @@ class JsepTransport : public sigslot::has_slots<>,
   // Needed in order to verify the local fingerprint.
   void SetLocalCertificate(
       const rtc::scoped_refptr<rtc::RTCCertificate>& local_certificate) {
+    RTC_DCHECK_RUN_ON(network_thread_);
     local_certificate_ = local_certificate;
   }
 
   // Return the local certificate provided by SetLocalCertificate.
   rtc::scoped_refptr<rtc::RTCCertificate> GetLocalCertificate() const {
+    RTC_DCHECK_RUN_ON(network_thread_);
     return local_certificate_;
   }
 
@@ -130,7 +133,10 @@ class JsepTransport : public sigslot::has_slots<>,
   // Returns true if the ICE restart flag above was set, and no ICE restart has
   // occurred yet for this transport (by applying a local description with
   // changed ufrag/password).
-  bool needs_ice_restart() const { return needs_ice_restart_; }
+  bool needs_ice_restart() const {
+    rtc::CritScope scope(&accessor_lock_);
+    return needs_ice_restart_;
+  }
 
   // Returns role if negotiated, or empty absl::optional if it hasn't been
   // negotiated yet.
@@ -140,14 +146,19 @@ class JsepTransport : public sigslot::has_slots<>,
   bool GetStats(TransportStats* stats);
 
   const JsepTransportDescription* local_description() const {
+    RTC_DCHECK_RUN_ON(network_thread_);
     return local_description_.get();
   }
 
   const JsepTransportDescription* remote_description() const {
+    RTC_DCHECK_RUN_ON(network_thread_);
     return remote_description_.get();
   }
 
   webrtc::RtpTransportInternal* rtp_transport() const {
+    // This method is called from the signaling thread, which means
+    // that a race is possible, making safety analysis complex.
+    // After fixing, this method should be marked "network thread only".
     if (dtls_srtp_transport_) {
       return dtls_srtp_transport_.get();
     } else if (sdes_transport_) {
@@ -158,6 +169,7 @@ class JsepTransport : public sigslot::has_slots<>,
   }
 
   const DtlsTransportInternal* rtp_dtls_transport() const {
+    rtc::CritScope scope(&accessor_lock_);
     if (rtp_dtls_transport_) {
       return rtp_dtls_transport_->internal();
     } else {
@@ -166,6 +178,7 @@ class JsepTransport : public sigslot::has_slots<>,
   }
 
   DtlsTransportInternal* rtp_dtls_transport() {
+    rtc::CritScope scope(&accessor_lock_);
     if (rtp_dtls_transport_) {
       return rtp_dtls_transport_->internal();
     } else {
@@ -174,6 +187,7 @@ class JsepTransport : public sigslot::has_slots<>,
   }
 
   const DtlsTransportInternal* rtcp_dtls_transport() const {
+    rtc::CritScope scope(&accessor_lock_);
     if (rtcp_dtls_transport_) {
       return rtcp_dtls_transport_->internal();
     } else {
@@ -182,6 +196,7 @@ class JsepTransport : public sigslot::has_slots<>,
   }
 
   DtlsTransportInternal* rtcp_dtls_transport() {
+    rtc::CritScope scope(&accessor_lock_);
     if (rtcp_dtls_transport_) {
       return rtcp_dtls_transport_->internal();
     } else {
@@ -190,6 +205,7 @@ class JsepTransport : public sigslot::has_slots<>,
   }
 
   rtc::scoped_refptr<webrtc::DtlsTransport> RtpDtlsTransport() {
+    rtc::CritScope scope(&accessor_lock_);
     return rtp_dtls_transport_;
   }
 
@@ -197,11 +213,13 @@ class JsepTransport : public sigslot::has_slots<>,
   // Note that media transport is owned by jseptransport and the pointer
   // to media transport will becomes invalid after destruction of jseptransport.
   webrtc::MediaTransportInterface* media_transport() const {
+    rtc::CritScope scope(&accessor_lock_);
     return media_transport_.get();
   }
 
   // Returns the latest media transport state.
   webrtc::MediaTransportState media_transport_state() const {
+    rtc::CritScope scope(&accessor_lock_);
     return media_transport_state_;
   }
 
@@ -271,36 +289,51 @@ class JsepTransport : public sigslot::has_slots<>,
   // Invoked whenever the state of the media transport changes.
   void OnStateChanged(webrtc::MediaTransportState state) override;
 
+  // Owning thread, for safety checks
+  const rtc::Thread* const network_thread_;
+  // Critical scope for fields accessed off-thread
+  // TODO(https://bugs.webrtc.org/10300): Stop doing this.
+  rtc::CriticalSection accessor_lock_;
   const std::string mid_;
   // needs-ice-restart bit as described in JSEP.
-  bool needs_ice_restart_ = false;
+  bool needs_ice_restart_ RTC_GUARDED_BY(accessor_lock_) = false;
-  rtc::scoped_refptr<rtc::RTCCertificate> local_certificate_;
+  rtc::scoped_refptr<rtc::RTCCertificate> local_certificate_
-  std::unique_ptr<JsepTransportDescription> local_description_;
+      RTC_GUARDED_BY(network_thread_);
-  std::unique_ptr<JsepTransportDescription> remote_description_;
+  std::unique_ptr<JsepTransportDescription> local_description_
+      RTC_GUARDED_BY(network_thread_);
+  std::unique_ptr<JsepTransportDescription> remote_description_
+      RTC_GUARDED_BY(network_thread_);
 
   // To avoid downcasting and make it type safe, keep three unique pointers for
   // different SRTP mode and only one of these is non-nullptr.
-  std::unique_ptr<webrtc::RtpTransport> unencrypted_rtp_transport_;
+  // Since these are const, the variables don't need locks;
-  std::unique_ptr<webrtc::SrtpTransport> sdes_transport_;
+  // accessing the objects depends on the objects' thread safety contract.
-  std::unique_ptr<webrtc::DtlsSrtpTransport> dtls_srtp_transport_;
+  const std::unique_ptr<webrtc::RtpTransport> unencrypted_rtp_transport_;
+  const std::unique_ptr<webrtc::SrtpTransport> sdes_transport_;
+  const std::unique_ptr<webrtc::DtlsSrtpTransport> dtls_srtp_transport_;
 
-  rtc::scoped_refptr<webrtc::DtlsTransport> rtp_dtls_transport_;
+  rtc::scoped_refptr<webrtc::DtlsTransport> rtp_dtls_transport_
-  rtc::scoped_refptr<webrtc::DtlsTransport> rtcp_dtls_transport_;
+      RTC_GUARDED_BY(accessor_lock_);
+  rtc::scoped_refptr<webrtc::DtlsTransport> rtcp_dtls_transport_
+      RTC_GUARDED_BY(accessor_lock_);
 
-  SrtpFilter sdes_negotiator_;
+  SrtpFilter sdes_negotiator_ RTC_GUARDED_BY(network_thread_);
-  RtcpMuxFilter rtcp_mux_negotiator_;
+  RtcpMuxFilter rtcp_mux_negotiator_ RTC_GUARDED_BY(network_thread_);
 
   // Cache the encrypted header extension IDs for SDES negoitation.
-  absl::optional<std::vector<int>> send_extension_ids_;
+  absl::optional<std::vector<int>> send_extension_ids_
-  absl::optional<std::vector<int>> recv_extension_ids_;
+      RTC_GUARDED_BY(network_thread_);
+  absl::optional<std::vector<int>> recv_extension_ids_
+      RTC_GUARDED_BY(network_thread_);
 
   // Optional media transport (experimental).
-  std::unique_ptr<webrtc::MediaTransportInterface> media_transport_;
+  std::unique_ptr<webrtc::MediaTransportInterface> media_transport_
+      RTC_GUARDED_BY(accessor_lock_);
 
   // If |media_transport_| is provided, this variable represents the state of
   // media transport.
-  webrtc::MediaTransportState media_transport_state_ =
+  webrtc::MediaTransportState media_transport_state_
-      webrtc::MediaTransportState::kPending;
+      RTC_GUARDED_BY(accessor_lock_) = webrtc::MediaTransportState::kPending;
 
   RTC_DISALLOW_COPY_AND_ASSIGN(JsepTransport);
 };

pc/peer_connection_end_to_end_unittest.cc

@@ -747,6 +747,24 @@ TEST_P(PeerConnectionEndToEndTest, TooManyDataChannelsOpenedBeforeConnecting) {
 
 #endif  // HAVE_SCTP
 
+TEST_P(PeerConnectionEndToEndTest, CanRestartIce) {
+  rtc::scoped_refptr<webrtc::AudioDecoderFactory> real_decoder_factory =
+      webrtc::CreateBuiltinAudioDecoderFactory();
+  CreatePcs(webrtc::CreateBuiltinAudioEncoderFactory(),
+            CreateForwardingMockDecoderFactory(real_decoder_factory.get()));
+  GetAndAddUserMedia();
+  Negotiate();
+  WaitForCallEstablished();
+  // Cause ICE restart to be requested.
+  auto config = caller_->pc()->GetConfiguration();
+  ASSERT_NE(PeerConnectionInterface::kRelay, config.type);
+  config.type = PeerConnectionInterface::kRelay;
+  webrtc::RTCError error;
+  ASSERT_TRUE(caller_->pc()->SetConfiguration(config, &error));
+  // When solving https://crbug.com/webrtc/10504, all we need to check
+  // is that we do not crash. We should also be testing that restart happens.
+}
+
 INSTANTIATE_TEST_SUITE_P(PeerConnectionEndToEndTest,
                          PeerConnectionEndToEndTest,
                          Values(SdpSemantics::kPlanB,