From fca128a058ec37d6538099a2b94714d2b15f1bc2 Mon Sep 17 00:00:00 2001
From: Sean DuBois <sean@siobud.com>
Date: Mon, 21 Feb 2022 15:36:48 -0500
Subject: [PATCH] Don't drop Binding Requests in Controlled Agent

A controlled Agent would discard incoming Binding Requests if it didn't
cause the pair to be selected. For UDP Candidate this would be
interpreted as packet loss. For TCP Candidates not responding with a
Binding Success could be interpreted as a failure.

Firefox's ICE Agent would disconnect TCP Candidates because of this
behavior.

Resolves to pion/webrtc#2125
Resolves to pion/webrtc#1356
See https://bugzilla.mozilla.org/show_bug.cgi?id=1756460
---
 candidatepair.go | 13 +++++++------
 selection.go     | 15 +++++++++------
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/candidatepair.go b/candidatepair.go
index 79cab1a9..0a53245c 100644
--- a/candidatepair.go
+++ b/candidatepair.go
@@ -18,12 +18,13 @@ func newCandidatePair(local, remote Candidate, controlling bool) *CandidatePair
 // CandidatePair is a combination of a
 // local and remote candidate
 type CandidatePair struct {
-	iceRoleControlling  bool
-	Remote              Candidate
-	Local               Candidate
-	bindingRequestCount uint16
-	state               CandidatePairState
-	nominated           bool
+	iceRoleControlling       bool
+	Remote                   Candidate
+	Local                    Candidate
+	bindingRequestCount      uint16
+	state                    CandidatePairState
+	nominated                bool
+	nominateOnBindingSuccess bool
 }
 
 func (p *CandidatePair) String() string {
diff --git a/selection.go b/selection.go
index 72484148..df43f03d 100644
--- a/selection.go
+++ b/selection.go
@@ -229,13 +229,17 @@ func (s *controlledSelector) HandleSuccessResponse(m *stun.Message, local, remot
 
 	p.state = CandidatePairStateSucceeded
 	s.log.Tracef("Found valid candidate pair: %s", p)
+	if p.nominateOnBindingSuccess {
+		if selectedPair := s.agent.getSelectedPair(); selectedPair == nil {
+			s.agent.setSelectedPair(p)
+		}
+	}
 }
 
 func (s *controlledSelector) HandleBindingRequest(m *stun.Message, local, remote Candidate) {
 	useCandidate := m.Contains(stun.AttrUseCandidate)
 
 	p := s.agent.findPair(local, remote)
-
 	if p == nil {
 		p = s.agent.addPair(local, remote)
 	}
@@ -251,7 +255,6 @@ func (s *controlledSelector) HandleBindingRequest(m *stun.Message, local, remote
 			if selectedPair := s.agent.getSelectedPair(); selectedPair == nil {
 				s.agent.setSelectedPair(p)
 			}
-			s.agent.sendBindingSuccess(m, local, remote)
 		} else {
 			// If the received Binding request triggered a new check to be
 			// enqueued in the triggered-check queue (Section 7.3.1.4), once the
@@ -261,12 +264,12 @@ func (s *controlledSelector) HandleBindingRequest(m *stun.Message, local, remote
 			// MUST remove the candidate pair from the valid list, set the
 			// candidate pair state to Failed, and set the checklist state to
 			// Failed.
-			s.PingCandidate(local, remote)
+			p.nominateOnBindingSuccess = true
 		}
-	} else {
-		s.agent.sendBindingSuccess(m, local, remote)
-		s.PingCandidate(local, remote)
 	}
+
+	s.agent.sendBindingSuccess(m, local, remote)
+	s.PingCandidate(local, remote)
 }
 
 type liteSelector struct {