fix more race conditions, and apply changes from review

src/components/views/rooms/UserIdentityWarning.tsx

@@ -80,18 +80,28 @@ export const UserIdentityWarning: React.FC<UserIdentityWarningProps> = ({ room }
     // with the newer value, so it will fix itself in the end.
     const gotVerificationStatusUpdateRef = useRef<Map<string, boolean>>(new Map());
 
-    // Select a new user to display a warning for.  This is called after the
+    // Update the current prompt.  Select a new user if needed, or hide the
-    // current prompted user no longer needs their identity approved.
+    // warning if we don't have anyone to warn about.
-    const selectCurrentPrompt = useCallback((): RoomMember | undefined => {
+    const updateCurrentPrompt = useCallback((): undefined => {
         const membersNeedingApproval = membersNeedingApprovalRef.current;
-        if (membersNeedingApproval.size === 0) {
+        // We have to do this in a callback to `setCurrentPrompt`
-            return undefined;
+        // because this function could have been called after an
-        }
+        // `await`, and the `currentPrompt` that this function would
+        // have may be outdated.
+        setCurrentPrompt((currentPrompt) => {
+            // If we're already displaying a warning, and that user still needs
+            // approval, continue showing that user.
+            if (currentPrompt && membersNeedingApproval.has(currentPrompt.userId)) return currentPrompt;
 
-        // We pick the user with the smallest user ID.
+            if (membersNeedingApproval.size === 0) {
-        const keys = Array.from(membersNeedingApproval.keys()).sort((a, b) => a.localeCompare(b));
+                return undefined;
-        const selection = membersNeedingApproval.get(keys[0]!);
+            }
-        return selection;
+
+            // We pick the user with the smallest user ID.
+            const keys = Array.from(membersNeedingApproval.keys()).sort((a, b) => a.localeCompare(b));
+            const selection = membersNeedingApproval.get(keys[0]!);
+            return selection;
+        });
     }, []);
 
     // Add a user to the membersNeedingApproval map, and update the current
@@ -105,54 +115,52 @@ export const UserIdentityWarning: React.FC<UserIdentityWarningProps> = ({ room }
                 return;
             }
             member = member ?? room.getMember(userId) ?? undefined;
-            if (member) {
+            if (!member) return;
-                membersNeedingApprovalRef.current.set(userId, member);
+
-                // We only select the prompt if we are done initialising,
+            membersNeedingApprovalRef.current.set(userId, member);
-                // because we will select the prompt after we're done
+            // We only select the prompt if we are done initialising,
-                // initialising, and we want to start by displaying a warning
+            // because we will select the prompt after we're done
-                // for the user with the smallest ID.
+            // initialising, and we want to start by displaying a warning
-                if (initialisedRef.current === InitialisationStatus.Completed) {
+            // for the user with the smallest ID.
-                    setCurrentPrompt((currentPrompt) => {
+            if (initialisedRef.current === InitialisationStatus.Completed) {
-                        // If we aren't currently displaying a warning, we pick
+                updateCurrentPrompt();
-                        // a new user to show a warning for.  If we are already
-                        // displaying a warning, don't change the display.
-                        //
-                        // We have to do this in a callback to
-                        // `setCurrentPrompt` because this function could have
-                        // been called after an `await`, and the `currentPrompt`
-                        // that this function would have may be outdated.
-                        if (!currentPrompt) {
-                            return selectCurrentPrompt();
-                        } else {
-                            return currentPrompt;
-                        }
-                    });
-                }
             }
         },
-        [cli, room, selectCurrentPrompt],
+        [cli, room, updateCurrentPrompt],
     );
 
     // Check if the user's identity needs approval, and if so, add them to the
     // membersNeedingApproval map and update the prompt if needed. They will
     // only be added if they are a member of the room.
-    const addMemberIfNeedsApproval = useCallback(
+    const addMembersWhoNeedApproval = useCallback(
-        async (userId: string, member?: RoomMember): Promise<void> => {
+        async (members: RoomMember[]): Promise<void> => {
             const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current;
-            const membersNeedingApproval = membersNeedingApprovalRef.current;
 
-            if (gotVerificationStatusUpdate.has(userId)) {
+            const promises: Promise<void>[] = [];
-                // We're already checking their verification status, so we don't
+
-                // need to do anything here.
+            for (const member of members) {
-                return;
+                const userId = member.userId;
-            }
+                if (gotVerificationStatusUpdate.has(userId)) {
-            gotVerificationStatusUpdate.set(userId, false);
+                    // We're already checking their verification status, so we don't
-            if (await userNeedsApproval(crypto!, userId)) {
+                    // need to do anything here.
-                if (!membersNeedingApproval.has(userId) && gotVerificationStatusUpdate.get(userId) === false) {
+                    continue;
-                    addMemberNeedingApproval(userId, member);
                 }
+                gotVerificationStatusUpdate.set(userId, false);
+                promises.push(
+                    userNeedsApproval(crypto!, userId).then((needsApproval) => {
+                        if (needsApproval) {
+                            // Only actually update the list if we haven't received a
+                            // `UserTrustStatusChanged` for this user in the meantime.
+                            if (gotVerificationStatusUpdate.get(userId) === false) {
+                                addMemberNeedingApproval(userId, member);
+                            }
+                        }
+                        gotVerificationStatusUpdate.delete(userId);
+                    }),
+                );
             }
-            gotVerificationStatusUpdate.delete(userId);
+
+            await Promise.all(promises);
         },
         [crypto, addMemberNeedingApproval],
     );
@@ -162,20 +170,15 @@ export const UserIdentityWarning: React.FC<UserIdentityWarningProps> = ({ room }
     const removeMemberNeedingApproval = useCallback(
         (userId: string): void => {
             membersNeedingApprovalRef.current.delete(userId);
-
+            updateCurrentPrompt();
-            // If we removed the currently displayed user, we need to pick a new one
-            // to display.
-            if (currentPrompt?.userId === userId) {
-                setCurrentPrompt(selectCurrentPrompt());
-            }
         },
-        [currentPrompt, selectCurrentPrompt],
+        [updateCurrentPrompt],
     );
 
     // Initialise the component.  Get the room members, check which ones need
     // their identity approved, and pick one to display.
     const loadMembers = useCallback(async (): Promise<void> => {
-        if (!crypto || initialisedRef.current != InitialisationStatus.Uninitialised) {
+        if (!crypto || initialisedRef.current !== InitialisationStatus.Uninitialised) {
             return;
         }
         // If encryption is not enabled in the room, we don't need to do
@@ -187,14 +190,11 @@ export const UserIdentityWarning: React.FC<UserIdentityWarningProps> = ({ room }
         initialisedRef.current = InitialisationStatus.Initialising;
 
         const members = await room.getEncryptionTargetMembers();
+        await addMembersWhoNeedApproval(members);
 
-        for (const member of members) {
+        updateCurrentPrompt();
-            await addMemberIfNeedsApproval(member.userId, member);
-        }
-
-        setCurrentPrompt(selectCurrentPrompt());
         initialisedRef.current = InitialisationStatus.Completed;
-    }, [crypto, room, addMemberIfNeedsApproval, selectCurrentPrompt]);
+    }, [crypto, room, addMembersWhoNeedApproval, updateCurrentPrompt]);
 
     loadMembers().catch((e) => {
         logger.error("Error initialising UserIdentityWarning:", e);
@@ -213,6 +213,9 @@ export const UserIdentityWarning: React.FC<UserIdentityWarningProps> = ({ room }
             }
 
             if (gotVerificationStatusUpdate.has(userId)) {
+                // There is an ongoing call to `addMembersWhoNeedApproval`. Flag
+                // to it that we've got a more up-to-date result so it should
+                // discard its result.
                 gotVerificationStatusUpdate.set(userId, true);
             }
 
@@ -259,14 +262,27 @@ export const UserIdentityWarning: React.FC<UserIdentityWarningProps> = ({ room }
             ) {
                 // Someone's membership changed and we will now encrypt to them.  If
                 // their identity needs approval, show a warning.
-                await addMemberIfNeedsApproval(userId);
+                const member = room.getMember(userId);
+                if (member) {
+                    await addMembersWhoNeedApproval([member]).catch((e) => {
+                        logger.error("Error adding member in UserIdentityWarning:", e);
+                    });
+                }
             } else {
                 // Someone's membership changed and we no longer encrypt to them.
                 // If we're showing a warning about them, we don't need to any more.
                 removeMemberNeedingApproval(userId);
+                const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current;
+                if (gotVerificationStatusUpdate.has(userId)) {
+                    // There is an ongoing call to `addMembersWhoNeedApproval`.
+                    // Indicate that we've received a verification status update
+                    // to prevent it from trying to add the user as needing
+                    // verification.
+                    gotVerificationStatusUpdate.set(userId, true);
+                }
             }
         },
-        [crypto, room, addMemberIfNeedsApproval, removeMemberNeedingApproval, loadMembers],
+        [crypto, room, addMembersWhoNeedApproval, removeMemberNeedingApproval, loadMembers],
     );
     useTypedEventEmitter(cli, RoomStateEvent.Events, onRoomStateEvent);
 

test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx

@@ -350,6 +350,89 @@ describe("UserIdentityWarning", () => {
                 expect(() => getWarningByText("@bob:example.org's identity appears to have changed.")).toThrow(),
             );
         });
+
+        it("when member leaves immediately after component is loaded", async () => {
+            jest.spyOn(room, "getEncryptionTargetMembers").mockImplementation(async () => {
+                setTimeout(() => {
+                    // Alice immediately leaves after we get the room
+                    // membership, so we shouldn't show the warning any more
+                    client.emit(
+                        RoomStateEvent.Events,
+                        new MatrixEvent({
+                            event_id: "$event_id",
+                            type: EventType.RoomMember,
+                            state_key: "@alice:example.org",
+                            content: {
+                                membership: "leave",
+                            },
+                            room_id: ROOM_ID,
+                            sender: "@alice:example.org",
+                        }),
+                        dummyRoomState(),
+                        null,
+                    );
+                });
+                return [mockRoomMember("@alice:example.org")];
+            });
+            jest.spyOn(room, "getMember").mockImplementation((userId) => mockRoomMember(userId));
+            jest.spyOn(room, "shouldEncryptForInvitedMembers").mockReturnValue(false);
+            const crypto = client.getCrypto()!;
+            jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue(
+                new UserVerificationStatus(false, false, false, true),
+            );
+            renderComponent(client, room);
+
+            await sleep(10);
+            expect(() => getWarningByText("@alice:example.org's identity appears to have changed.")).toThrow();
+        });
+
+        it("when member leaves immediately after joining", async () => {
+            // Nobody in the room yet
+            jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([]);
+            jest.spyOn(room, "getMember").mockImplementation((userId) => mockRoomMember(userId));
+            jest.spyOn(room, "shouldEncryptForInvitedMembers").mockReturnValue(false);
+            const crypto = client.getCrypto()!;
+            jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue(
+                new UserVerificationStatus(false, false, false, true),
+            );
+            renderComponent(client, room);
+            await sleep(10); // give it some time to finish initialising
+
+            // Alice joins.  Her identity needs approval, so we should show a warning.
+            client.emit(
+                RoomStateEvent.Events,
+                new MatrixEvent({
+                    event_id: "$event_id",
+                    type: EventType.RoomMember,
+                    state_key: "@alice:example.org",
+                    content: {
+                        membership: "join",
+                    },
+                    room_id: ROOM_ID,
+                    sender: "@alice:example.org",
+                }),
+                dummyRoomState(),
+                null,
+            );
+            // ... but she immediately leaves, so we shouldn't show the warning any more
+            client.emit(
+                RoomStateEvent.Events,
+                new MatrixEvent({
+                    event_id: "$event_id",
+                    type: EventType.RoomMember,
+                    state_key: "@alice:example.org",
+                    content: {
+                        membership: "leave",
+                    },
+                    room_id: ROOM_ID,
+                    sender: "@alice:example.org",
+                }),
+                dummyRoomState(),
+                null,
+            );
+            await sleep(10); // give it some time to finish
+            expect(() => getWarningByText("@alice:example.org's identity appears to have changed.")).toThrow();
+        });
     });
 
     // When we have multiple users whose identity needs approval, one user's