Skip to content

Commit

Permalink
Bug 1876843 - Vendor libwebrtc from 0f741da200
Browse files Browse the repository at this point in the history 
Upstream commit: https://webrtc.googlesource.com/src/+/0f741da200c064aea70a790d2fbf678e930bff39
    [M121] 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 c56052001dd747ae37c0cf7bab604791fe7912b0)

    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/+/335142
    Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
    Cr-Commit-Position: refs/branch-heads/6167@{#4}
    Cr-Branched-From: ece5cb83715dea85617114b6d4e981fdee2623ba-refs/heads/main@{#41315}
  • Loading branch information
@jan-ivar
jan-ivar committed Feb 11, 2024
1 parent 26cedef commit 6c05bbe
Show file tree
Hide file tree
Showing 7 changed files with 478 additions and 328 deletions.
3 changes: 3 additions & 0 deletions third_party/libwebrtc/README.moz-ff-commit
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27681,3 +27681,6 @@ ece5cb8371
# MOZ_LIBWEBRTC_SRC=/Users/jan-ivar/moz/elm/.moz-fast-forward/moz-libwebrtc MOZ_LIBWEBRTC_BRANCH=mozpatches bash dom/media/webrtc/third_party_build/fast-forward-libwebrtc.sh
# base of lastest vendoring
3df661f798
# MOZ_LIBWEBRTC_SRC=/Users/jan-ivar/moz/elm/.moz-fast-forward/moz-libwebrtc MOZ_LIBWEBRTC_BRANCH=mozpatches bash dom/media/webrtc/third_party_build/fast-forward-libwebrtc.sh
# base of lastest vendoring
0f741da200
2 changes: 2 additions & 0 deletions third_party/libwebrtc/README.mozilla
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18478,3 +18478,5 @@ libwebrtc updated from /Users/jan-ivar/moz/elm/.moz-fast-forward/moz-libwebrtc c
libwebrtc updated from /Users/jan-ivar/moz/elm/.moz-fast-forward/moz-libwebrtc commit mozpatches on 2024-02-10T23:04:03.473809.
# ./mach python dom/media/webrtc/third_party_build/vendor-libwebrtc.py --from-local /Users/jan-ivar/moz/elm/.moz-fast-forward/moz-libwebrtc --commit mozpatches libwebrtc
libwebrtc updated from /Users/jan-ivar/moz/elm/.moz-fast-forward/moz-libwebrtc commit mozpatches on 2024-02-11T17:10:57.292037.
# ./mach python dom/media/webrtc/third_party_build/vendor-libwebrtc.py --from-local /Users/jan-ivar/moz/elm/.moz-fast-forward/moz-libwebrtc --commit mozpatches libwebrtc
libwebrtc updated from /Users/jan-ivar/moz/elm/.moz-fast-forward/moz-libwebrtc commit mozpatches on 2024-02-11T17:12:06.916591.
164 changes: 89 additions & 75 deletions third_party/libwebrtc/pc/jsep_transport_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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_) {
Expand All @@ -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(
Expand Down Expand Up @@ -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");
RTC_DCHECK(description);

if (local) {
local_desc_ = description;
} else {
remote_desc_ = description;
}
// 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);

RTCError error;
error = ValidateAndMaybeUpdateBundleGroups(local, type, description);
error =
ValidateAndMaybeUpdateBundleGroups(local, type, local_desc, remote_desc);
if (!error.ok()) {
return error;
}
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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);

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;
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) {
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 in answer contains a MID='" +
content_name +
"' that was not in the offered group.");
"A BUNDLE group was added in the answer that did not "
"exist in the offer.");
}
}
}

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) {
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.");
}
}
}
}
Expand Down
34 changes: 27 additions & 7 deletions third_party/libwebrtc/pc/jsep_transport_controller.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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_;
Expand Down
Loading

0 comments on commit 6c05bbe

Please sign in to comment.