Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Commit

Permalink
Fix infinite loop when pinning/unpinning persistent widgets (#8396)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@robintown
robintown authored Apr 25, 2022
1 parent b2bebda commit 6bfe043
Show file tree
Hide file tree
Showing 5 changed files with 124 additions and 83 deletions.
56 changes: 11 additions & 45 deletions src/components/views/elements/AppTile.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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));
Expand Down Expand Up @@ -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();
}

Expand Down
21 changes: 14 additions & 7 deletions src/components/views/voip/PipView.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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));
}

Expand All @@ -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));
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 });
}
Expand Down
40 changes: 37 additions & 3 deletions src/stores/ActiveWidgetStore.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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 {
Expand All @@ -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;
37 changes: 9 additions & 28 deletions test/components/views/elements/AppTile-test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand All @@ -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();
});
Expand Down Expand Up @@ -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") {
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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.
Expand All @@ -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 () => {
Expand Down
53 changes: 53 additions & 0 deletions test/stores/ActiveWidgetStore-test.ts
View file
Original file line number Diff line number Diff line change
@@ -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);
});
});

0 comments on commit 6bfe043

Please sign in to comment.