Handle the corner cases for BUNDLE.

Reject the local/remote description trying to change the pre-negotiated
BUNDLE tag.

Reject an answer containing a BUNDLE group that's not a subset of the offered group.

Reject an offer/answer with a BUNDLE group containing a MID that no m= section has.

Reject an answer removes an m= section from an established BUNDLE group without
rejecting it.

Bug: chromium:827917
Change-Id: If334eefb00b1c1c1e24f9afba0cb00b5867f5590
Reviewed-on: https://webrtc-review.googlesource.com/67190
Commit-Queue: Zhi Huang <zhihuang@webrtc.org>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22813}

media/sctp/sctptransport.h

@@ -140,7 +140,7 @@ class SctpTransport : public SctpTransportInternal,
   // Helps pass inbound/outbound packets asynchronously to the network thread.
   rtc::AsyncInvoker invoker_;
   // Underlying DTLS channel.
-  rtc::PacketTransportInternal* transport_;
+  rtc::PacketTransportInternal* transport_ = nullptr;
   bool was_ever_writable_ = false;
   int local_port_ = kSctpDefaultPort;
   int remote_port_ = kSctpDefaultPort;

pc/jseptransportcontroller.cc

@@ -543,17 +543,8 @@ RTCError JsepTransportController::ApplyDescription_n(
     remote_desc_ = description;
   }
 
-  if (ShouldUpdateBundleGroup(type, description)) {
-    if (!description->HasGroup(cricket::GROUP_TYPE_BUNDLE)) {
-      return RTCError(RTCErrorType::INVALID_PARAMETER,
-                      "max-bundle is used but no bundle group found.");
-    } else {
-      bundle_group_ = *description->GetGroupByName(cricket::GROUP_TYPE_BUNDLE);
-    }
-  }
-
   RTCError error;
-  error = ValidateBundleGroup(description);
+  error = ValidateAndMaybeUpdateBundleGroup(local, type, description);
   if (!error.ok()) {
     return error;
   }
@@ -570,7 +561,7 @@ RTCError JsepTransportController::ApplyDescription_n(
         (IsBundled(content_info.name) && content_info.name != *bundled_mid())) {
       continue;
     }
-    error = MaybeCreateJsepTransport(content_info.name, content_info);
+    error = MaybeCreateJsepTransport(content_info);
     if (!error.ok()) {
       return error;
     }
