admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

JsepTransportController: Remove raw pointers to description objects

Browse Source 
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.

Bug: webrtc:1515832
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-Commit-Position: refs/heads/main@{#41505}
This commit is contained in:
Tommi 2024-01-11 22:15:27 +01:00 committed by WebRTC LUCI CQ
parent 4e3b101bc9
commit c56052001d
5 changed files with 472 additions and 327 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

164
pc/jsep_transport_controller.cc
View File

@ -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(
@ -563,18 +569,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;
}
@ -686,7 +694,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 =
@ -752,72 +764,74 @@ RTCError JsepTransportController::ValidateAndMaybeUpdateBundleGroups(
}
}
} else if (type == SdpType::kAnswer) {
std::vector<const cricket::ContentGroup*> offered_bundle_groups =
local ? remote_desc_->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE)
: local_desc_->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE);
if ((local && remote_desc) || (!local && local_desc)) {
std::vector<const cricket::ContentGroup*> offered_bundle_groups =
local ? remote_desc->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE)
: local_desc->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE);
std::map<std::string, const cricket::ContentGroup*>
offered_bundle_groups_by_mid;
for (const cricket::ContentGroup* offered_bundle_group :
offered_bundle_groups) {
for (const std::string& content_name :
offered_bundle_group->content_names()) {
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.");
std::map<std::string, const cricket::ContentGroup*>
offered_bundle_groups_by_mid;
for (const cricket::ContentGroup* offered_bundle_group :
offered_bundle_groups) {
for (const std::string& content_name :
offered_bundle_group->content_names()) {
offered_bundle_groups_by_mid[content_name] = offered_bundle_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) {
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,
"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.");
}
}
}
}

32
pc/jsep_transport_controller.h
View File

@ -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.
@ -325,14 +338,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)
@ -481,8 +503,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_;

581
pc/jsep_transport_controller_unittest.cc
View File

File diff suppressed because it is too large Load Diff

15
pc/sdp_offer_answer.cc
View File

@ -1996,8 +1996,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(
@ -4910,13 +4913,15 @@ RTCError SdpOfferAnswerHandler::PushdownTransportDescription(
if (source == cricket::CS_LOCAL) {
const SessionDescriptionInterface* sdesc = local_description();
RTC_DCHECK(sdesc);
return transport_controller_s()->SetLocalDescription(type,
sdesc->description());
const auto* remote = remote_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,
sdesc->description());
const auto* local = local_description();
return transport_controller_s()->SetRemoteDescription(
type, local ? local->description() : nullptr, sdesc->description());
}
}

7
test/peer_scenario/scenario_connection.cc
View File

@ -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()) {
@ -194,7 +196,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();
}