From b5d8e63c6dbef48e198d7bd7e8907b1c96dbd638 Mon Sep 17 00:00:00 2001 From: Robin Date: Tue, 23 Sep 2025 11:45:48 -0400 Subject: [PATCH] Avoid creating multiple call objects for the same widget (#30839) * Extract some setup code out of the call tests * Don't force all rooms to be rechecked for calls when starting a call * Remove misleading unused group call callbacks The GroupCallEventHandler hasn't been relevant to our Element Call group calls for some time; instead we look at the state of the MatrixRTCSessionManager and WidgetStore to determine whether a call has been started. * Avoid creating multiple call objects for the same widget * fix test --------- Co-authored-by: Will Hunt --- src/models/Call.ts | 5 +- src/stores/CallStore.ts | 24 ++--- src/stores/WidgetStore.ts | 2 + test/test-utils/call.ts | 110 ++++++++++++++++++++++- test/unit-tests/models/Call-test.ts | 90 +------------------ test/unit-tests/stores/CallStore-test.ts | 38 ++++++++ 6 files changed, 162 insertions(+), 107 deletions(-) create mode 100644 test/unit-tests/stores/CallStore-test.ts diff --git a/src/models/Call.ts b/src/models/Call.ts index 81970da6b1..1eabd09698 100644 --- a/src/models/Call.ts +++ b/src/models/Call.ts @@ -38,7 +38,6 @@ import { WidgetMessagingStore, WidgetMessagingStoreEvent } from "../stores/widge import ActiveWidgetStore, { ActiveWidgetStoreEvent } from "../stores/ActiveWidgetStore"; import { getCurrentLanguage } from "../languageHandler"; import { Anonymity, PosthogAnalytics } from "../PosthogAnalytics"; -import { UPDATE_EVENT } from "../stores/AsyncStore"; import { isVideoRoom } from "../utils/video-rooms"; import { FontWatcher } from "../settings/watchers/FontWatcher"; import { type JitsiCallMemberContent, JitsiCallMemberEventType } from "../call-types"; @@ -702,7 +701,7 @@ export class ElementCall extends Call { // To use Element Call without touching room state, we create a virtual // widget (one that doesn't have a corresponding state event) const url = ElementCall.generateWidgetUrl(client, roomId); - const createdWidget = WidgetStore.instance.addVirtualWidget( + return WidgetStore.instance.addVirtualWidget( { id: secureRandomString(24), // So that it's globally unique creatorUserId: client.getUserId()!, @@ -722,8 +721,6 @@ export class ElementCall extends Call { }, roomId, ); - WidgetStore.instance.emit(UPDATE_EVENT, null); - return createdWidget; } private static getWidgetData( diff --git a/src/stores/CallStore.ts b/src/stores/CallStore.ts index 6347cc898e..6dc41dfce1 100644 --- a/src/stores/CallStore.ts +++ b/src/stores/CallStore.ts @@ -7,10 +7,9 @@ Please see LICENSE files in the repository root for full details. */ import { logger } from "matrix-js-sdk/src/logger"; -import { GroupCallEventHandlerEvent } from "matrix-js-sdk/src/webrtc/groupCallEventHandler"; import { type MatrixRTCSession, MatrixRTCSessionManagerEvents } from "matrix-js-sdk/src/matrixrtc"; -import type { EmptyObject, GroupCall, Room } from "matrix-js-sdk/src/matrix"; +import type { EmptyObject, Room } from "matrix-js-sdk/src/matrix"; import defaultDispatcher from "../dispatcher/dispatcher"; import { UPDATE_EVENT } from "./AsyncStore"; import { AsyncStoreWithClient } from "./AsyncStoreWithClient"; @@ -53,8 +52,6 @@ export class CallStore extends AsyncStoreWithClient { for (const room of this.matrixClient.getRooms()) { this.updateRoom(room); } - this.matrixClient.on(GroupCallEventHandlerEvent.Incoming, this.onGroupCall); - this.matrixClient.on(GroupCallEventHandlerEvent.Outgoing, this.onGroupCall); this.matrixClient.matrixRTC.on(MatrixRTCSessionManagerEvents.SessionStarted, this.onRTCSessionStart); WidgetStore.instance.on(UPDATE_EVENT, this.onWidgets); @@ -85,12 +82,7 @@ export class CallStore extends AsyncStoreWithClient { this.calls.clear(); this._connectedCalls.clear(); - if (this.matrixClient) { - this.matrixClient.off(GroupCallEventHandlerEvent.Incoming, this.onGroupCall); - this.matrixClient.off(GroupCallEventHandlerEvent.Outgoing, this.onGroupCall); - this.matrixClient.off(GroupCallEventHandlerEvent.Ended, this.onGroupCall); - this.matrixClient.matrixRTC.off(MatrixRTCSessionManagerEvents.SessionStarted, this.onRTCSessionStart); - } + this.matrixClient?.matrixRTC.off(MatrixRTCSessionManagerEvents.SessionStarted, this.onRTCSessionStart); WidgetStore.instance.off(UPDATE_EVENT, this.onWidgets); } @@ -117,8 +109,16 @@ export class CallStore extends AsyncStoreWithClient { private calls = new Map(); // Key is room ID private callListeners = new Map unknown>>(); + private inUpdateRoom = false; private updateRoom(room: Room): void { - if (!this.calls.has(room.roomId)) { + // XXX: This method is guarded with the flag this.inUpdateRoom because + // we need to block this method from calling itself recursively. That + // could happen, for instance, if Call.get adds a new virtual widget to + // the WidgetStore, firing a WidgetStore update that we don't actually + // care about. Without the guard we could get duplicate Call objects + // fighting for control over the same widget. + if (!this.inUpdateRoom && !this.calls.has(room.roomId)) { + this.inUpdateRoom = true; const call = Call.get(room); if (call) { @@ -149,6 +149,7 @@ export class CallStore extends AsyncStoreWithClient { } this.emit(CallStoreEvent.Call, call, room.roomId); + this.inUpdateRoom = false; } } @@ -186,7 +187,6 @@ export class CallStore extends AsyncStoreWithClient { } }; - private onGroupCall = (groupCall: GroupCall): void => this.updateRoom(groupCall.room); private onRTCSessionStart = (roomId: string, session: MatrixRTCSession): void => { this.updateRoom(session.room); }; diff --git a/src/stores/WidgetStore.ts b/src/stores/WidgetStore.ts index 5a76df7103..fbae950dbc 100644 --- a/src/stores/WidgetStore.ts +++ b/src/stores/WidgetStore.ts @@ -189,6 +189,7 @@ export default class WidgetStore extends AsyncStoreWithClient { const app = WidgetUtils.makeAppConfig(widget.id, widget, widget.creatorUserId, roomId, undefined); this.widgetMap.set(WidgetUtils.getWidgetUid(app), app); this.roomMap.get(roomId)!.widgets.push(app); + this.emit(UPDATE_EVENT, roomId); return app; } @@ -198,6 +199,7 @@ export default class WidgetStore extends AsyncStoreWithClient { if (roomApps) { roomApps.widgets = roomApps.widgets.filter((app) => !(app.id === widgetId && app.roomId === roomId)); } + this.emit(UPDATE_EVENT, roomId); } } diff --git a/test/test-utils/call.ts b/test/test-utils/call.ts index 36fc2b505f..c122d1d756 100644 --- a/test/test-utils/call.ts +++ b/test/test-utils/call.ts @@ -7,11 +7,29 @@ Please see LICENSE files in the repository root for full details. */ import { MatrixWidgetType } from "matrix-widget-api"; +import { + type GroupCall, + Room, + type RoomMember, + type MatrixEvent, + type MatrixClient, + PendingEventOrdering, + KnownMembership, + RoomStateEvent, + type IContent, +} from "matrix-js-sdk/src/matrix"; +import { mocked, type Mocked } from "jest-mock"; +import { type MatrixRTCSession } from "matrix-js-sdk/src/matrixrtc"; -import type { GroupCall, Room, RoomMember, MatrixEvent } from "matrix-js-sdk/src/matrix"; -import { mkEvent } from "./test-utils"; +import { mkEvent, mkRoomMember, setupAsyncStoreWithClient, stubClient } from "./test-utils"; import { Call, type ConnectionState, ElementCall, JitsiCall } from "../../src/models/Call"; import { CallStore } from "../../src/stores/CallStore"; +import { MatrixClientPeg } from "../../src/MatrixClientPeg"; +import DMRoomMap from "../../src/utils/DMRoomMap"; +import { MockEventEmitter } from "./client"; +import WidgetStore from "../../src/stores/WidgetStore"; +import { WidgetMessagingStore } from "../../src/stores/widgets/WidgetMessagingStore"; +import SettingsStore from "../../src/settings/SettingsStore"; export class MockedCall extends Call { public static readonly EVENT_TYPE = "org.example.mocked_call"; @@ -105,8 +123,92 @@ export class MockedCall extends Call { /** * Sets up the call store to use mocked calls. */ -export const useMockedCalls = () => { +export function useMockedCalls() { Call.get = (room) => MockedCall.get(room); JitsiCall.create = async (room) => MockedCall.create(room, "1"); ElementCall.create = (room) => MockedCall.create(room, "1"); -}; +} + +/** + * Enables the feature flags required for call tests. + */ +export function enableCalls(): { enabledSettings: Set } { + const enabledSettings = new Set(["feature_group_calls", "feature_video_rooms", "feature_element_call_video_rooms"]); + jest.spyOn(SettingsStore, "getValue").mockImplementation((settingName): any => { + if (settingName.startsWith("feature_")) return enabledSettings.has(settingName); + if (settingName === "activeCallRoomIds") return []; + return undefined; + }); + return { enabledSettings }; +} + +export function setUpClientRoomAndStores(): { + client: Mocked; + room: Room; + alice: RoomMember; + bob: RoomMember; + carol: RoomMember; + roomSession: Mocked; +} { + stubClient(); + const client = mocked(MatrixClientPeg.safeGet()); + DMRoomMap.makeShared(client); + + const room = new Room("!1:example.org", client, "@alice:example.org", { + pendingEventOrdering: PendingEventOrdering.Detached, + }); + + const alice = mkRoomMember(room.roomId, "@alice:example.org"); + const bob = mkRoomMember(room.roomId, "@bob:example.org"); + const carol = mkRoomMember(room.roomId, "@carol:example.org"); + jest.spyOn(room, "getMember").mockImplementation((userId) => { + switch (userId) { + case alice.userId: + return alice; + case bob.userId: + return bob; + case carol.userId: + return carol; + default: + return null; + } + }); + + jest.spyOn(room, "getMyMembership").mockReturnValue(KnownMembership.Join); + + client.getRoom.mockImplementation((roomId) => (roomId === room.roomId ? room : null)); + + const roomSession = new MockEventEmitter({ + memberships: [], + getOldestMembership: jest.fn().mockReturnValue(undefined), + room, + }) as Mocked; + + client.matrixRTC.getRoomSession.mockReturnValue(roomSession); + client.getRooms.mockReturnValue([room]); + client.getUserId.mockReturnValue(alice.userId); + client.getDeviceId.mockReturnValue("alices_device"); + client.reEmitter.reEmit(room, [RoomStateEvent.Events]); + client.sendStateEvent.mockImplementation(async (roomId, eventType, content, stateKey = "") => { + if (roomId !== room.roomId) throw new Error("Unknown room"); + const event = mkEvent({ + event: true, + type: eventType, + room: roomId, + user: alice.userId, + skey: stateKey, + content: content as IContent, + }); + room.addLiveEvents([event], { addToState: true }); + return { event_id: event.getId()! }; + }); + + setupAsyncStoreWithClient(WidgetStore.instance, client); + setupAsyncStoreWithClient(WidgetMessagingStore.instance, client); + + return { client, room, alice, bob, carol, roomSession }; +} + +export function cleanUpClientRoomAndStores(client: MatrixClient, room: Room) { + client.reEmitter.stopReEmitting(room, [RoomStateEvent.Events]); +} diff --git a/test/unit-tests/models/Call-test.ts b/test/unit-tests/models/Call-test.ts index a91f951be3..d25d477395 100644 --- a/test/unit-tests/models/Call-test.ts +++ b/test/unit-tests/models/Call-test.ts @@ -11,12 +11,9 @@ import { mocked } from "jest-mock"; import { waitFor } from "jest-matrix-react"; import { RoomType, - Room, + type Room, RoomEvent, MatrixEvent, - RoomStateEvent, - PendingEventOrdering, - type IContent, type MatrixClient, type IMyDevice, type RoomMember, @@ -41,15 +38,7 @@ import { ElementCall, ElementCallIntent, } from "../../../src/models/Call"; -import { - stubClient, - mkEvent, - mkRoomMember, - setupAsyncStoreWithClient, - mockPlatformPeg, - MockEventEmitter, -} from "../../test-utils"; -import { MatrixClientPeg } from "../../../src/MatrixClientPeg"; +import { cleanUpClientRoomAndStores, enableCalls, mockPlatformPeg, setUpClientRoomAndStores } from "../../test-utils"; import WidgetStore from "../../../src/stores/WidgetStore"; import { WidgetMessagingStore } from "../../../src/stores/widgets/WidgetMessagingStore"; import ActiveWidgetStore, { ActiveWidgetStoreEvent } from "../../../src/stores/ActiveWidgetStore"; @@ -60,80 +49,7 @@ import { type SettingKey } from "../../../src/settings/Settings.tsx"; import SdkConfig from "../../../src/SdkConfig.ts"; import DMRoomMap from "../../../src/utils/DMRoomMap.ts"; -const enabledSettings = new Set(["feature_group_calls", "feature_video_rooms", "feature_element_call_video_rooms"]); -jest.spyOn(SettingsStore, "getValue").mockImplementation( - (settingName): any => enabledSettings.has(settingName) || undefined, -); - -const setUpClientRoomAndStores = (): { - client: Mocked; - room: Room; - alice: RoomMember; - bob: RoomMember; - carol: RoomMember; - roomSession: Mocked; -} => { - stubClient(); - const client = mocked(MatrixClientPeg.safeGet()); - DMRoomMap.makeShared(client); - - const room = new Room("!1:example.org", client, "@alice:example.org", { - pendingEventOrdering: PendingEventOrdering.Detached, - }); - - const alice = mkRoomMember(room.roomId, "@alice:example.org"); - const bob = mkRoomMember(room.roomId, "@bob:example.org"); - const carol = mkRoomMember(room.roomId, "@carol:example.org"); - jest.spyOn(room, "getMember").mockImplementation((userId) => { - switch (userId) { - case alice.userId: - return alice; - case bob.userId: - return bob; - case carol.userId: - return carol; - default: - return null; - } - }); - - jest.spyOn(room, "getMyMembership").mockReturnValue(KnownMembership.Join); - - client.getRoom.mockImplementation((roomId) => (roomId === room.roomId ? room : null)); - - const roomSession = new MockEventEmitter({ - memberships: [], - getOldestMembership: jest.fn().mockReturnValue(undefined), - }) as Mocked; - - client.matrixRTC.getRoomSession.mockReturnValue(roomSession); - client.getRooms.mockReturnValue([room]); - client.getUserId.mockReturnValue(alice.userId); - client.getDeviceId.mockReturnValue("alices_device"); - client.reEmitter.reEmit(room, [RoomStateEvent.Events]); - client.sendStateEvent.mockImplementation(async (roomId, eventType, content, stateKey = "") => { - if (roomId !== room.roomId) throw new Error("Unknown room"); - const event = mkEvent({ - event: true, - type: eventType, - room: roomId, - user: alice.userId, - skey: stateKey, - content: content as IContent, - }); - room.addLiveEvents([event], { addToState: true }); - return { event_id: event.getId()! }; - }); - - setupAsyncStoreWithClient(WidgetStore.instance, client); - setupAsyncStoreWithClient(WidgetMessagingStore.instance, client); - - return { client, room, alice, bob, carol, roomSession }; -}; - -const cleanUpClientRoomAndStores = (client: MatrixClient, room: Room) => { - client.reEmitter.stopReEmitting(room, [RoomStateEvent.Events]); -}; +const { enabledSettings } = enableCalls(); const setUpWidget = (call: Call): { widget: Widget; messaging: Mocked } => { call.widget.data = { ...call.widget, skipLobby: true }; diff --git a/test/unit-tests/stores/CallStore-test.ts b/test/unit-tests/stores/CallStore-test.ts new file mode 100644 index 0000000000..30f42757e2 --- /dev/null +++ b/test/unit-tests/stores/CallStore-test.ts @@ -0,0 +1,38 @@ +/* + * Copyright 2025 New Vector Ltd. + * + * SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial + * Please see LICENSE files in the repository root for full details. + */ + +import { type CallMembership, MatrixRTCSessionManagerEvents } from "matrix-js-sdk/src/matrixrtc"; + +import { ElementCall } from "../../../src/models/Call"; +import { CallStore } from "../../../src/stores/CallStore"; +import { + setUpClientRoomAndStores, + cleanUpClientRoomAndStores, + setupAsyncStoreWithClient, + enableCalls, +} from "../../test-utils"; + +enableCalls(); + +test("CallStore constructs one call for one MatrixRTC session", () => { + const { client, room } = setUpClientRoomAndStores(); + try { + setupAsyncStoreWithClient(CallStore.instance, client); + const getSpy = jest.spyOn(ElementCall, "get"); + + // Simulate another user starting a new MatrixRTC session + const session = client.matrixRTC.getRoomSession(room); + session.memberships.push({} as CallMembership); + client.matrixRTC.emit(MatrixRTCSessionManagerEvents.SessionStarted, room.roomId, session); + + expect(getSpy).toHaveBeenCalledTimes(1); + expect(getSpy).toHaveReturnedWith(expect.any(ElementCall)); + expect(CallStore.instance.getCall(room.roomId)).not.toBe(null); + } finally { + cleanUpClientRoomAndStores(client, room); + } +});