zqz/platform-external-webrtc
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

AddRemoteCandidate on the network thread

Browse Source 
SdpOfferAnswerHandler now hands over most of the work of adding a
remote candidate over to PeerConnection where the work will be
carried out asynchronously on the network thread (was
synchronous/blocking).

Once added, reporting (ReportRemoteIceCandidateAdded) continues on the
signaling thread as before. The difference is though that we don't
block the UseCandidate() operation which is a part of applying the
local and remote descriptions.

Besides now being asynchronous, there's one behavioural change:
Before starting the 'add' operation, the validity of the candidate
instance to be added, is checked. Previously if such an error occurred,
the error was silently ignored.

Bug: webrtc:9987
Change-Id: Ic1bfb8e27670fc81038b6ccec95ff36c65d12262
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/206063
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33230}
This commit is contained in:
Tomas Gunnarsson
2021-02-08 18:57:04 +01:00
committed by Commit Bot
parent ff0e01f689
commit 8cb9706288
8 changed files with 148 additions and 108 deletions
Download Patch File Download Diff File Expand all files Collapse all files

44
p2p/base/ice_transport_internal.cc
View File

@ -14,6 +14,50 @@
namespace cricket {
using webrtc::RTCError;
using webrtc::RTCErrorType;
RTCError VerifyCandidate(const Candidate& cand) {
// No address zero.
if (cand.address().IsNil() || cand.address().IsAnyIP()) {
return RTCError(RTCErrorType::INVALID_PARAMETER,
"candidate has address of zero");
}
// Disallow all ports below 1024, except for 80 and 443 on public addresses.
int port = cand.address().port();
if (cand.protocol() == cricket::TCP_PROTOCOL_NAME &&
(cand.tcptype() == cricket::TCPTYPE_ACTIVE_STR || port == 0)) {
// Expected for active-only candidates per
// http://tools.ietf.org/html/rfc6544#section-4.5 so no error.
// Libjingle clients emit port 0, in "active" mode.
return RTCError::OK();
}
if (port < 1024) {
if ((port != 80) && (port != 443)) {
return RTCError(RTCErrorType::INVALID_PARAMETER,
"candidate has port below 1024, but not 80 or 443");
}
if (cand.address().IsPrivateIP()) {
return RTCError(
RTCErrorType::INVALID_PARAMETER,
"candidate has port of 80 or 443 with private IP address");
}
}
return RTCError::OK();
}
RTCError VerifyCandidates(const Candidates& candidates) {
for (const Candidate& candidate : candidates) {
RTCError error = VerifyCandidate(candidate);
if (!error.ok())
return error;
}
return RTCError::OK();
}
IceConfig::IceConfig() = default;
IceConfig::IceConfig(int receiving_timeout_ms,

12
p2p/base/ice_transport_internal.h
View File

@ -18,6 +18,7 @@
#include "absl/types/optional.h"
#include "api/candidate.h"
#include "api/rtc_error.h"
#include "api/transport/enums.h"
#include "p2p/base/connection.h"
#include "p2p/base/packet_transport_internal.h"
@ -74,6 +75,17 @@ enum class NominationMode {
// The details are described in P2PTransportChannel.
};
// Utility method that checks if various required Candidate fields are filled in
// and contain valid values. If conditions are not met, an RTCError with the
// appropriated error number and description is returned. If the configuration
// is valid RTCError::OK() is returned.
webrtc::RTCError VerifyCandidate(const Candidate& cand);
// Runs through a list of cricket::Candidate instances and calls VerifyCandidate
// for each one, stopping on the first error encounted and returning that error
// value if so. On success returns RTCError::OK().
webrtc::RTCError VerifyCandidates(const Candidates& candidates);
// Information about ICE configuration.
// TODO(deadbeef): Use absl::optional to represent unset values, instead of
// -1.

60
pc/jsep_transport_controller.cc
View File

@ -34,53 +34,6 @@
using webrtc::SdpType;
namespace {
webrtc::RTCError VerifyCandidate(const cricket::Candidate& cand) {
// No address zero.
if (cand.address().IsNil() || cand.address().IsAnyIP()) {
return webrtc::RTCError(webrtc::RTCErrorType::INVALID_PARAMETER,
"candidate has address of zero");
}
// Disallow all ports below 1024, except for 80 and 443 on public addresses.
int port = cand.address().port();
if (cand.protocol() == cricket::TCP_PROTOCOL_NAME &&
(cand.tcptype() == cricket::TCPTYPE_ACTIVE_STR || port == 0)) {
// Expected for active-only candidates per
// http://tools.ietf.org/html/rfc6544#section-4.5 so no error.
// Libjingle clients emit port 0, in "active" mode.
return webrtc::RTCError::OK();
}
if (port < 1024) {
if ((port != 80) && (port != 443)) {
return webrtc::RTCError(
webrtc::RTCErrorType::INVALID_PARAMETER,
"candidate has port below 1024, but not 80 or 443");
}
if (cand.address().IsPrivateIP()) {
return webrtc::RTCError(
webrtc::RTCErrorType::INVALID_PARAMETER,
"candidate has port of 80 or 443 with private IP address");
}
}
return webrtc::RTCError::OK();
}
webrtc::RTCError VerifyCandidates(const cricket::Candidates& candidates) {
for (const cricket::Candidate& candidate : candidates) {
webrtc::RTCError error = VerifyCandidate(candidate);
if (!error.ok()) {
return error;
}
}
return webrtc::RTCError::OK();
}
} // namespace
namespace webrtc {
JsepTransportController::JsepTransportController(
@ -333,19 +286,8 @@ void JsepTransportController::MaybeStartGathering() {
RTCError JsepTransportController::AddRemoteCandidates(
const std::string& transport_name,
const cricket::Candidates& candidates) {
if (!network_thread_->IsCurrent()) {
return network_thread_->Invoke<RTCError>(RTC_FROM_HERE, [&] {
return AddRemoteCandidates(transport_name, candidates);
});
}
RTC_DCHECK_RUN_ON(network_thread_);
// Verify each candidate before passing down to the transport layer.
RTCError error = VerifyCandidates(candidates);
if (!error.ok()) {
return error;
}
RTC_DCHECK(VerifyCandidates(candidates).ok());
auto jsep_transport = GetJsepTransportByName(transport_name);
if (!jsep_transport) {
RTC_LOG(LS_WARNING) << "Not adding candidate because the JsepTransport "

58
pc/peer_connection.cc
View File

@ -2521,10 +2521,68 @@ void PeerConnection::NoteUsageEvent(UsageEvent event) {
usage_pattern_.NoteUsageEvent(event);
}
// Asynchronously adds remote candidates on the network thread.
void PeerConnection::AddRemoteCandidate(const std::string& mid,
const cricket::Candidate& candidate) {
RTC_DCHECK_RUN_ON(signaling_thread());
network_thread()->PostTask(ToQueuedTask(
network_thread_safety_, [this, mid = mid, candidate = candidate] {
RTC_DCHECK_RUN_ON(network_thread());
std::vector<cricket::Candidate> candidates = {candidate};
RTCError error =
transport_controller_->AddRemoteCandidates(mid, candidates);
if (error.ok()) {
signaling_thread()->PostTask(ToQueuedTask(
signaling_thread_safety_.flag(),
[this, candidate = std::move(candidate)] {
ReportRemoteIceCandidateAdded(candidate);
// Candidates successfully submitted for checking.
if (ice_connection_state() ==
PeerConnectionInterface::kIceConnectionNew ||
ice_connection_state() ==
PeerConnectionInterface::kIceConnectionDisconnected) {
// If state is New, then the session has just gotten its first
// remote ICE candidates, so go to Checking. If state is
// Disconnected, the session is re-using old candidates or
// receiving additional ones, so go to Checking. If state is
// Connected, stay Connected.
// TODO(bemasc): If state is Connected, and the new candidates
// are for a newly added transport, then the state actually
// _should_ move to checking. Add a way to distinguish that
// case.
SetIceConnectionState(
PeerConnectionInterface::kIceConnectionChecking);
}
// TODO(bemasc): If state is Completed, go back to Connected.
}));
} else {
RTC_LOG(LS_WARNING) << error.message();
}
}));
}
void PeerConnection::ReportUsagePattern() const {
usage_pattern_.ReportUsagePattern(observer_);
}
void PeerConnection::ReportRemoteIceCandidateAdded(
const cricket::Candidate& candidate) {
RTC_DCHECK_RUN_ON(signaling_thread());
NoteUsageEvent(UsageEvent::REMOTE_CANDIDATE_ADDED);
if (candidate.address().IsPrivateIP()) {
NoteUsageEvent(UsageEvent::REMOTE_PRIVATE_CANDIDATE_ADDED);
}
if (candidate.address().IsUnresolvedIP()) {
NoteUsageEvent(UsageEvent::REMOTE_MDNS_CANDIDATE_ADDED);
}
if (candidate.address().family() == AF_INET6) {
NoteUsageEvent(UsageEvent::REMOTE_IPV6_CANDIDATE_ADDED);
}
}
bool PeerConnection::SrtpRequired() const {
return (dtls_enabled_ ||
sdp_handler_->webrtc_session_desc_factory()->SdesPolicy() ==

8
pc/peer_connection.h
View File

@ -380,6 +380,10 @@ class PeerConnection : public PeerConnectionInternal,
void SetIceConnectionState(IceConnectionState new_state);
void NoteUsageEvent(UsageEvent event);
// Asynchronously adds a remote candidate on the network thread.
void AddRemoteCandidate(const std::string& mid,
const cricket::Candidate& candidate);
// Report the UMA metric SdpFormatReceived for the given remote description.
void ReportSdpFormatReceived(
const SessionDescriptionInterface& remote_description);
@ -502,10 +506,8 @@ class PeerConnection : public PeerConnectionInternal,
const cricket::CandidatePairChangeEvent& event)
RTC_RUN_ON(signaling_thread());
void OnNegotiationNeeded();
// Returns the specified SCTP DataChannel in sctp_data_channels_,
// or nullptr if not found.
SctpDataChannel* FindDataChannelBySid(int sid) const
@ -590,6 +592,8 @@ class PeerConnection : public PeerConnectionInternal,
void ReportUsagePattern() const RTC_RUN_ON(signaling_thread());
void ReportRemoteIceCandidateAdded(const cricket::Candidate& candidate);
// JsepTransportController::Observer override.
//
// Called by |transport_controller_| when processing transport information

15
pc/peer_connection_ice_unittest.cc
View File

@ -497,6 +497,21 @@ TEST_P(PeerConnectionIceTest, DuplicateIceCandidateIgnoredWhenAdded) {
EXPECT_EQ(1u, caller->GetIceCandidatesFromRemoteDescription().size());
}
TEST_P(PeerConnectionIceTest, ErrorOnInvalidRemoteIceCandidateAdded) {
auto caller = CreatePeerConnectionWithAudioVideo();
auto callee = CreatePeerConnectionWithAudioVideo();
ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
// Add a candidate to the remote description with a candidate that has an
// invalid address (port number == 2).
auto answer = callee->CreateAnswerAndSetAsLocal();
cricket::Candidate bad_candidate =
CreateLocalUdpCandidate(SocketAddress("2.2.2.2", 2));
RTC_LOG(LS_INFO) << "Bad candidate: " << bad_candidate.ToString();
AddCandidateToFirstTransport(&bad_candidate, answer.get());
// Now the call to SetRemoteDescription should fail.
EXPECT_FALSE(caller->SetRemoteDescription(std::move(answer)));
}
TEST_P(PeerConnectionIceTest,
CannotRemoveIceCandidatesWhenPeerConnectionClosed) {
const SocketAddress kCalleeAddress("1.1.1.1", 1111);

57
pc/sdp_offer_answer.cc
View File

@ -4468,40 +4468,21 @@ bool SdpOfferAnswerHandler::UseCandidatesInSessionDescription(
bool SdpOfferAnswerHandler::UseCandidate(
const IceCandidateInterface* candidate) {
RTC_DCHECK_RUN_ON(signaling_thread());
rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls;
RTCErrorOr<const cricket::ContentInfo*> result =
FindContentInfo(remote_description(), candidate);
if (!result.ok()) {
RTC_LOG(LS_ERROR) << "UseCandidate: Invalid candidate. "
<< result.error().message();
if (!result.ok())
return false;
}
std::vector<cricket::Candidate> candidates;
candidates.push_back(candidate->candidate());
// Invoking BaseSession method to handle remote candidates.
RTCError error = transport_controller()->AddRemoteCandidates(
result.value()->name, candidates);
if (error.ok()) {
ReportRemoteIceCandidateAdded(candidate->candidate());
// Candidates successfully submitted for checking.
if (pc_->ice_connection_state() ==
PeerConnectionInterface::kIceConnectionNew ||
pc_->ice_connection_state() ==
PeerConnectionInterface::kIceConnectionDisconnected) {
// If state is New, then the session has just gotten its first remote ICE
// candidates, so go to Checking.
// If state is Disconnected, the session is re-using old candidates or
// receiving additional ones, so go to Checking.
// If state is Connected, stay Connected.
// TODO(bemasc): If state is Connected, and the new candidates are for a
// newly added transport, then the state actually _should_ move to
// checking. Add a way to distinguish that case.
pc_->SetIceConnectionState(
PeerConnectionInterface::kIceConnectionChecking);
}
// TODO(bemasc): If state is Completed, go back to Connected.
} else {
RTC_LOG(LS_WARNING) << error.message();
}
const cricket::Candidate& c = candidate->candidate();
RTCError error = cricket::VerifyCandidate(c);
if (!error.ok())
return false;
pc_->AddRemoteCandidate(result.value()->name, c);
return true;
}
@ -4546,20 +4527,6 @@ bool SdpOfferAnswerHandler::ReadyToUseRemoteCandidate(
return has_transport;
}
void SdpOfferAnswerHandler::ReportRemoteIceCandidateAdded(
const cricket::Candidate& candidate) {
pc_->NoteUsageEvent(UsageEvent::REMOTE_CANDIDATE_ADDED);
if (candidate.address().IsPrivateIP()) {
pc_->NoteUsageEvent(UsageEvent::REMOTE_PRIVATE_CANDIDATE_ADDED);
}
if (candidate.address().IsUnresolvedIP()) {
pc_->NoteUsageEvent(UsageEvent::REMOTE_MDNS_CANDIDATE_ADDED);
}
if (candidate.address().family() == AF_INET6) {
pc_->NoteUsageEvent(UsageEvent::REMOTE_IPV6_CANDIDATE_ADDED);
}
}
RTCErrorOr<const cricket::ContentInfo*> SdpOfferAnswerHandler::FindContentInfo(
const SessionDescriptionInterface* description,
const IceCandidateInterface* candidate) {

2
pc/sdp_offer_answer.h
View File

@ -494,8 +494,6 @@ class SdpOfferAnswerHandler : public SdpStateProvider,
bool ReadyToUseRemoteCandidate(const IceCandidateInterface* candidate,
const SessionDescriptionInterface* remote_desc,
bool* valid);
void ReportRemoteIceCandidateAdded(const cricket::Candidate& candidate)
RTC_RUN_ON(signaling_thread());
RTCErrorOr<const cricket::ContentInfo*> FindContentInfo(
const SessionDescriptionInterface* description,