Fix infinite loop when pinning/unpinning persistent widgets

src/components/views/elements/AppTile.tsx

@@ -117,30 +117,6 @@ export default class AppTile extends React.Component<IProps, IState> {
         showLayoutButtons: true,
     };
 
-    // We track a count of all "live" `AppTile`s for a given widget UID.
-    // For this purpose, an `AppTile` is considered live from the time it is
-    // constructed until it is unmounted. This is used to aid logic around when
-    // to tear down the widget iframe. See `componentWillUnmount` for details.
-    private static liveTilesByUid = new Map<string, number>();
-
-    public static addLiveTile(widgetId: string, roomId: string): void {
-        const uid = WidgetUtils.calcWidgetUid(widgetId, roomId);
-        const refs = this.liveTilesByUid.get(uid) ?? 0;
-        this.liveTilesByUid.set(uid, refs + 1);
-    }
-
-    public static removeLiveTile(widgetId: string, roomId: string): void {
-        const uid = WidgetUtils.calcWidgetUid(widgetId, roomId);
-        const refs = this.liveTilesByUid.get(uid);
-        if (refs) this.liveTilesByUid.set(uid, refs - 1);
-    }
-
-    public static isLive(widgetId: string, roomId: string): boolean {
-        const uid = WidgetUtils.calcWidgetUid(widgetId, roomId);
-        const refs = this.liveTilesByUid.get(uid) ?? 0;
-        return refs > 0;
-    }
-
     private contextMenuButton = createRef<any>();
     private iframe: HTMLIFrameElement; // ref to the iframe (callback style)
     private allowedWidgetsWatchRef: string;
