Revert^2 "[M120] JsepTransportController: Remove raw pointers to description objects"

This reverts commit bf2e30678e0e5b21f2a2d49180a13225fdcfaa1a.

Reason for revert: Time to reland the fix for M120 now.

Original change's description:
> Revert "[M120] JsepTransportController: Remove raw pointers to description objects"
>
> This reverts commit e79a99060f70a356b131d9f2f7497914984944d1.
>
> Reason for revert: Merged too early. Will re-land for the next spin.
>
> Original change's description:
> > [M120] JsepTransportController: Remove raw pointers to description objects
> >
> > Remove member variables that point to objects owned externally (in practice by SdpOfferAnswerHandler). The objects also live on the
> > signaling thread whereas JsepTransportController performs
> > operations on the network thread. Removing the raw pointers avoids
> > the risk of referencing the description objects after they've been
> > deleted or if the state is inconsistent across threads.
> >
> > (cherry picked from commit c56052001d)
> >
> > Bug: webrtc:1515832
> > No-Try: true
> > Change-Id: I852b2a3993964be817f93c46b5bc4b03121cde86
> > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/334061
> > Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
> > Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> > Cr-Original-Commit-Position: refs/heads/main@{#41505}
> > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/335180
> > Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
> > Cr-Commit-Position: refs/branch-heads/6099@{#2}
> > Cr-Branched-From: 507f1cc3270d0577f79882acbd78e63e66008f3d-refs/heads/main@{#41042}
>
> Change-Id: Id4bd21fbc8b7306de1aba0854815ada5c9333468
> No-Try: true
> Bug: chromium:1515832
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/335620
> Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
> Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
> Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
> Cr-Commit-Position: refs/branch-heads/6099@{#3}
> Cr-Branched-From: 507f1cc3270d0577f79882acbd78e63e66008f3d-refs/heads/main@{#41042}

No-Try: true
Bug: chromium:1515832
Change-Id: I13a24182908d7a616234d9c701bf7000a162df8e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/336282
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/branch-heads/6099@{#4}
Cr-Branched-From: 507f1cc3270d0577f79882acbd78e63e66008f3d-refs/heads/main@{#41042}

pc/jsep_transport_controller.cc

@@ -76,14 +76,18 @@ JsepTransportController::~JsepTransportController() {
 
 RTCError JsepTransportController::SetLocalDescription(
     SdpType type,
-    const cricket::SessionDescription* description) {
+    const cricket::SessionDescription* local_desc,
+    const cricket::SessionDescription* remote_desc) {
+  RTC_DCHECK(local_desc);
   TRACE_EVENT0("webrtc", "JsepTransportController::SetLocalDescription");
+
   if (!network_thread_->IsCurrent()) {
     return network_thread_->BlockingCall(
-        [=] { return SetLocalDescription(type, description); });
+        [=] { return SetLocalDescription(type, local_desc, remote_desc); });
   }
 
   RTC_DCHECK_RUN_ON(network_thread_);
+
   if (!initial_offerer_.has_value()) {
     initial_offerer_.emplace(type == SdpType::kOffer);
     if (*initial_offerer_) {
@@ -92,20 +96,22 @@ RTCError JsepTransportController::SetLocalDescription(
       SetIceRole_n(cricket::ICEROLE_CONTROLLED);
     }
   }
-  return ApplyDescription_n(/*local=*/true, type, description);
+  return ApplyDescription_n(/*local=*/true, type, local_desc, remote_desc);
 }
 
 RTCError JsepTransportController::SetRemoteDescription(
     SdpType type,
-    const cricket::SessionDescription* description) {
+    const cricket::SessionDescription* local_desc,
+    const cricket::SessionDescription* remote_desc) {
+  RTC_DCHECK(remote_desc);
   TRACE_EVENT0("webrtc", "JsepTransportController::SetRemoteDescription");
   if (!network_thread_->IsCurrent()) {
     return network_thread_->BlockingCall(
-        [=] { return SetRemoteDescription(type, description); });
+        [=] { return SetRemoteDescription(type, local_desc, remote_desc); });
   }
 
   RTC_DCHECK_RUN_ON(network_thread_);
-  return ApplyDescription_n(/*local=*/false, type, description);
+  return ApplyDescription_n(/*local=*/false, type, local_desc, remote_desc);
 }
 
 RtpTransportInternal* JsepTransportController::GetRtpTransport(
@@ -587,18 +593,20 @@ JsepTransportController::GetActiveDtlsTransports() {
 RTCError JsepTransportController::ApplyDescription_n(
     bool local,
     SdpType type,
-    const cricket::SessionDescription* description) {
+    const cricket::SessionDescription* local_desc,
+    const cricket::SessionDescription* remote_desc) {
   TRACE_EVENT0("webrtc", "JsepTransportController::ApplyDescription_n");
+
+  // Stash away the description object that we'll be applying (since this
+  // function is used for both local and remote).
+  const cricket::SessionDescription* description =
+      local ? local_desc : remote_desc;
+
   RTC_DCHECK(description);
 
-  if (local) {
-    local_desc_ = description;
-  } else {
-    remote_desc_ = description;
-  }
-
   RTCError error;
-  error = ValidateAndMaybeUpdateBundleGroups(local, type, description);
+  error =
+      ValidateAndMaybeUpdateBundleGroups(local, type, local_desc, remote_desc);
   if (!error.ok()) {
     return error;
   }
@@ -710,7 +718,11 @@ RTCError JsepTransportController::ApplyDescription_n(
 RTCError JsepTransportController::ValidateAndMaybeUpdateBundleGroups(
     bool local,
     SdpType type,
-    const cricket::SessionDescription* description) {
+    const cricket::SessionDescription* local_desc,
+    const cricket::SessionDescription* remote_desc) {
+  const cricket::SessionDescription* description =
+      local ? local_desc : remote_desc;
+
   RTC_DCHECK(description);
 
   std::vector<const cricket::ContentGroup*> new_bundle_groups =
@@ -776,72 +788,74 @@ RTCError JsepTransportController::ValidateAndMaybeUpdateBundleGroups(
       }
     }
   } else if (type == SdpType::kAnswer) {
-    std::vector<const cricket::ContentGroup*> offered_bundle_groups =
+    if ((local && remote_desc) || (!local && local_desc)) {
-        local ? remote_desc_->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE)
+      std::vector<const cricket::ContentGroup*> offered_bundle_groups =
-              : local_desc_->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE);
+          local ? remote_desc->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE)
+                : local_desc->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE);
 
-    std::map<std::string, const cricket::ContentGroup*>
+      std::map<std::string, const cricket::ContentGroup*>
-        offered_bundle_groups_by_mid;
+          offered_bundle_groups_by_mid;
-    for (const cricket::ContentGroup* offered_bundle_group :
+      for (const cricket::ContentGroup* offered_bundle_group :
-         offered_bundle_groups) {
+           offered_bundle_groups) {
-      for (const std::string& content_name :
+        for (const std::string& content_name :
-           offered_bundle_group->content_names()) {
+             offered_bundle_group->content_names()) {
-        offered_bundle_groups_by_mid[content_name] = offered_bundle_group;
+          offered_bundle_groups_by_mid[content_name] = offered_bundle_group;
-      }
-    }
-
-    std::map<const cricket::ContentGroup*, const cricket::ContentGroup*>
-        new_bundle_groups_by_offered_bundle_groups;
-    for (const cricket::ContentGroup* new_bundle_group : new_bundle_groups) {
-      if (!new_bundle_group->FirstContentName()) {
-        // Empty groups could be a subset of any group.
-        continue;
-      }
-      // The group in the answer (new_bundle_group) must have a corresponding
-      // group in the offer (original_group), because the answer groups may only
-      // be subsets of the offer groups.
-      auto it = offered_bundle_groups_by_mid.find(
-          *new_bundle_group->FirstContentName());
-      if (it == offered_bundle_groups_by_mid.end()) {
-        return RTCError(RTCErrorType::INVALID_PARAMETER,
-                        "A BUNDLE group was added in the answer that did not "
-                        "exist in the offer.");
-      }
-      const cricket::ContentGroup* offered_bundle_group = it->second;
-      if (new_bundle_groups_by_offered_bundle_groups.find(
-              offered_bundle_group) !=
-          new_bundle_groups_by_offered_bundle_groups.end()) {
-        return RTCError(RTCErrorType::INVALID_PARAMETER,
-                        "A MID in the answer has changed group.");
-      }
-      new_bundle_groups_by_offered_bundle_groups.insert(
-          std::make_pair(offered_bundle_group, new_bundle_group));
-      for (const std::string& content_name :
-           new_bundle_group->content_names()) {
-        it = offered_bundle_groups_by_mid.find(content_name);
-        // The BUNDLE group in answer should be a subset of offered group.
-        if (it == offered_bundle_groups_by_mid.end() ||
-            it->second != offered_bundle_group) {
-          return RTCError(RTCErrorType::INVALID_PARAMETER,
-                          "A BUNDLE group in answer contains a MID='" +
-                              content_name +
-                              "' that was not in the offered group.");
         }
       }
-    }
 
-    for (const auto& bundle_group : bundles_.bundle_groups()) {
+      std::map<const cricket::ContentGroup*, const cricket::ContentGroup*>
-      for (const std::string& content_name : bundle_group->content_names()) {
+          new_bundle_groups_by_offered_bundle_groups;
-        // An answer that removes m= sections from pre-negotiated BUNDLE group
+      for (const cricket::ContentGroup* new_bundle_group : new_bundle_groups) {
-        // without rejecting it, is invalid.
+        if (!new_bundle_group->FirstContentName()) {
-        auto it = new_bundle_groups_by_mid.find(content_name);
+          // Empty groups could be a subset of any group.
-        if (it == new_bundle_groups_by_mid.end()) {
+          continue;
-          auto* content_info = description->GetContentByName(content_name);
+        }
-          if (!content_info || !content_info->rejected) {
+        // The group in the answer (new_bundle_group) must have a corresponding
+        // group in the offer (original_group), because the answer groups may
+        // only be subsets of the offer groups.
+        auto it = offered_bundle_groups_by_mid.find(
+            *new_bundle_group->FirstContentName());
+        if (it == offered_bundle_groups_by_mid.end()) {
+          return RTCError(RTCErrorType::INVALID_PARAMETER,
+                          "A BUNDLE group was added in the answer that did not "
+                          "exist in the offer.");
+        }
+        const cricket::ContentGroup* offered_bundle_group = it->second;
+        if (new_bundle_groups_by_offered_bundle_groups.find(
+                offered_bundle_group) !=
+            new_bundle_groups_by_offered_bundle_groups.end()) {
+          return RTCError(RTCErrorType::INVALID_PARAMETER,
+                          "A MID in the answer has changed group.");
+        }
+        new_bundle_groups_by_offered_bundle_groups.insert(
+            std::make_pair(offered_bundle_group, new_bundle_group));
+        for (const std::string& content_name :
+             new_bundle_group->content_names()) {
+          it = offered_bundle_groups_by_mid.find(content_name);
+          // The BUNDLE group in answer should be a subset of offered group.
+          if (it == offered_bundle_groups_by_mid.end() ||
+              it->second != offered_bundle_group) {
             return RTCError(RTCErrorType::INVALID_PARAMETER,
-                            "Answer cannot remove m= section with mid='" +
+                            "A BUNDLE group in answer contains a MID='" +
                                 content_name +
-                                "' from already-established BUNDLE group.");
+                                "' that was not in the offered group.");
+          }
+        }
+      }
+
+      for (const auto& bundle_group : bundles_.bundle_groups()) {
+        for (const std::string& content_name : bundle_group->content_names()) {
+          // An answer that removes m= sections from pre-negotiated BUNDLE group
+          // without rejecting it, is invalid.
+          auto it = new_bundle_groups_by_mid.find(content_name);
+          if (it == new_bundle_groups_by_mid.end()) {
+            auto* content_info = description->GetContentByName(content_name);
+            if (!content_info || !content_info->rejected) {
+              return RTCError(RTCErrorType::INVALID_PARAMETER,
+                              "Answer cannot remove m= section with mid='" +
+                                  content_name +
+                                  "' from already-established BUNDLE group.");
+            }
           }
         }
       }

pc/jsep_transport_controller.h

@@ -161,11 +161,24 @@ class JsepTransportController : public sigslot::has_slots<> {
   // level, creating/destroying transport objects as needed and updating their
   // properties. This includes RTP, DTLS, and ICE (but not SCTP). At least not
   // yet? May make sense to in the future.
+  //
+  // `local_desc` must always be valid. If a remote description has previously
+  // been set via a call to `SetRemoteDescription()` then `remote_desc` should
+  // point to that description object in order to keep the current local and
+  // remote session descriptions in sync.
   RTCError SetLocalDescription(SdpType type,
-                               const cricket::SessionDescription* description);
+                               const cricket::SessionDescription* local_desc,
+                               const cricket::SessionDescription* remote_desc);
 
+  // Call to apply a remote description (See `SetLocalDescription()` for local).
+  //
+  // `remote_desc` must always be valid. If a local description has previously
+  // been set via a call to `SetLocalDescription()` then `local_desc` should
+  // point to that description object in order to keep the current local and
+  // remote session descriptions in sync.
   RTCError SetRemoteDescription(SdpType type,
-                                const cricket::SessionDescription* description);
+                                const cricket::SessionDescription* local_desc,
+                                const cricket::SessionDescription* remote_desc);
 
   // Get transports to be used for the provided `mid`. If bundling is enabled,
   // calling GetRtpTransport for multiple MIDs may yield the same object.
@@ -346,14 +359,23 @@ class JsepTransportController : public sigslot::has_slots<> {
   CallbackList<const cricket::CandidatePairChangeEvent&>
       signal_ice_candidate_pair_changed_ RTC_GUARDED_BY(network_thread_);
 
+  // Called from SetLocalDescription and SetRemoteDescription.
+  // When `local` is true, local_desc must be valid. Similarly when
+  // `local` is false, remote_desc must be valid. The description counterpart
+  // to the one that's being applied, may be nullptr but when it's supplied
+  // the counterpart description's content groups will  be kept up to date for
+  // `type == SdpType::kAnswer`.
   RTCError ApplyDescription_n(bool local,
                               SdpType type,
-                              const cricket::SessionDescription* description)
+                              const cricket::SessionDescription* local_desc,
+                              const cricket::SessionDescription* remote_desc)
       RTC_RUN_ON(network_thread_);
   RTCError ValidateAndMaybeUpdateBundleGroups(
       bool local,
       SdpType type,
-      const cricket::SessionDescription* description);
+      const cricket::SessionDescription* local_desc,
+      const cricket::SessionDescription* remote_desc)
+      RTC_RUN_ON(network_thread_);
   RTCError ValidateContent(const cricket::ContentInfo& content_info);
 
   void HandleRejectedContent(const cricket::ContentInfo& content_info)
@@ -502,8 +524,6 @@ class JsepTransportController : public sigslot::has_slots<> {
   const Config config_;
   bool active_reset_srtp_params_ RTC_GUARDED_BY(network_thread_);
 
-  const cricket::SessionDescription* local_desc_ = nullptr;
-  const cricket::SessionDescription* remote_desc_ = nullptr;
   absl::optional<bool> initial_offerer_;
 
   cricket::IceConfig ice_config_;

pc/sdp_offer_answer.cc

@@ -2011,8 +2011,11 @@ RTCError SdpOfferAnswerHandler::ReplaceRemoteDescription(
   const cricket::SessionDescription* session_desc =
       remote_description()->description();
 
+  const auto* local = local_description();
+
   // NOTE: This will perform a BlockingCall() to the network thread.
-  return transport_controller_s()->SetRemoteDescription(sdp_type, session_desc);
+  return transport_controller_s()->SetRemoteDescription(
+      sdp_type, local ? local->description() : nullptr, session_desc);
 }
 
 void SdpOfferAnswerHandler::ApplyRemoteDescription(
@@ -4954,13 +4957,15 @@ RTCError SdpOfferAnswerHandler::PushdownTransportDescription(
   if (source == cricket::CS_LOCAL) {
     const SessionDescriptionInterface* sdesc = local_description();
     RTC_DCHECK(sdesc);
-    return transport_controller_s()->SetLocalDescription(type,
+    const auto* remote = remote_description();
-                                                         sdesc->description());
+    return transport_controller_s()->SetLocalDescription(
+        type, sdesc->description(), remote ? remote->description() : nullptr);
   } else {
     const SessionDescriptionInterface* sdesc = remote_description();
     RTC_DCHECK(sdesc);
-    return transport_controller_s()->SetRemoteDescription(type,
+    const auto* local = local_description();
-                                                          sdesc->description());
+    return transport_controller_s()->SetRemoteDescription(
+        type, local ? local->description() : nullptr, sdesc->description());
   }
 }
 

test/peer_scenario/scenario_connection.cc

@@ -173,7 +173,9 @@ void ScenarioIceConnectionImpl::SetRemoteSdp(SdpType type,
       });
 
   auto res = jsep_controller_->SetRemoteDescription(
-      remote_description_->GetType(), remote_description_->description());
+      remote_description_->GetType(),
+      local_description_ ? local_description_->description() : nullptr,
+      remote_description_->description());
   RTC_CHECK(res.ok()) << res.message();
   RtpDemuxerCriteria criteria;
   for (const auto& content : remote_description_->description()->contents()) {
@@ -203,7 +205,8 @@ void ScenarioIceConnectionImpl::SetLocalSdp(SdpType type,
   RTC_DCHECK_RUN_ON(signaling_thread_);
   local_description_ = webrtc::CreateSessionDescription(type, local_sdp);
   auto res = jsep_controller_->SetLocalDescription(
-      local_description_->GetType(), local_description_->description());
+      local_description_->GetType(), local_description_->description(),
+      remote_description_ ? remote_description_->description() : nullptr);
   RTC_CHECK(res.ok()) << res.message();
   jsep_controller_->MaybeStartGathering();
 }