fix: weak glare protection on webrtc renegotiations

.changeset/six-adults-lie.md

@@ -0,0 +1,6 @@
+---
+'@rocket.chat/media-signaling': minor
+'@rocket.chat/media-calls': minor
+---
+
+Fixes an error where Voice Calls could fail to negotiate webrtc params if both clients requested a renegotiation at the same time

ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts

@@ -177,13 +177,69 @@ export class UserActorSignalProcessor {
 	}
 
 	private async processNegotiationNeeded(oldNegotiationId: string): Promise<void> {
+		// Unsigned clients may not request negotiations
+		if (!this.signed) {
+			return;
+		}
+
 		logger.debug({ msg: 'UserActorSignalProcessor.processNegotiationNeeded', oldNegotiationId });
 		const negotiation = await MediaCallNegotiations.findLatestByCallId(this.callId);
-		// If the negotiation that triggered a request for renegotiation is not the latest negotiation, then a new one must already be happening and we can ignore this request.
-		if (negotiation?._id !== oldNegotiationId) {
+
+		// If the call doesn't even have an initial negotiation yet, the clients shouldn't be requesting new ones.
+		if (!negotiation) {
 			return;
 		}
 
+		// If the latest negotiation has an answer, we can accept any request
+		if (negotiation.answer) {
+			return this.startNewNegotiation();
+		}
+
+		const comingFromLatest = oldNegotiationId === negotiation._id;
+		const isRequestImpolite = this.role === 'caller';
+		const isLatestImpolite = negotiation.offerer === 'caller';
+
+		// If the request came from a client who was not yet aware of a newer renegotiation
+		if (!comingFromLatest) {
+			// If the client is polite, we can ignore their request in favor of the existing renegotiation
+			if (!isRequestImpolite) {
+				logger.debug({ msg: 'Ignoring outdated polite renegotiation request' });
+				return;
+			}
+
+			// If the latest negotiation is impolite and the impolite client is not aware of it yet, this must be a duplicate request
+			if (isLatestImpolite) {
+				// If we already received an offer in this situation then something is very wrong (some proxy interfering with signals, perhaps?)
+				if (negotiation.offer) {
+					logger.error({ msg: 'Invalid renegotiation request', requestedBy: this.role, isLatestImpolite });
+					return;
+				}
+
+				// Resend the offer request to the impolite client
+				return this.requestWebRTCOffer({ negotiationId: negotiation._id });
+			}
+
+			// The state of polite negotiations is irrelevant for impolite requests, so we can start a new negotiation here.
+			return this.startNewNegotiation();
+		}
+
+		// The client is up-to-date and requested a renegotiation before the last one was complete
+		// If the request came from the same side as the last negotiation, the client was in no position to request it
+		if (this.role === negotiation.offerer) {
+			logger.error({ msg: 'Invalid state for renegotiation request', requestedBy: this.role, isLatestImpolite });
+			return;
+		}
+
+		// If the request is from the impolite client, it takes priority over the existing polite negotiation
+		if (isRequestImpolite) {
+			return this.startNewNegotiation();
+		}
+
+		// It's a polite negotiation requested while an impolite one was not yet complete
+		logger.error({ msg: 'Invalid state for renegotiation request', requestedBy: this.role, isLatestImpolite });
+	}
+
+	private async startNewNegotiation(): Promise<void> {
 		const negotiationId = await mediaCallDirector.startNewNegotiation(this.call, this.role);
 		if (negotiationId) {
 			await this.requestWebRTCOffer({ negotiationId });

packages/media-signaling/src/definition/client.ts

@@ -4,12 +4,8 @@ export type ClientState =
 	| 'accepting' // The client tried to accept the call and is wating for confirmation from the server
 	| 'accepted' // The call was accepted, but the client doesn't have a webrtc offer yet
 	| 'busy-elsewhere' // The call is happening in a different session/client
-	| 'has-offer' // The call was accepted and the client has a webrtc offer
-	| 'has-answer' // The call was accepted and the client has a webrtc offer and answer
 	| 'active' // The webrtc call was established
 	| 'renegotiating' // the webrtc call was established but the client is starting a new negotiation
-	| 'has-new-offer' // The webrtc call was established but the client has an offer for a new negotiation
-	| 'has-new-answer' // The webrtc call was established but the client is finishing a renegotiation
 	| 'hangup'; // The call is over, or happening in some other client
 
 export type ClientContractState =

packages/media-signaling/src/definition/services/IServiceProcessor.ts

@@ -14,11 +14,13 @@ export type ServiceStateValue<ServiceStateMap extends DefaultServiceStateMap, K
 export type ServiceProcessorEvents<ServiceStateMap extends DefaultServiceStateMap> = {
 	internalStateChange: keyof ServiceStateMap;
 	internalError: { critical: boolean; error: string | Error; errorDetails?: string };
-	negotiationNeeded: void;
 };
 
-export interface IServiceProcessor<ServiceStateMap extends DefaultServiceStateMap = DefaultServiceStateMap> {
-	emitter: Emitter<ServiceProcessorEvents<ServiceStateMap>>;
+export interface IServiceProcessor<
+	ServiceStateMap extends DefaultServiceStateMap = DefaultServiceStateMap,
+	ServiceUniqueEvents = Record<never, never>,
+> {
+	emitter: Emitter<ServiceProcessorEvents<ServiceStateMap> & ServiceUniqueEvents>;
 
 	getInternalState<K extends keyof ServiceStateMap>(stateName: K): ServiceStateValue<ServiceStateMap, K>;
 }

packages/media-signaling/src/definition/services/index.ts

@@ -1,3 +1,4 @@
 export * from './webrtc/IWebRTCProcessor';
 export * from './IServiceProcessorFactoryList';
 export * from './MediaStreamFactory';
+export * from './negotiation';

packages/media-signaling/src/definition/services/negotiation.ts

@@ -0,0 +1,30 @@
+import type { IClientMediaCall } from '../call';
+import type { IMediaSignalLogger } from '../logger';
+
+export type NegotiationManagerEvents = {
+	'error': { errorCode: string; negotiationId: string };
+	'local-sdp': { negotiationId: string; sdp: RTCSessionDescriptionInit };
+	'negotiation-needed': { oldNegotiationId: string };
+};
+
+export type NegotiationManagerConfig = {
+	logger?: IMediaSignalLogger | null;
+};
+
+export type NegotiationEvents = {
+	'error': { errorCode: string };
+	'ended': void;
+	'local-sdp': { sdp: RTCSessionDescriptionInit };
+};
+
+export type NegotiationData = {
+	negotiationId: string;
+	sequence: number;
+	isPolite: boolean;
+
+	remoteOffer: RTCSessionDescriptionInit | null;
+};
+
+export interface INegotiationCompatibleMediaCall extends IClientMediaCall {
+	hasInputTrack(): boolean;
+}

packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts

@@ -12,9 +12,13 @@ export type WebRTCInternalStateMap = {
 	iceUntrickler: 'waiting' | 'not-waiting' | 'timeout';
 };
 
-export type WebRTCProcessorEvents = ServiceProcessorEvents<WebRTCInternalStateMap>;
+export type WebRTCUniqueEvents = {
+	negotiationNeeded: void;
+};
+
+export type WebRTCProcessorEvents = ServiceProcessorEvents<WebRTCInternalStateMap> & WebRTCUniqueEvents;
 
-export interface IWebRTCProcessor extends IServiceProcessor<WebRTCInternalStateMap> {
+export interface IWebRTCProcessor extends IServiceProcessor<WebRTCInternalStateMap, WebRTCUniqueEvents> {
 	emitter: Emitter<WebRTCProcessorEvents>;
 
 	muted: boolean;
@@ -24,10 +28,13 @@ export interface IWebRTCProcessor extends IServiceProcessor<WebRTCInternalStateM
 	stop(): void;
 
 	setInputTrack(newInputTrack: MediaStreamTrack | null): Promise<void>;
-	startNewNegotiation(): void;
-	createOffer(params: { iceRestart?: boolean }): Promise<{ sdp: RTCSessionDescriptionInit }>;
-	createAnswer(params: { sdp: RTCSessionDescriptionInit }): Promise<{ sdp: RTCSessionDescriptionInit }>;
-	setRemoteAnswer(params: { sdp: RTCSessionDescriptionInit }): Promise<void>;
+	createOffer(params: { iceRestart?: boolean }): Promise<RTCSessionDescriptionInit>;
+	createAnswer(): Promise<RTCSessionDescriptionInit>;
+
+	setLocalDescription(sdp: RTCSessionDescriptionInit): Promise<void>;
+	setRemoteDescription(sdp: RTCSessionDescriptionInit): Promise<void>;
+	waitForIceGathering(): Promise<void>;
+	getLocalDescription(): RTCSessionDescriptionInit | null;
 
 	getRemoteMediaStream(): MediaStream;
 

packages/media-signaling/src/definition/signals/client/local-state.ts

@@ -41,20 +41,7 @@ export const clientMediaSignalLocalStateSchema: JSONSchemaType<ClientMediaSignal
 		},
 		clientState: {
 			type: 'string',
-			enum: [
-				'none',
-				'pending',
-				'accepting',
-				'accepted',
-				'busy-elsewhere',
-				'has-offer',
-				'has-answer',
-				'active',
-				'renegotiating',
-				'has-new-offer',
-				'has-new-answer',
-				'hangup',
-			],
+			enum: ['none', 'pending', 'accepting', 'accepted', 'busy-elsewhere', 'active', 'renegotiating', 'hangup'],
 			nullable: false,
 		},
 		serviceStates: {