@@ -583,7 +574,7 @@ RTCError JsepTransportController::ApplyDescription_n(
     const cricket::TransportInfo& transport_info =
         description->transport_infos()[i];
     if (content_info.rejected) {
-      HandleRejectedContent(content_info);
+      HandleRejectedContent(content_info, description);
       continue;
     }
 
@@ -633,9 +624,75 @@ RTCError JsepTransportController::ApplyDescription_n(
   return RTCError::OK();
 }
 
-RTCError JsepTransportController::ValidateBundleGroup(
+RTCError JsepTransportController::ValidateAndMaybeUpdateBundleGroup(
+    bool local,
+    SdpType type,
     const cricket::SessionDescription* description) {
   RTC_DCHECK(description);
+  const cricket::ContentGroup* new_bundle_group =
+      description->GetGroupByName(cricket::GROUP_TYPE_BUNDLE);
+
+  // The BUNDLE group containing a MID that no m= section has is invalid.
+  if (new_bundle_group) {
+    for (auto content_name : new_bundle_group->content_names()) {
+      if (!description->GetContentByName(content_name)) {
+        return RTCError(RTCErrorType::INVALID_PARAMETER,
+                        "The BUNDLE group contains MID:" + content_name +
+                            " matching no m= section.");
+      }
+    }
+  }
+
+  if (type == SdpType::kAnswer) {
+    const cricket::ContentGroup* offered_bundle_group =
+        local ? remote_desc_->GetGroupByName(cricket::GROUP_TYPE_BUNDLE)
+              : local_desc_->GetGroupByName(cricket::GROUP_TYPE_BUNDLE);
+
+    if (new_bundle_group) {
+      // The BUNDLE group in answer should be a subset of offered group.
+      for (auto content_name : new_bundle_group->content_names()) {
+        if (!offered_bundle_group ||
+            !offered_bundle_group->HasContentName(content_name)) {
+          return RTCError(RTCErrorType::INVALID_PARAMETER,
+                          "The BUNDLE group in answer contains a MID that was "
+                          "not in the offered group.");
+        }
+      }
+    }
+
+    if (bundle_group_) {
+      for (auto content_name : bundle_group_->content_names()) {
+        // An answer that removes m= sections from pre-negotiated BUNDLE group
+        // without rejecting it, is invalid.
+        if (!new_bundle_group ||
+            !new_bundle_group->HasContentName(content_name)) {
+          auto* content_info = description->GetContentByName(content_name);
+          if (!content_info || !content_info->rejected) {
+            return RTCError(RTCErrorType::INVALID_PARAMETER,
+                            "Answer cannot remove m= section  " + content_name +
+                                " from already-established BUNDLE group.");
+          }
+        }
+      }
+    }
+  }
+
+  if (config_.bundle_policy ==
+          PeerConnectionInterface::kBundlePolicyMaxBundle &&
+      !description->HasGroup(cricket::GROUP_TYPE_BUNDLE)) {
+    return RTCError(RTCErrorType::INVALID_PARAMETER,
+                    "max-bundle is used but no bundle group found.");
+  }
+
+  if (ShouldUpdateBundleGroup(type, description)) {
+    std::string new_bundled_mid = *(new_bundle_group->FirstContentName());
+    if (bundled_mid() && *bundled_mid() != new_bundled_mid) {
+      return RTCError(RTCErrorType::UNSUPPORTED_OPERATION,
+                      "Changing the negotiated BUNDLE-tag is not supported.");
+    }
+
+    bundle_group_ = *new_bundle_group;
+  }
 
   if (!bundled_mid()) {
     return RTCError::OK();
@@ -679,24 +736,20 @@ RTCError JsepTransportController::ValidateContent(
 }
 
 void JsepTransportController::HandleRejectedContent(
-    const cricket::ContentInfo& content_info) {
+    const cricket::ContentInfo& content_info,
+    const cricket::SessionDescription* description) {
   // If the content is rejected, let the
   // BaseChannel/SctpTransport change the RtpTransport/DtlsTransport first,
   // then destroy the cricket::JsepTransport2.
-  if (content_info.type == cricket::MediaProtocolType::kRtp) {
-    SignalRtpTransportChanged(content_info.name, nullptr);
-  } else {
-    SignalDtlsTransportChanged(content_info.name, nullptr);
-  }
+  RemoveTransportForMid(content_info.name, content_info.type);
   // If the answerer rejects the first content, which other contents are bundled
   // on, all the other contents in the bundle group will be rejected.
   if (content_info.name == bundled_mid()) {
     for (auto content_name : bundle_group_->content_names()) {
-      if (content_info.type == cricket::MediaProtocolType::kRtp) {
-        SignalRtpTransportChanged(content_name, nullptr);
-      } else {
-        SignalDtlsTransportChanged(content_name, nullptr);
-      }
+      const cricket::ContentInfo* content_in_group =
+          description->GetContentByName(content_name);
+      RTC_DCHECK(content_in_group);
+      RemoveTransportForMid(content_name, content_in_group->type);
     }
     bundle_group_.reset();
   } else if (IsBundled(content_info.name)) {
@@ -712,21 +765,42 @@ void JsepTransportController::HandleRejectedContent(
 
 void JsepTransportController::HandleBundledContent(
     const cricket::ContentInfo& content_info) {
+  auto jsep_transport = GetJsepTransportByName(*bundled_mid());
+  RTC_DCHECK(jsep_transport);
   // If the content is bundled, let the
   // BaseChannel/SctpTransport change the RtpTransport/DtlsTransport first,
   // then destroy the cricket::JsepTransport2.
-  if (content_info.type == cricket::MediaProtocolType::kRtp) {
-    auto rtp_transport =
-        jsep_transports_by_name_[*bundled_mid()]->rtp_transport();
-    SignalRtpTransportChanged(content_info.name, rtp_transport);
-  } else {
-    auto dtls_transport =
-        jsep_transports_by_name_[*bundled_mid()]->rtp_dtls_transport();
-    SignalDtlsTransportChanged(content_info.name, dtls_transport);
-  }
+  SetTransportForMid(content_info.name, jsep_transport, content_info.type);
   MaybeDestroyJsepTransport(content_info.name);
 }
 
+void JsepTransportController::SetTransportForMid(
+    const std::string& mid,
+    cricket::JsepTransport2* jsep_transport,
+    cricket::MediaProtocolType protocol_type) {
+  if (mid_to_transport_[mid] == jsep_transport) {
+    return;
+  }
+
+  mid_to_transport_[mid] = jsep_transport;
+  if (protocol_type == cricket::MediaProtocolType::kRtp) {
+    SignalRtpTransportChanged(mid, jsep_transport->rtp_transport());
+  } else {
+    SignalDtlsTransportChanged(mid, jsep_transport->rtp_dtls_transport());
+  }
+}
+
+void JsepTransportController::RemoveTransportForMid(
+    const std::string& mid,
+    cricket::MediaProtocolType protocol_type) {
+  if (protocol_type == cricket::MediaProtocolType::kRtp) {
+    SignalRtpTransportChanged(mid, nullptr);
+  } else {
+    SignalDtlsTransportChanged(mid, nullptr);
+  }
+  mid_to_transport_.erase(mid);
+}
+
 cricket::JsepTransportDescription
 JsepTransportController::CreateJsepTransportDescription(
     cricket::ContentInfo content_info,
@@ -832,22 +906,14 @@ int JsepTransportController::GetRtpAbsSendTimeHeaderExtensionId(
 
 const cricket::JsepTransport2* JsepTransportController::GetJsepTransportForMid(
     const std::string& mid) const {
-  auto target_mid = mid;
-  if (IsBundled(mid)) {
-    target_mid = *bundled_mid();
-  }
-  auto it = jsep_transports_by_name_.find(target_mid);
-  return (it == jsep_transports_by_name_.end()) ? nullptr : it->second.get();
+  auto it = mid_to_transport_.find(mid);
+  return it == mid_to_transport_.end() ? nullptr : it->second;
 }
 
 cricket::JsepTransport2* JsepTransportController::GetJsepTransportForMid(
     const std::string& mid) {
-  auto target_mid = mid;
-  if (IsBundled(mid)) {
-    target_mid = *bundled_mid();
-  }
-  auto it = jsep_transports_by_name_.find(target_mid);
-  return (it == jsep_transports_by_name_.end()) ? nullptr : it->second.get();
+  auto it = mid_to_transport_.find(mid);
+  return it == mid_to_transport_.end() ? nullptr : it->second;
 }
 
 const cricket::JsepTransport2* JsepTransportController::GetJsepTransportByName(
@@ -863,10 +929,10 @@ cricket::JsepTransport2* JsepTransportController::GetJsepTransportByName(
 }
 
 RTCError JsepTransportController::MaybeCreateJsepTransport(
-    const std::string& mid,
     const cricket::ContentInfo& content_info) {
   RTC_DCHECK(network_thread_->IsCurrent());
-  cricket::JsepTransport2* transport = GetJsepTransportForMid(mid);
+  cricket::JsepTransport2* transport =
+      GetJsepTransportByName(content_info.name);
   if (transport) {
     return RTCError::OK();
   }
@@ -880,12 +946,13 @@ RTCError JsepTransportController::MaybeCreateJsepTransport(
   }
 
   std::unique_ptr<cricket::DtlsTransportInternal> rtp_dtls_transport =
-      CreateDtlsTransport(mid, /*rtcp =*/false);
+      CreateDtlsTransport(content_info.name, /*rtcp =*/false);
   std::unique_ptr<cricket::DtlsTransportInternal> rtcp_dtls_transport;
   if (config_.rtcp_mux_policy !=
           PeerConnectionInterface::kRtcpMuxPolicyRequire &&
       content_info.type == cricket::MediaProtocolType::kRtp) {
-    rtcp_dtls_transport = CreateDtlsTransport(mid, /*rtcp =*/true);
+    rtcp_dtls_transport =
+        CreateDtlsTransport(content_info.name, /*rtcp =*/true);
   }
 
   std::unique_ptr<RtpTransport> unencrypted_rtp_transport;
@@ -893,30 +960,44 @@ RTCError JsepTransportController::MaybeCreateJsepTransport(
   std::unique_ptr<DtlsSrtpTransport> dtls_srtp_transport;
   if (config_.disable_encryption) {
     unencrypted_rtp_transport = CreateUnencryptedRtpTransport(
-        mid, rtp_dtls_transport.get(), rtcp_dtls_transport.get());
+        content_info.name, rtp_dtls_transport.get(), rtcp_dtls_transport.get());
   } else if (!content_desc->cryptos().empty()) {
-    sdes_transport = CreateSdesTransport(mid, rtp_dtls_transport.get(),
-                                         rtcp_dtls_transport.get());
+    sdes_transport = CreateSdesTransport(
+        content_info.name, rtp_dtls_transport.get(), rtcp_dtls_transport.get());
   } else {
-    dtls_srtp_transport = CreateDtlsSrtpTransport(mid, rtp_dtls_transport.get(),
-                                                  rtcp_dtls_transport.get());
+    dtls_srtp_transport = CreateDtlsSrtpTransport(
+        content_info.name, rtp_dtls_transport.get(), rtcp_dtls_transport.get());
   }
 
   std::unique_ptr<cricket::JsepTransport2> jsep_transport =
       rtc::MakeUnique<cricket::JsepTransport2>(
-          mid, certificate_, std::move(unencrypted_rtp_transport),
+          content_info.name, certificate_, std::move(unencrypted_rtp_transport),
           std::move(sdes_transport), std::move(dtls_srtp_transport),
           std::move(rtp_dtls_transport), std::move(rtcp_dtls_transport));
   jsep_transport->SignalRtcpMuxActive.connect(
       this, &JsepTransportController::UpdateAggregateStates_n);
-  jsep_transports_by_name_[mid] = std::move(jsep_transport);
-  UpdateAggregateStates_n();
+  SetTransportForMid(content_info.name, jsep_transport.get(),
+                     content_info.type);
 
+  jsep_transports_by_name_[content_info.name] = std::move(jsep_transport);
+  UpdateAggregateStates_n();
   return RTCError::OK();
 }
 
 void JsepTransportController::MaybeDestroyJsepTransport(
     const std::string& mid) {
+  auto jsep_transport = GetJsepTransportByName(mid);
+  if (!jsep_transport) {
+    return;
+  }
+
+  // Don't destroy the JsepTransport if there are still media sections referring
+  // to it.
+  for (const auto& kv : mid_to_transport_) {
+    if (kv.second == jsep_transport) {
+      return;
+    }
+  }
   jsep_transports_by_name_.erase(mid);
   UpdateAggregateStates_n();
 }

pc/jseptransportcontroller.h

@@ -177,12 +177,22 @@ class JsepTransportController : public sigslot::has_slots<>,
   RTCError ApplyDescription_n(bool local,
                               SdpType type,
                               const cricket::SessionDescription* description);
-  RTCError ValidateBundleGroup(const cricket::SessionDescription* description);
+  RTCError ValidateAndMaybeUpdateBundleGroup(
+      bool local,
+      SdpType type,
+      const cricket::SessionDescription* description);
   RTCError ValidateContent(const cricket::ContentInfo& content_info);
 
-  void HandleRejectedContent(const cricket::ContentInfo& content_info);
+  void HandleRejectedContent(const cricket::ContentInfo& content_info,
+                             const cricket::SessionDescription* description);
   void HandleBundledContent(const cricket::ContentInfo& content_info);
 
+  void SetTransportForMid(const std::string& mid,
+                          cricket::JsepTransport2* jsep_transport,
+                          cricket::MediaProtocolType protocol_type);
+  void RemoveTransportForMid(const std::string& mid,
+                             cricket::MediaProtocolType protocol_type);
+
   cricket::JsepTransportDescription CreateJsepTransportDescription(
       cricket::ContentInfo content_info,
       cricket::TransportInfo transport_info,
@@ -227,8 +237,7 @@ class JsepTransportController : public sigslot::has_slots<>,
   cricket::JsepTransport2* GetJsepTransportByName(
       const std::string& transport_name);
 
-  RTCError MaybeCreateJsepTransport(const std::string& mid,
-                                    const cricket::ContentInfo& content_info);
+  RTCError MaybeCreateJsepTransport(const cricket::ContentInfo& content_info);
   void MaybeDestroyJsepTransport(const std::string& mid);
   void DestroyAllJsepTransports_n();
 
@@ -284,6 +293,9 @@ class JsepTransportController : public sigslot::has_slots<>,
 
   std::map<std::string, std::unique_ptr<cricket::JsepTransport2>>
       jsep_transports_by_name_;
+  // This keeps track of the mapping between media section
+  // (BaseChannel/SctpTransport) and the JsepTransport2 underneath.
+  std::map<std::string, cricket::JsepTransport2*> mid_to_transport_;
 
   // Aggregate state for Transports.
   cricket::IceConnectionState ice_connection_state_ =

pc/jseptransportcontroller_unittest.cc

@@ -1053,7 +1053,7 @@ TEST_F(JsepTransportControllerTest, BundleSubsetOfMediaSections) {
   ASSERT_TRUE(it != changed_rtp_transport_by_mid_.end());
   EXPECT_EQ(transport1, it->second);
   it = changed_rtp_transport_by_mid_.find(kAudioMid2);
-  EXPECT_TRUE(it == changed_rtp_transport_by_mid_.end());
+  EXPECT_TRUE(transport2 == it->second);
 }
 
 // Tests that the initial offer/answer only have data section and audio/video
@@ -1325,4 +1325,88 @@ TEST_F(JsepTransportControllerTest, ApplyNonRtcpMuxAnswerWhenMuxingRequired) {
                    .ok());
 }
 
+// This tests that the BUNDLE group in answer should be a subset of the offered
+// group.
+TEST_F(JsepTransportControllerTest,
+       AddContentToBundleGroupInAnswerNotSupported) {
+  CreateJsepTransportController(JsepTransportController::Config());
+  auto local_offer = CreateSessionDescriptionWithoutBundle();
+  auto remote_answer = CreateSessionDescriptionWithoutBundle();
+
+  cricket::ContentGroup offer_bundle_group(cricket::GROUP_TYPE_BUNDLE);
+  offer_bundle_group.AddContentName(kAudioMid1);
+  local_offer->AddGroup(offer_bundle_group);
+
+  cricket::ContentGroup answer_bundle_group(cricket::GROUP_TYPE_BUNDLE);
+  answer_bundle_group.AddContentName(kAudioMid1);
+  answer_bundle_group.AddContentName(kVideoMid1);
+  remote_answer->AddGroup(answer_bundle_group);
+  EXPECT_TRUE(transport_controller_
+                  ->SetLocalDescription(SdpType::kOffer, local_offer.get())
+                  .ok());
+  EXPECT_FALSE(transport_controller_
+                   ->SetRemoteDescription(SdpType::kAnswer, remote_answer.get())
+                   .ok());
+}
+
+// This tests that the BUNDLE group with non-existing MID should be rejectd.
+TEST_F(JsepTransportControllerTest, RejectBundleGroupWithNonExistingMid) {
+  CreateJsepTransportController(JsepTransportController::Config());
+  auto local_offer = CreateSessionDescriptionWithoutBundle();
+  auto remote_answer = CreateSessionDescriptionWithoutBundle();
+
+  cricket::ContentGroup invalid_bundle_group(cricket::GROUP_TYPE_BUNDLE);
+  // The BUNDLE group is invalid because there is no data section in the
+  // description.
+  invalid_bundle_group.AddContentName(kDataMid1);
+  local_offer->AddGroup(invalid_bundle_group);
+  remote_answer->AddGroup(invalid_bundle_group);
+
+  EXPECT_FALSE(transport_controller_
+                   ->SetLocalDescription(SdpType::kOffer, local_offer.get())
+                   .ok());
+  EXPECT_FALSE(transport_controller_
+                   ->SetRemoteDescription(SdpType::kAnswer, remote_answer.get())
+                   .ok());
+}
+
+// This tests that an answer shouldn't be able to remove an m= section from an
+// established group without rejecting it.
+TEST_F(JsepTransportControllerTest, RemoveContentFromBundleGroup) {
+  CreateJsepTransportController(JsepTransportController::Config());
+
+  auto local_offer = CreateSessionDescriptionWithBundleGroup();
+  auto remote_answer = CreateSessionDescriptionWithBundleGroup();
+  EXPECT_TRUE(transport_controller_
+                  ->SetLocalDescription(SdpType::kOffer, local_offer.get())
+                  .ok());
+  EXPECT_TRUE(transport_controller_
+                  ->SetRemoteDescription(SdpType::kAnswer, remote_answer.get())
+                  .ok());
+
+  // Do an re-offer/answer.
+  EXPECT_TRUE(transport_controller_
+                  ->SetLocalDescription(SdpType::kOffer, local_offer.get())
+                  .ok());
+  auto new_answer = CreateSessionDescriptionWithoutBundle();
+  cricket::ContentGroup new_bundle_group(cricket::GROUP_TYPE_BUNDLE);
+  //  The answer removes video from the BUNDLE group without rejecting it is
+  //  invalid.
+  new_bundle_group.AddContentName(kAudioMid1);
+  new_answer->AddGroup(new_bundle_group);
+
+  // Applying invalid answer is expected to fail.
+  EXPECT_FALSE(transport_controller_
+                   ->SetRemoteDescription(SdpType::kAnswer, new_answer.get())
+                   .ok());
+
+  // Rejected the video content.
+  auto video_content = new_answer->GetContentByName(kVideoMid1);
+  ASSERT_TRUE(video_content);
+  video_content->rejected = true;
+  EXPECT_TRUE(transport_controller_
+                  ->SetRemoteDescription(SdpType::kAnswer, new_answer.get())
+                  .ok());
+}
+
 }  // namespace webrtc

pc/peerconnection_bundle_unittest.cc

@@ -659,6 +659,146 @@ TEST_P(PeerConnectionBundleTest, BundleOnFirstMidInAnswer) {
   EXPECT_EQ(caller->voice_rtp_transport(), caller->video_rtp_transport());
 }
 
+// This tests that changing the pre-negotiated BUNDLE tag is not supported.
+TEST_P(PeerConnectionBundleTest, RejectDescriptionChangingBundleTag) {
+  RTCConfiguration config;
+  config.bundle_policy = BundlePolicy::kBundlePolicyMaxBundle;
+  auto caller = CreatePeerConnectionWithAudioVideo(config);
+  auto callee = CreatePeerConnectionWithAudioVideo(config);
+
+  RTCOfferAnswerOptions options;
+  options.use_rtp_mux = true;
+  auto offer = caller->CreateOfferAndSetAsLocal(options);
+
+  // Create a new bundle-group with different bundled_mid.
+  auto* old_bundle_group =
+      offer->description()->GetGroupByName(cricket::GROUP_TYPE_BUNDLE);
+  std::string first_mid = old_bundle_group->content_names()[0];
+  std::string second_mid = old_bundle_group->content_names()[1];
+  cricket::ContentGroup new_bundle_group(cricket::GROUP_TYPE_BUNDLE);
+  new_bundle_group.AddContentName(second_mid);
+
+  auto re_offer = CloneSessionDescription(offer.get());
+  callee->SetRemoteDescription(std::move(offer));
+  auto answer = callee->CreateAnswer(options);
+  // Reject the first MID.
+  answer->description()->contents()[0].rejected = true;
+  // Remove the first MID from the bundle group.
+  answer->description()->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE);
+  answer->description()->AddGroup(new_bundle_group);
+  // The answer is expected to be rejected.
+  EXPECT_FALSE(caller->SetRemoteDescription(std::move(answer)));
+
+  // Do the same thing for re-offer.
+  re_offer->description()->contents()[0].rejected = true;
+  re_offer->description()->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE);
+  re_offer->description()->AddGroup(new_bundle_group);
+  // The re-offer is expected to be rejected.
+  EXPECT_FALSE(caller->SetLocalDescription(std::move(re_offer)));
+}
+
+// This tests that removing contents from BUNDLE group and reject the whole
+// BUNDLE group could work. This is a regression test for
+// (https://bugs.chromium.org/p/chromium/issues/detail?id=827917)
+TEST_P(PeerConnectionBundleTest, RemovingContentAndRejectBundleGroup) {
+  RTCConfiguration config;
+#ifndef HAVE_SCTP
+  config.enable_rtp_data_channel = true;
+#endif
+  config.bundle_policy = BundlePolicy::kBundlePolicyMaxBundle;
+  auto caller = CreatePeerConnectionWithAudioVideo(config);
+  caller->CreateDataChannel("dc");
+
+  auto offer = caller->CreateOfferAndSetAsLocal();
+  auto re_offer = CloneSessionDescription(offer.get());
+
+  // Removing the second MID from the BUNDLE group.
+  auto* old_bundle_group =
+      offer->description()->GetGroupByName(cricket::GROUP_TYPE_BUNDLE);
+  std::string first_mid = old_bundle_group->content_names()[0];
+  std::string third_mid = old_bundle_group->content_names()[2];
+  cricket::ContentGroup new_bundle_group(cricket::GROUP_TYPE_BUNDLE);
+  new_bundle_group.AddContentName(first_mid);
+  new_bundle_group.AddContentName(third_mid);
+
+  // Reject the entire new bundle group.
+  re_offer->description()->contents()[0].rejected = true;
+  re_offer->description()->contents()[2].rejected = true;
+  re_offer->description()->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE);
+  re_offer->description()->AddGroup(new_bundle_group);
+
+  EXPECT_TRUE(caller->SetLocalDescription(std::move(re_offer)));
+}
+
+// This tests that the BUNDLE group in answer should be a subset of the offered
+// group.
+TEST_P(PeerConnectionBundleTest, AddContentToBundleGroupInAnswerNotSupported) {
+  auto caller = CreatePeerConnectionWithAudioVideo();
+  auto callee = CreatePeerConnectionWithAudioVideo();
+
+  auto offer = caller->CreateOffer();
+  std::string first_mid = offer->description()->contents()[0].name;
+  std::string second_mid = offer->description()->contents()[1].name;
+
+  cricket::ContentGroup bundle_group(cricket::GROUP_TYPE_BUNDLE);
+  bundle_group.AddContentName(first_mid);
+  offer->description()->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE);
+  offer->description()->AddGroup(bundle_group);
+  EXPECT_TRUE(
+      caller->SetLocalDescription(CloneSessionDescription(offer.get())));
+  EXPECT_TRUE(callee->SetRemoteDescription(std::move(offer)));
+
+  auto answer = callee->CreateAnswer();
+  bundle_group.AddContentName(second_mid);
+  answer->description()->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE);
+  answer->description()->AddGroup(bundle_group);
+
+  // The answer is expected to be rejected because second mid is not in the
+  // offered BUNDLE group.
+  EXPECT_FALSE(callee->SetLocalDescription(std::move(answer)));
+}
+
+// This tests that the BUNDLE group with non-existing MID should be rejectd.
+TEST_P(PeerConnectionBundleTest, RejectBundleGroupWithNonExistingMid) {
+  auto caller = CreatePeerConnectionWithAudioVideo();
+  auto callee = CreatePeerConnectionWithAudioVideo();
+
+  auto offer = caller->CreateOffer();
+  auto invalid_bundle_group =
+      *offer->description()->GetGroupByName(cricket::GROUP_TYPE_BUNDLE);
+  invalid_bundle_group.AddContentName("non-existing-MID");
+  offer->description()->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE);
+  offer->description()->AddGroup(invalid_bundle_group);
+
+  EXPECT_FALSE(
+      caller->SetLocalDescription(CloneSessionDescription(offer.get())));
+  EXPECT_FALSE(callee->SetRemoteDescription(std::move(offer)));
+}
+
+// This tests that an answer shouldn't be able to remove an m= section from an
+// established group without rejecting it.
+TEST_P(PeerConnectionBundleTest, RemoveContentFromBundleGroup) {
+  auto caller = CreatePeerConnectionWithAudioVideo();
+  auto callee = CreatePeerConnectionWithAudioVideo();
+
+  EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
+  EXPECT_TRUE(
+      caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal()));
+
+  EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
+  auto answer = callee->CreateAnswer();
+  std::string second_mid = answer->description()->contents()[1].name;
+
+  auto invalid_bundle_group =
+      *answer->description()->GetGroupByName(cricket::GROUP_TYPE_BUNDLE);
+  invalid_bundle_group.RemoveContentName(second_mid);
+  answer->description()->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE);
+  answer->description()->AddGroup(invalid_bundle_group);
+
+  EXPECT_FALSE(
+      callee->SetLocalDescription(CloneSessionDescription(answer.get())));
+}
+
 INSTANTIATE_TEST_CASE_P(PeerConnectionBundleTest,
                         PeerConnectionBundleTest,
                         Values(SdpSemantics::kPlanB,

pc/peerconnection_media_unittest.cc

@@ -955,6 +955,14 @@ void RenameContent(cricket::SessionDescription* desc,
   auto* transport = desc->GetTransportInfoByName(old_name);
   RTC_DCHECK(transport);
   transport->content_name = new_name;
+
+  // Rename the content name in the BUNDLE group.
+  cricket::ContentGroup new_bundle_group =
+      *desc->GetGroupByName(cricket::GROUP_TYPE_BUNDLE);
+  new_bundle_group.RemoveContentName(old_name);
+  new_bundle_group.AddContentName(new_name);
+  desc->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE);
+  desc->AddGroup(new_bundle_group);
 }
 
 // Tests that an answer responds with the same MIDs as the offer.