Don't use RoomMember as a calls a key on GroupCall (#2993)

spec/test-utils/webrtc.ts

@@ -513,9 +513,6 @@ export class MockMatrixCall extends TypedEventEmitter<CallEvent, CallEventHandle
 
     public sendMetadataUpdate = jest.fn<void, []>();
 
-    public on = jest.fn();
-    public removeListener = jest.fn();
-
     public getOpponentMember(): Partial<RoomMember> {
         return this.opponentMember;
     }

spec/unit/webrtc/groupCall.spec.ts

@@ -142,6 +142,15 @@ describe("Group Call", function () {
             } as unknown as RoomMember;
         });
 
+        it.each(Object.values(GroupCallState).filter((v) => v !== GroupCallState.LocalCallFeedUninitialized))(
+            "throws when initializing local call feed in %s state",
+            async (state: GroupCallState) => {
+                // @ts-ignore
+                groupCall.state = state;
+                await expect(groupCall.initLocalCallFeed()).rejects.toThrowError();
+            },
+        );
+
         it("does not initialize local call feed, if it already is", async () => {
             await groupCall.initLocalCallFeed();
             jest.spyOn(groupCall, "initLocalCallFeed");
@@ -308,6 +317,17 @@ describe("Group Call", function () {
             }
         });
 
+        describe("hasLocalParticipant()", () => {
+            it("should return false, if we don't have a local participant", () => {
+                expect(groupCall.hasLocalParticipant()).toBeFalsy();
+            });
+
+            it("should return true, if we do have local participant", async () => {
+                await groupCall.enter();
+                expect(groupCall.hasLocalParticipant()).toBeTruthy();
+            });
+        });
+
         describe("call feeds changing", () => {
             let call: MockMatrixCall;
             const currentFeed = new MockCallFeed(FAKE_USER_ID_1, FAKE_DEVICE_ID_1, new MockMediaStream("current"));
@@ -475,7 +495,7 @@ describe("Group Call", function () {
                 const mockCall = new MockMatrixCall(FAKE_ROOM_ID, groupCall.groupCallId);
                 // @ts-ignore
                 groupCall.calls.set(
-                    mockCall.getOpponentMember() as RoomMember,
+                    mockCall.getOpponentMember().userId!,
                     new Map([[mockCall.getOpponentDeviceId()!, mockCall.typed()]]),
                 );
 
@@ -501,7 +521,7 @@ describe("Group Call", function () {
                 const mockCall = new MockMatrixCall(FAKE_ROOM_ID, groupCall.groupCallId);
                 // @ts-ignore
                 groupCall.calls.set(
-                    mockCall.getOpponentMember() as RoomMember,
+                    mockCall.getOpponentMember().userId!,
                     new Map([[mockCall.getOpponentDeviceId()!, mockCall.typed()]]),
                 );
 
@@ -663,9 +683,7 @@ describe("Group Call", function () {
                 expect(client1.sendToDevice).toHaveBeenCalled();
 
                 // @ts-ignore
-                const oldCall = groupCall1.calls
+                const oldCall = groupCall1.calls.get(client2.userId)!.get(client2.deviceId)!;
-                    .get(groupCall1.room.getMember(client2.userId)!)!
-                    .get(client2.deviceId)!;
                 oldCall.emit(CallEvent.Hangup, oldCall!);
 
                 client1.sendToDevice.mockClear();
@@ -685,9 +703,7 @@ describe("Group Call", function () {
                 let newCall: MatrixCall | undefined;
                 while (
                     // @ts-ignore
-                    (newCall = groupCall1.calls
+                    (newCall = groupCall1.calls.get(client2.userId)?.get(client2.deviceId)) === undefined ||
-                        .get(groupCall1.room.getMember(client2.userId)!)
-                        ?.get(client2.deviceId)) === undefined ||
                     newCall.peerConn === undefined ||
                     newCall.callId == oldCall.callId
                 ) {
@@ -730,7 +746,7 @@ describe("Group Call", function () {
                 groupCall1.setLocalVideoMuted(false);
 
                 // @ts-ignore
-                const call = groupCall1.calls.get(groupCall1.room.getMember(client2.userId)!)!.get(client2.deviceId)!;
+                const call = groupCall1.calls.get(client2.userId)!.get(client2.deviceId)!;
                 call.isMicrophoneMuted = jest.fn().mockReturnValue(true);
                 call.setMicrophoneMuted = jest.fn();
                 call.isLocalVideoMuted = jest.fn().mockReturnValue(true);
@@ -839,7 +855,7 @@ describe("Group Call", function () {
                 await sleep(10);
 
                 // @ts-ignore
-                const call = groupCall.calls.get(groupCall.room.getMember(FAKE_USER_ID_2)!)!.get(FAKE_DEVICE_ID_2)!;
+                const call = groupCall.calls.get(FAKE_USER_ID_2)!.get(FAKE_DEVICE_ID_2)!;
                 call.getOpponentMember = () => ({ userId: call.invitee } as RoomMember);
                 // @ts-ignore Mock
                 call.pushRemoteFeed(
@@ -866,7 +882,7 @@ describe("Group Call", function () {
                 await sleep(10);
 
                 // @ts-ignore
-                const call = groupCall.calls.get(groupCall.room.getMember(FAKE_USER_ID_2)!)!.get(FAKE_DEVICE_ID_2)!;
+                const call = groupCall.calls.get(FAKE_USER_ID_2).get(FAKE_DEVICE_ID_2)!;
                 call.getOpponentMember = () => ({ userId: call.invitee } as RoomMember);
                 // @ts-ignore Mock
                 call.pushRemoteFeed(
@@ -943,9 +959,7 @@ describe("Group Call", function () {
             expect(mockCall.reject).not.toHaveBeenCalled();
             expect(mockCall.answerWithCallFeeds).toHaveBeenCalled();
             // @ts-ignore
-            expect(groupCall.calls).toEqual(
+            expect(groupCall.calls).toEqual(new Map([[FAKE_USER_ID_1, new Map([[FAKE_DEVICE_ID_1, mockCall]])]]));
-                new Map([[groupCall.room.getMember(FAKE_USER_ID_1)!, new Map([[FAKE_DEVICE_ID_1, mockCall]])]]),
-            );
         });
 
         it("replaces calls if it already has one with the same user", async () => {
@@ -960,9 +974,7 @@ describe("Group Call", function () {
             expect(oldMockCall.hangup).toHaveBeenCalled();
             expect(newMockCall.answerWithCallFeeds).toHaveBeenCalled();
             // @ts-ignore
-            expect(groupCall.calls).toEqual(
+            expect(groupCall.calls).toEqual(new Map([[FAKE_USER_ID_1, new Map([[FAKE_DEVICE_ID_1, newMockCall]])]]));
-                new Map([[groupCall.room.getMember(FAKE_USER_ID_1)!, new Map([[FAKE_DEVICE_ID_1, newMockCall]])]]),
-            );
         });
 
         it("starts to process incoming calls when we've entered", async () => {
@@ -975,6 +987,83 @@ describe("Group Call", function () {
 
             expect(call.answerWithCallFeeds).toHaveBeenCalled();
         });
+
+        describe("handles call being replaced", () => {
+            let callChangedListener: jest.Mock;
+            let oldMockCall: MockMatrixCall;
+            let newMockCall: MockMatrixCall;
+            let newCallsMap: Map<string, Map<string, MatrixCall>>;
+
+            beforeEach(() => {
+                callChangedListener = jest.fn();
+                groupCall.addListener(GroupCallEvent.CallsChanged, callChangedListener);
+
+                oldMockCall = new MockMatrixCall(room.roomId, groupCall.groupCallId);
+                newMockCall = new MockMatrixCall(room.roomId, groupCall.groupCallId);
+                newCallsMap = new Map([[FAKE_USER_ID_1, new Map([[FAKE_DEVICE_ID_1, newMockCall.typed()]])]]);
+
+                newMockCall.opponentMember = oldMockCall.opponentMember; // Ensure referential equality
+                newMockCall.callId = "not " + oldMockCall.callId;
+                mockClient.emit(CallEventHandlerEvent.Incoming, oldMockCall.typed());
+            });
+
+            it("handles regular case", () => {
+                oldMockCall.emit(CallEvent.Replaced, newMockCall.typed());
+
+                expect(oldMockCall.hangup).toHaveBeenCalled();
+                expect(callChangedListener).toHaveBeenCalledWith(newCallsMap);
+                // @ts-ignore
+                expect(groupCall.calls).toEqual(newCallsMap);
+            });
+
+            it("handles case where call is missing from the calls map", () => {
+                // @ts-ignore
+                groupCall.calls = new Map();
+                oldMockCall.emit(CallEvent.Replaced, newMockCall.typed());
+
+                expect(oldMockCall.hangup).toHaveBeenCalled();
+                expect(callChangedListener).toHaveBeenCalledWith(newCallsMap);
+                // @ts-ignore
+                expect(groupCall.calls).toEqual(newCallsMap);
+            });
+        });
+
+        describe("handles call being hangup", () => {
+            let callChangedListener: jest.Mock;
+            let mockCall: MockMatrixCall;
+
+            beforeEach(() => {
+                callChangedListener = jest.fn();
+                groupCall.addListener(GroupCallEvent.CallsChanged, callChangedListener);
+                mockCall = new MockMatrixCall(room.roomId, groupCall.groupCallId);
+            });
+
+            it("doesn't throw when calls map is empty", () => {
+                // @ts-ignore
+                expect(() => groupCall.onCallHangup(mockCall)).not.toThrow();
+            });
+
+            it("clears map completely when we're the last users device left", () => {
+                mockClient.emit(CallEventHandlerEvent.Incoming, mockCall.typed());
+                mockCall.emit(CallEvent.Hangup, mockCall.typed());
+                // @ts-ignore
+                expect(groupCall.calls).toEqual(new Map());
+            });
+
+            it("doesn't remove another call of the same user", () => {
+                const anotherCallOfTheSameUser = new MockMatrixCall(room.roomId, groupCall.groupCallId);
+                anotherCallOfTheSameUser.callId = "another call id";
+                anotherCallOfTheSameUser.getOpponentDeviceId = () => FAKE_DEVICE_ID_2;
+                mockClient.emit(CallEventHandlerEvent.Incoming, anotherCallOfTheSameUser.typed());
+
+                mockClient.emit(CallEventHandlerEvent.Incoming, mockCall.typed());
+                mockCall.emit(CallEvent.Hangup, mockCall.typed());
+                // @ts-ignore
+                expect(groupCall.calls).toEqual(
+                    new Map([[FAKE_USER_ID_1, new Map([[FAKE_DEVICE_ID_2, anotherCallOfTheSameUser.typed()]])]]),
+                );
+            });
+        });
     });
 
     describe("screensharing", () => {
@@ -1039,7 +1128,7 @@ describe("Group Call", function () {
             await sleep(10);
 
             // @ts-ignore
-            const call = groupCall.calls.get(groupCall.room.getMember(FAKE_USER_ID_2)!)!.get(FAKE_DEVICE_ID_2)!;
+            const call = groupCall.calls.get(FAKE_USER_ID_2)!.get(FAKE_DEVICE_ID_2)!;
             call.getOpponentMember = () => ({ userId: call.invitee } as RoomMember);
             call.onNegotiateReceived({
                 getContent: () => ({

src/webrtc/groupCall.ts

@@ -55,7 +55,7 @@ export enum GroupCallEvent {
 export type GroupCallEventHandlerMap = {
     [GroupCallEvent.GroupCallStateChanged]: (newState: GroupCallState, oldState: GroupCallState) => void;
     [GroupCallEvent.ActiveSpeakerChanged]: (activeSpeaker: CallFeed | undefined) => void;
-    [GroupCallEvent.CallsChanged]: (calls: Map<RoomMember, Map<string, MatrixCall>>) => void;
+    [GroupCallEvent.CallsChanged]: (calls: Map<string, Map<string, MatrixCall>>) => void;
     [GroupCallEvent.UserMediaFeedsChanged]: (feeds: CallFeed[]) => void;
     [GroupCallEvent.ScreenshareFeedsChanged]: (feeds: CallFeed[]) => void;
     [GroupCallEvent.LocalScreenshareStateChanged]: (
@@ -197,11 +197,11 @@ export class GroupCall extends TypedEventEmitter<
     public readonly screenshareFeeds: CallFeed[] = [];
     public groupCallId: string;
 
-    private readonly calls = new Map<RoomMember, Map<string, MatrixCall>>(); // RoomMember -> device ID -> MatrixCall
+    private readonly calls = new Map<string, Map<string, MatrixCall>>(); // user_id -> device_id -> MatrixCall
-    private callHandlers = new Map<string, Map<string, ICallHandlers>>(); // User ID -> device ID -> handlers
+    private callHandlers = new Map<string, Map<string, ICallHandlers>>(); // user_id -> device_id -> ICallHandlers
     private activeSpeakerLoopInterval?: ReturnType<typeof setTimeout>;
     private retryCallLoopInterval?: ReturnType<typeof setTimeout>;
-    private retryCallCounts: Map<RoomMember, Map<string, number>> = new Map();
+    private retryCallCounts: Map<string, Map<string, number>> = new Map(); // user_id -> device_id -> count
     private reEmitter: ReEmitter;
     private transmitTimer: ReturnType<typeof setTimeout> | null = null;
     private participantsExpirationTimer: ReturnType<typeof setTimeout> | null = null;
@@ -728,18 +728,18 @@ export class GroupCall extends TypedEventEmitter<
             return;
         }
 
-        const opponent = newCall.getOpponentMember();
+        const opponentUserId = newCall.getOpponentMember()?.userId;
-        if (opponent === undefined) {
+        if (opponentUserId === undefined) {
             logger.warn("Incoming call with no member. Ignoring.");
             return;
         }
 
-        const deviceMap = this.calls.get(opponent) ?? new Map<string, MatrixCall>();
+        const deviceMap = this.calls.get(opponentUserId) ?? new Map<string, MatrixCall>();
         const prevCall = deviceMap.get(newCall.getOpponentDeviceId()!);
 
         if (prevCall?.callId === newCall.callId) return;
 
-        logger.log(`GroupCall: incoming call from ${opponent.userId} with ID ${newCall.callId}`);
+        logger.log(`GroupCall: incoming call from ${opponentUserId} with ID ${newCall.callId}`);
 
         if (prevCall) this.disposeCall(prevCall, CallErrorCode.Replaced);
 
@@ -747,7 +747,7 @@ export class GroupCall extends TypedEventEmitter<
         newCall.answerWithCallFeeds(this.getLocalFeeds().map((feed) => feed.clone()));
 
         deviceMap.set(newCall.getOpponentDeviceId()!, newCall);
-        this.calls.set(opponent, deviceMap);
+        this.calls.set(opponentUserId, deviceMap);
         this.emit(GroupCallEvent.CallsChanged, this.calls);
     };
 
@@ -775,38 +775,38 @@ export class GroupCall extends TypedEventEmitter<
     private placeOutgoingCalls(): void {
         let callsChanged = false;
 
-        for (const [member, participantMap] of this.participants) {
+        for (const [{ userId }, participantMap] of this.participants) {
-            const callMap = this.calls.get(member) ?? new Map<string, MatrixCall>();
+            const callMap = this.calls.get(userId) ?? new Map<string, MatrixCall>();
 
             for (const [deviceId, participant] of participantMap) {
                 const prevCall = callMap.get(deviceId);
 
                 if (
                     prevCall?.getOpponentSessionId() !== participant.sessionId &&
-                    this.wantsOutgoingCall(member.userId, deviceId)
+                    this.wantsOutgoingCall(userId, deviceId)
                 ) {
                     callsChanged = true;
 
                     if (prevCall !== undefined) {
-                        logger.debug(`Replacing call ${prevCall.callId} to ${member.userId} ${deviceId}`);
+                        logger.debug(`Replacing call ${prevCall.callId} to ${userId} ${deviceId}`);
                         this.disposeCall(prevCall, CallErrorCode.NewSession);
                     }
 
                     const newCall = createNewMatrixCall(this.client, this.room.roomId, {
-                        invitee: member.userId,
+                        invitee: userId,
                         opponentDeviceId: deviceId,
                         opponentSessionId: participant.sessionId,
                         groupCallId: this.groupCallId,
                     });
 
                     if (newCall === null) {
-                        logger.error(`Failed to create call with ${member.userId} ${deviceId}`);
+                        logger.error(`Failed to create call with ${userId} ${deviceId}`);
                         callMap.delete(deviceId);
                     } else {
                         this.initCall(newCall);
                         callMap.set(deviceId, newCall);
 
-                        logger.debug(`Placing call to ${member.userId} ${deviceId} (session ${participant.sessionId})`);
+                        logger.debug(`Placing call to ${userId} ${deviceId} (session ${participant.sessionId})`);
 
                         newCall
                             .placeCallWithCallFeeds(
@@ -819,7 +819,7 @@ export class GroupCall extends TypedEventEmitter<
                                 }
                             })
                             .catch((e) => {
-                                logger.warn(`Failed to place call to ${member.userId}`, e);
+                                logger.warn(`Failed to place call to ${userId}`, e);
 
                                 if (e instanceof CallError && e.code === GroupCallErrorCode.UnknownDevice) {
                                     this.emit(GroupCallEvent.Error, e);
@@ -828,7 +828,7 @@ export class GroupCall extends TypedEventEmitter<
                                         GroupCallEvent.Error,
                                         new GroupCallError(
                                             GroupCallErrorCode.PlaceCallFailed,
-                                            `Failed to place call to ${member.userId}`,
+                                            `Failed to place call to ${userId}`,
                                         ),
                                     );
                                 }
@@ -841,9 +841,9 @@ export class GroupCall extends TypedEventEmitter<
             }
 
             if (callMap.size > 0) {
-                this.calls.set(member, callMap);
+                this.calls.set(userId, callMap);
             } else {
-                this.calls.delete(member);
+                this.calls.delete(userId);
             }
         }
 
@@ -865,9 +865,9 @@ export class GroupCall extends TypedEventEmitter<
     private onRetryCallLoop = (): void => {
         let needsRetry = false;
 
-        for (const [member, participantMap] of this.participants) {
+        for (const [{ userId }, participantMap] of this.participants) {
-            const callMap = this.calls.get(member);
+            const callMap = this.calls.get(userId);
-            let retriesMap = this.retryCallCounts.get(member);
+            let retriesMap = this.retryCallCounts.get(userId);
 
             for (const [deviceId, participant] of participantMap) {
                 const call = callMap?.get(deviceId);
@@ -875,12 +875,12 @@ export class GroupCall extends TypedEventEmitter<
 
                 if (
                     call?.getOpponentSessionId() !== participant.sessionId &&
-                    this.wantsOutgoingCall(member.userId, deviceId) &&
+                    this.wantsOutgoingCall(userId, deviceId) &&
                     retries < 3
                 ) {
                     if (retriesMap === undefined) {
                         retriesMap = new Map();
-                        this.retryCallCounts.set(member, retriesMap);
+                        this.retryCallCounts.set(userId, retriesMap);
                     }
                     retriesMap.set(deviceId, retries + 1);
                     needsRetry = true;
@@ -1020,36 +1020,36 @@ export class GroupCall extends TypedEventEmitter<
             call.setLocalVideoMuted(videoMuted);
         }
 
-        if (state === CallState.Connected) {
+        const opponentUserId = call.getOpponentMember()?.userId;
-            const opponent = call.getOpponentMember()!;
+        if (state === CallState.Connected && opponentUserId) {
-            const retriesMap = this.retryCallCounts.get(opponent);
+            const retriesMap = this.retryCallCounts.get(opponentUserId);
             retriesMap?.delete(call.getOpponentDeviceId()!);
-            if (retriesMap?.size === 0) this.retryCallCounts.delete(opponent);
+            if (retriesMap?.size === 0) this.retryCallCounts.delete(opponentUserId);
         }
     };
 
     private onCallHangup = (call: MatrixCall): void => {
         if (call.hangupReason === CallErrorCode.Replaced) return;
 
-        const opponent = call.getOpponentMember() ?? this.room.getMember(call.invitee!)!;
+        const opponentUserId = call.getOpponentMember()?.userId ?? this.room.getMember(call.invitee!)!.userId;
-        const deviceMap = this.calls.get(opponent);
+        const deviceMap = this.calls.get(opponentUserId);
 
         // Sanity check that this call is in fact in the map
         if (deviceMap?.get(call.getOpponentDeviceId()!) === call) {
             this.disposeCall(call, call.hangupReason as CallErrorCode);
             deviceMap.delete(call.getOpponentDeviceId()!);
-            if (deviceMap.size === 0) this.calls.delete(opponent);
+            if (deviceMap.size === 0) this.calls.delete(opponentUserId);
             this.emit(GroupCallEvent.CallsChanged, this.calls);
         }
     };
 
     private onCallReplaced = (prevCall: MatrixCall, newCall: MatrixCall): void => {
-        const opponent = prevCall.getOpponentMember()!;
+        const opponentUserId = prevCall.getOpponentMember()!.userId;
 
-        let deviceMap = this.calls.get(opponent);
+        let deviceMap = this.calls.get(opponentUserId);
         if (deviceMap === undefined) {
             deviceMap = new Map();
-            this.calls.set(opponent, deviceMap);
+            this.calls.set(opponentUserId, deviceMap);
         }
 
         this.disposeCall(prevCall, CallErrorCode.Replaced);