Fix infinite loop when pinning/unpinning persistent widgets (#8396)

Pinning or unpinning a persistent widget, such as Jitsi, could cause the
PiP view and the app drawer to fight for control over the widget, since
the PiP view never realized that it was supposed to relinquish control.
This was due to a race between the WidgetLayoutStore update and the
AppTile lifecycle tracking update.

Author: robintown

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);
+    });
+});