@@ -152,7 +128,10 @@ export default class AppTile extends React.Component<IProps, IState> {
     constructor(props: IProps) {
         super(props);
 
-        AppTile.addLiveTile(this.props.app.id, this.props.app.roomId);
+        // Tiles in miniMode are floating, and therefore not docked
+        if (!this.props.miniMode) {
+            ActiveWidgetStore.instance.dockWidget(this.props.app.id, this.props.app.roomId);
+        }
 
         // The key used for PersistedElement
         this.persistKey = getPersistKey(WidgetUtils.getWidgetUid(this.props.app));
@@ -284,27 +263,14 @@ export default class AppTile extends React.Component<IProps, IState> {
     public componentWillUnmount(): void {
         this.unmounted = true;
 
-        // It might seem simplest to always tear down the widget itself here,
-        // and indeed that would be a bit easier to reason about... however, we
-        // support moving widgets between containers (e.g. top <-> center).
-        // During such a move, this component will unmount from the old
-        // container and remount in the new container. By keeping the widget
-        // iframe loaded across this transition, the widget doesn't notice that
-        // anything happened, which improves overall widget UX. During this kind
-        // of movement between containers, the new `AppTile` for the new
-        // container is constructed before the old one unmounts. By counting the
-        // mounted `AppTile`s for each widget, we know to only tear down the
-        // widget iframe when the last the `AppTile` unmounts.
-        AppTile.removeLiveTile(this.props.app.id, this.props.app.roomId);
-
-        // We also support a separate "persistence" mode where a single widget
-        // can request to be "sticky" and follow you across rooms in a PIP
-        // container.
-        const isActiveWidget = ActiveWidgetStore.instance.getWidgetPersistence(
-            this.props.app.id, this.props.app.roomId,
-        );
+        if (!this.props.miniMode) {
+            ActiveWidgetStore.instance.undockWidget(this.props.app.id, this.props.app.roomId);
+        }
 
-        if (!AppTile.isLive(this.props.app.id, this.props.app.roomId) && !isActiveWidget) {
+        // Only tear down the widget if no other component is keeping it alive,
+        // because we support moving widgets between containers, in which case
+        // another component will keep it loaded throughout the transition
+        if (!ActiveWidgetStore.instance.isLive(this.props.app.id, this.props.app.roomId)) {
             this.endWidgetActions();
         }
 

src/components/views/voip/PipView.tsx

@@ -33,7 +33,6 @@ import { WidgetLayoutStore } from '../../../stores/widgets/WidgetLayoutStore';
 import CallViewHeader from './CallView/CallViewHeader';
 import ActiveWidgetStore, { ActiveWidgetStoreEvent } from '../../../stores/ActiveWidgetStore';
 import { ViewRoomPayload } from "../../../dispatcher/payloads/ViewRoomPayload";
-import AppTile from '../elements/AppTile';
 
 const SHOW_CALL_IN_STATES = [
     CallState.Connected,
@@ -135,7 +134,9 @@ export default class PipView extends React.Component<IProps, IState> {
         if (room) {
             WidgetLayoutStore.instance.on(WidgetLayoutStore.emissionForRoom(room), this.updateCalls);
         }
-        ActiveWidgetStore.instance.on(ActiveWidgetStoreEvent.Update, this.onActiveWidgetStoreUpdate);
+        ActiveWidgetStore.instance.on(ActiveWidgetStoreEvent.Persistence, this.onWidgetPersistence);
+        ActiveWidgetStore.instance.on(ActiveWidgetStoreEvent.Dock, this.onWidgetDockChanges);
+        ActiveWidgetStore.instance.on(ActiveWidgetStoreEvent.Undock, this.onWidgetDockChanges);
         document.addEventListener("mouseup", this.onEndMoving.bind(this));
     }
 
@@ -149,7 +150,9 @@ export default class PipView extends React.Component<IProps, IState> {
         if (room) {
             WidgetLayoutStore.instance.off(WidgetLayoutStore.emissionForRoom(room), this.updateCalls);
         }
-        ActiveWidgetStore.instance.off(ActiveWidgetStoreEvent.Update, this.onActiveWidgetStoreUpdate);
+        ActiveWidgetStore.instance.off(ActiveWidgetStoreEvent.Persistence, this.onWidgetPersistence);
+        ActiveWidgetStore.instance.off(ActiveWidgetStoreEvent.Dock, this.onWidgetDockChanges);
+        ActiveWidgetStore.instance.off(ActiveWidgetStoreEvent.Undock, this.onWidgetDockChanges);
         document.removeEventListener("mouseup", this.onEndMoving.bind(this));
     }
 
@@ -186,13 +189,17 @@ export default class PipView extends React.Component<IProps, IState> {
         this.updateShowWidgetInPip();
     };
 
-    private onActiveWidgetStoreUpdate = (): void => {
+    private onWidgetPersistence = (): void => {
         this.updateShowWidgetInPip(
             ActiveWidgetStore.instance.getPersistentWidgetId(),
             ActiveWidgetStore.instance.getPersistentRoomId(),
         );
     };
 
+    private onWidgetDockChanges = (): void => {
+        this.updateShowWidgetInPip();
+    };
+
     private updateCalls = (): void => {
         if (!this.state.viewedRoomId) return;
         const [primaryCall, secondaryCalls] = getPrimarySecondaryCallsForPip(this.state.viewedRoomId);
@@ -231,19 +238,19 @@ export default class PipView extends React.Component<IProps, IState> {
         persistentRoomId = this.state.persistentRoomId,
     ) {
         let fromAnotherRoom = false;
-        let notVisible = false;
+        let notDocked = false;
         // Sanity check the room - the widget may have been destroyed between render cycles, and
         // thus no room is associated anymore.
         if (persistentWidgetId && MatrixClientPeg.get().getRoom(persistentRoomId)) {
-            notVisible = !AppTile.isLive(persistentWidgetId, persistentRoomId);
+            notDocked = !ActiveWidgetStore.instance.isDocked(persistentWidgetId, persistentRoomId);
             fromAnotherRoom = this.state.viewedRoomId !== persistentRoomId;
         }
 
         // The widget should only be shown as a persistent app (in a floating
         // pip container) if it is not visible on screen: either because we are
         // viewing a different room OR because it is in none of the possible
         // containers of the room view.
-        const showWidgetInPip = fromAnotherRoom || notVisible;
+        const showWidgetInPip = fromAnotherRoom || notDocked;
 
         this.setState({ showWidgetInPip, persistentWidgetId, persistentRoomId });
     }

src/stores/ActiveWidgetStore.ts

@@ -23,19 +23,25 @@ import WidgetUtils from "../utils/WidgetUtils";
 import { WidgetMessagingStore } from "./widgets/WidgetMessagingStore";
 
 export enum ActiveWidgetStoreEvent {
-    Update = "update",
+    // Indicates a change in the currently persistent widget
+    Persistence = "persistence",
+    // Indicate changes in the currently docked widgets
+    Dock = "dock",
+    Undock = "undock",
 }
 
 /**
  * Stores information about the widgets active in the app right now:
  *  * What widget is set to remain always-on-screen, if any
  *    Only one widget may be 'always on screen' at any one time.
- *  * Negotiated capabilities for active apps
+ *  * Reference counts to keep track of whether a widget is kept docked or alive
+ *    by any components
  */
 export default class ActiveWidgetStore extends EventEmitter {
     private static internalInstance: ActiveWidgetStore;
     private persistentWidgetId: string;
     private persistentRoomId: string;
+    private dockedWidgetsByUid = new Map<string, number>();
 
     public static get instance(): ActiveWidgetStore {
         if (!ActiveWidgetStore.internalInstance) {
@@ -79,7 +85,7 @@ export default class ActiveWidgetStore extends EventEmitter {
             this.persistentWidgetId = widgetId;
             this.persistentRoomId = roomId;
         }
-        this.emit(ActiveWidgetStoreEvent.Update);
+        this.emit(ActiveWidgetStoreEvent.Persistence);
     }
 
     public getWidgetPersistence(widgetId: string, roomId: string): boolean {
@@ -93,6 +99,34 @@ export default class ActiveWidgetStore extends EventEmitter {
     public getPersistentRoomId(): string {
         return this.persistentRoomId;
     }
+
+    // Registers the given widget as being docked somewhere in the UI (not a PiP),
+    // to allow its lifecycle to be tracked.
+    public dockWidget(widgetId: string, roomId: string): void {
+        const uid = WidgetUtils.calcWidgetUid(widgetId, roomId);
+        const refs = this.dockedWidgetsByUid.get(uid) ?? 0;
+        this.dockedWidgetsByUid.set(uid, refs + 1);
+        if (refs === 0) this.emit(ActiveWidgetStoreEvent.Dock);
+    }
+
+    public undockWidget(widgetId: string, roomId: string): void {
+        const uid = WidgetUtils.calcWidgetUid(widgetId, roomId);
+        const refs = this.dockedWidgetsByUid.get(uid);
+        if (refs) this.dockedWidgetsByUid.set(uid, refs - 1);
+        if (refs === 1) this.emit(ActiveWidgetStoreEvent.Undock);
+    }
+
+    // Determines whether the given widget is docked anywhere in the UI (not a PiP)
+    public isDocked(widgetId: string, roomId: string): boolean {
+        const uid = WidgetUtils.calcWidgetUid(widgetId, roomId);
+        const refs = this.dockedWidgetsByUid.get(uid) ?? 0;
+        return refs > 0;
+    }
+
+    // Determines whether the given widget is being kept alive in the UI, including PiPs
+    public isLive(widgetId: string, roomId: string): boolean {
+        return this.isDocked(widgetId, roomId) || this.getWidgetPersistence(widgetId, roomId);
+    }
 }
 
 window.mxActiveWidgetStore = ActiveWidgetStore.instance;

test/components/views/elements/AppTile-test.tsx

@@ -33,6 +33,7 @@ import { RightPanelPhases } from "../../../../src/stores/right-panel/RightPanelS
 import RightPanelStore from "../../../../src/stores/right-panel/RightPanelStore";
 import { UPDATE_EVENT } from "../../../../src/stores/AsyncStore";
 import WidgetStore, { IApp } from "../../../../src/stores/WidgetStore";
+import ActiveWidgetStore from "../../../../src/stores/ActiveWidgetStore";
 import AppTile from "../../../../src/components/views/elements/AppTile";
 import { Container, WidgetLayoutStore } from "../../../../src/stores/widgets/WidgetLayoutStore";
 import AppsDrawer from "../../../../src/components/views/rooms/AppsDrawer";
@@ -116,26 +117,6 @@ describe("AppTile", () => {
         jest.spyOn(SettingsStore, "getValue").mockRestore();
     });
 
-    it("tracks live tiles correctly", () => {
-        expect(AppTile.isLive("1", "r1")).toEqual(false);
-
-        // Try removing the tile before it gets added
-        AppTile.removeLiveTile("1", "r1");
-        expect(AppTile.isLive("1", "r1")).toEqual(false);
-
-        AppTile.addLiveTile("1", "r1");
-        expect(AppTile.isLive("1", "r1")).toEqual(true);
-
-        AppTile.addLiveTile("1", "r1");
-        expect(AppTile.isLive("1", "r1")).toEqual(true);
-
-        AppTile.removeLiveTile("1", "r1");
-        expect(AppTile.isLive("1", "r1")).toEqual(true);
-
-        AppTile.removeLiveTile("1", "r1");
-        expect(AppTile.isLive("1", "r1")).toEqual(false);
-    });
-
     it("destroys non-persisted right panel widget on room change", async () => {
         // Set up right panel state
         const realGetValue = SettingsStore.getValue;
@@ -170,7 +151,7 @@ describe("AppTile", () => {
         });
         await rpsUpdated;
 
-        expect(AppTile.isLive("1", "r1")).toBe(true);
+        expect(ActiveWidgetStore.instance.isLive("1", "r1")).toBe(true);
 
         // We want to verify that as we change to room 2, we should close the
         // right panel and destroy the widget.
@@ -190,7 +171,7 @@ describe("AppTile", () => {
         </MatrixClientContext.Provider>);
 
         expect(endWidgetActions.mock.calls.length).toBe(1);
-        expect(AppTile.isLive("1", "r1")).toBe(false);
+        expect(ActiveWidgetStore.instance.isLive("1", "r1")).toBe(false);
 
         mockSettings.mockRestore();
     });
@@ -231,8 +212,8 @@ describe("AppTile", () => {
         });
         await rpsUpdated1;
 
-        expect(AppTile.isLive("1", "r1")).toBe(true);
-        expect(AppTile.isLive("1", "r2")).toBe(false);
+        expect(ActiveWidgetStore.instance.isLive("1", "r1")).toBe(true);
+        expect(ActiveWidgetStore.instance.isLive("1", "r2")).toBe(false);
 
         jest.spyOn(SettingsStore, "getValue").mockImplementation((name, roomId) => {
             if (name === "RightPanel.phases") {
@@ -266,8 +247,8 @@ describe("AppTile", () => {
         </MatrixClientContext.Provider>);
         await rpsUpdated2;
 
-        expect(AppTile.isLive("1", "r1")).toBe(false);
-        expect(AppTile.isLive("1", "r2")).toBe(true);
+        expect(ActiveWidgetStore.instance.isLive("1", "r1")).toBe(false);
+        expect(ActiveWidgetStore.instance.isLive("1", "r2")).toBe(true);
     });
 
     it("preserves non-persisted widget on container move", async () => {
@@ -300,7 +281,7 @@ describe("AppTile", () => {
             />
         </MatrixClientContext.Provider>);
 
-        expect(AppTile.isLive("1", "r1")).toBe(true);
+        expect(ActiveWidgetStore.instance.isLive("1", "r1")).toBe(true);
 
         // We want to verify that as we move the widget to the center container,
         // the widget frame remains running.
@@ -316,7 +297,7 @@ describe("AppTile", () => {
         });
 
         expect(endWidgetActions.mock.calls.length).toBe(0);
-        expect(AppTile.isLive("1", "r1")).toBe(true);
+        expect(ActiveWidgetStore.instance.isLive("1", "r1")).toBe(true);
     });
 
     afterAll(async () => {

test/stores/ActiveWidgetStore-test.ts

@@ -0,0 +1,53 @@
+/*
+Copyright 2022 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+import ActiveWidgetStore from "../../src/stores/ActiveWidgetStore";
+
+describe("ActiveWidgetStore", () => {
+    const store = ActiveWidgetStore.instance;
+
+    it("tracks docked and live tiles correctly", () => {
+        expect(store.isDocked("1", "r1")).toEqual(false);
+        expect(store.isLive("1", "r1")).toEqual(false);
+
+        // Try undocking the widget before it gets docked
+        store.undockWidget("1", "r1");
+        expect(store.isDocked("1", "r1")).toEqual(false);
+        expect(store.isLive("1", "r1")).toEqual(false);
+
+        store.dockWidget("1", "r1");
+        expect(store.isDocked("1", "r1")).toEqual(true);
+        expect(store.isLive("1", "r1")).toEqual(true);
+
+        store.dockWidget("1", "r1");
+        expect(store.isDocked("1", "r1")).toEqual(true);
+        expect(store.isLive("1", "r1")).toEqual(true);
+
+        store.undockWidget("1", "r1");
+        expect(store.isDocked("1", "r1")).toEqual(true);
+        expect(store.isLive("1", "r1")).toEqual(true);
+
+        // Ensure that persistent widgets remain live even while undocked
+        store.setWidgetPersistence("1", "r1", true);
+        store.undockWidget("1", "r1");
+        expect(store.isDocked("1", "r1")).toEqual(false);
+        expect(store.isLive("1", "r1")).toEqual(true);
+
+        store.setWidgetPersistence("1", "r1", false);
+        expect(store.isDocked("1", "r1")).toEqual(false);
+        expect(store.isLive("1", "r1")).toEqual(false);
+    });
+});