From 1f2bf0485ebee982ca5cdb50cac3f525b26b07fc Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Sat, 18 Apr 2020 14:52:41 +0300 Subject: [PATCH 1/7] Implement widget API for signaling the widget to gracefully terminate In theory, widgets could use iframe unload/beforeunload events for cleanup, but in practice browsers have restrictions on what can be done in those events which may not give sufficient time for clean termination. Signed-off-by: Pauli Virtanen --- src/WidgetMessaging.js | 13 +++++++++++++ src/widgets/WidgetApi.ts | 14 ++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/src/WidgetMessaging.js b/src/WidgetMessaging.js index 5f877bd48a..6a20c340a5 100644 --- a/src/WidgetMessaging.js +++ b/src/WidgetMessaging.js @@ -87,6 +87,19 @@ export default class WidgetMessaging { }); } + /** + * Tells the widget that it should terminate now. + * It is not necessarily called in all instances before the widget is removed, + * and the client may force termination with a timeout. + * @returns {Promise<*>} Resolves when widget has acknowledged the message. + */ + terminate() { + return this.messageToWidget({ + api: OUTBOUND_API_NAME, + action: KnownWidgetActions.Terminate, + }); + } + /** * Request a screenshot from a widget * @return {Promise} To be resolved with screenshot data when it has been generated diff --git a/src/widgets/WidgetApi.ts b/src/widgets/WidgetApi.ts index 05237d258f..e08476edb5 100644 --- a/src/widgets/WidgetApi.ts +++ b/src/widgets/WidgetApi.ts @@ -34,6 +34,7 @@ export enum KnownWidgetActions { ReceiveOpenIDCredentials = "openid_credentials", SetAlwaysOnScreen = "set_always_on_screen", ClientReady = "im.vector.ready", + Terminate = "im.vector.terminate", } export type WidgetAction = KnownWidgetActions | string; @@ -68,6 +69,8 @@ export class WidgetApi { private inFlightRequests: { [requestId: string]: (reply: FromWidgetRequest) => void } = {}; private readyPromise: Promise; private readyPromiseResolve: () => void; + private terminatePromise: Promise; + private terminatePromiseResolve: () => void; /** * Set this to true if your widget is expecting a ready message from the client. False otherwise (default). @@ -78,6 +81,7 @@ export class WidgetApi { this.origin = new URL(currentUrl).origin; this.readyPromise = new Promise(resolve => this.readyPromiseResolve = resolve); + this.terminatePromise = new Promise(resolve => this.terminatePromiseResolve = resolve); window.addEventListener("message", event => { if (event.origin !== this.origin) return; // ignore: invalid origin @@ -98,6 +102,12 @@ export class WidgetApi { // Automatically acknowledge so we can move on this.replyToRequest(payload, {}); + } else if (payload.action === KnownWidgetActions.Terminate) { + // Reply after resolving + this.terminatePromise.then(() => { + this.replyToRequest(payload, {}); + }); + this.terminatePromiseResolve(); } else { console.warn(`[WidgetAPI] Got unexpected action: ${payload.action}`); } @@ -116,6 +126,10 @@ export class WidgetApi { return this.readyPromise; } + public addTerminateCallback(action) { + this.terminatePromise = this.terminatePromise.then(action); + } + private replyToRequest(payload: ToWidgetRequest, reply: any) { if (!window.parent) return; From 4fac7810517795061ec6a750b49d46ed31ebf059 Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Sat, 18 Apr 2020 14:53:48 +0300 Subject: [PATCH 2/7] Tell widgets to terminate gracefully in AppTile:_endWidgetActions If the widget fails to terminate in two seconds, proceed with disposing the iframe nevertheless. This allows e.g. Jitsi to hangup the conference when minimizing the widget, instead of abrupt disconnect that can leave ghost users behind. Signed-off-by: Pauli Virtanen --- src/components/views/elements/AppTile.js | 74 ++++++++++++++---------- 1 file changed, 43 insertions(+), 31 deletions(-) diff --git a/src/components/views/elements/AppTile.js b/src/components/views/elements/AppTile.js index 58bfda8d22..5176753037 100644 --- a/src/components/views/elements/AppTile.js +++ b/src/components/views/elements/AppTile.js @@ -340,23 +340,30 @@ export default class AppTile extends React.Component { /** * Ends all widget interaction, such as cancelling calls and disabling webcams. * @private + * @returns {Promise<*>} Resolves when the widget is terminated, or timeout passed. */ _endWidgetActions() { - // HACK: This is a really dirty way to ensure that Jitsi cleans up - // its hold on the webcam. Without this, the widget holds a media - // stream open, even after death. See https://github.com/vector-im/riot-web/issues/7351 - if (this._appFrame.current) { - // In practice we could just do `+= ''` to trick the browser - // into thinking the URL changed, however I can foresee this - // being optimized out by a browser. Instead, we'll just point - // the iframe at a page that is reasonably safe to use in the - // event the iframe doesn't wink away. - // This is relative to where the Riot instance is located. - this._appFrame.current.src = 'about:blank'; - } + const timeout = 2000; + const timeoutPromise = new Promise(resolve => setTimeout(resolve, timeout)); + const messaging = ActiveWidgetStore.getWidgetMessaging(this.props.app.id); - // Delete the widget from the persisted store for good measure. - PersistedElement.destroyElement(this._persistKey); + return Promise.race([messaging.terminate(), timeoutPromise]).finally(() => { + // HACK: This is a really dirty way to ensure that Jitsi cleans up + // its hold on the webcam. Without this, the widget holds a media + // stream open, even after death. See https://github.com/vector-im/riot-web/issues/7351 + if (this._appFrame.current) { + // In practice we could just do `+= ''` to trick the browser + // into thinking the URL changed, however I can foresee this + // being optimized out by a browser. Instead, we'll just point + // the iframe at a page that is reasonably safe to use in the + // event the iframe doesn't wink away. + // This is relative to where the Riot instance is located. + this._appFrame.current.src = 'about:blank'; + } + + // Delete the widget from the persisted store for good measure. + PersistedElement.destroyElement(this._persistKey); + }); } /* If user has permission to modify widgets, delete the widget, @@ -380,21 +387,21 @@ export default class AppTile extends React.Component { } this.setState({deleting: true}); - this._endWidgetActions(); + this._endWidgetActions().then(() => { + WidgetUtils.setRoomWidget( + this.props.room.roomId, + this.props.app.id, + ).catch((e) => { + console.error('Failed to delete widget', e); + const ErrorDialog = sdk.getComponent("dialogs.ErrorDialog"); - WidgetUtils.setRoomWidget( - this.props.room.roomId, - this.props.app.id, - ).catch((e) => { - console.error('Failed to delete widget', e); - const ErrorDialog = sdk.getComponent("dialogs.ErrorDialog"); - - Modal.createTrackedDialog('Failed to remove widget', '', ErrorDialog, { - title: _t('Failed to remove widget'), - description: _t('An error ocurred whilst trying to remove the widget from the room'), + Modal.createTrackedDialog('Failed to remove widget', '', ErrorDialog, { + title: _t('Failed to remove widget'), + description: _t('An error ocurred whilst trying to remove the widget from the room'), + }); + }).finally(() => { + this.setState({deleting: false}); }); - }).finally(() => { - this.setState({deleting: false}); }); }, }); @@ -545,13 +552,18 @@ export default class AppTile extends React.Component { if (this.props.userWidget) { this._onMinimiseClick(); } else { + let promise; if (this.props.show) { // if we were being shown, end the widget as we're about to be minimized. - this._endWidgetActions(); + promise = this._endWidgetActions(); + } else { + promise = Promise.resolve(); } - dis.dispatch({ - action: 'appsDrawer', - show: !this.props.show, + promise.then(() => { + dis.dispatch({ + action: 'appsDrawer', + show: !this.props.show, + }); }); } } From 94745e9407a5a772febf74608b536f5d70797417 Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Sat, 18 Apr 2020 16:57:19 +0300 Subject: [PATCH 3/7] Minimize widget immediately, and end it later Signed-off-by: Pauli Virtanen --- src/components/views/elements/AppTile.js | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/components/views/elements/AppTile.js b/src/components/views/elements/AppTile.js index 5176753037..100a31bdcc 100644 --- a/src/components/views/elements/AppTile.js +++ b/src/components/views/elements/AppTile.js @@ -39,6 +39,7 @@ import SettingsStore, {SettingLevel} from "../../../settings/SettingsStore"; import {aboveLeftOf, ContextMenu, ContextMenuButton} from "../../structures/ContextMenu"; import PersistedElement from "./PersistedElement"; import {WidgetType} from "../../../widgets/WidgetType"; +import {sleep} from "../../../utils/promise"; const ALLOWED_APP_URL_SCHEMES = ['https:', 'http:']; const ENABLE_REACT_PERF = false; @@ -344,10 +345,9 @@ export default class AppTile extends React.Component { */ _endWidgetActions() { const timeout = 2000; - const timeoutPromise = new Promise(resolve => setTimeout(resolve, timeout)); const messaging = ActiveWidgetStore.getWidgetMessaging(this.props.app.id); - return Promise.race([messaging.terminate(), timeoutPromise]).finally(() => { + return Promise.race([messaging.terminate(), sleep(timeout)]).finally(() => { // HACK: This is a really dirty way to ensure that Jitsi cleans up // its hold on the webcam. Without this, the widget holds a media // stream open, even after death. See https://github.com/vector-im/riot-web/issues/7351 @@ -552,18 +552,13 @@ export default class AppTile extends React.Component { if (this.props.userWidget) { this._onMinimiseClick(); } else { - let promise; if (this.props.show) { // if we were being shown, end the widget as we're about to be minimized. - promise = this._endWidgetActions(); - } else { - promise = Promise.resolve(); + this._endWidgetActions(); } - promise.then(() => { - dis.dispatch({ - action: 'appsDrawer', - show: !this.props.show, - }); + dis.dispatch({ + action: 'appsDrawer', + show: !this.props.show, }); } } From 352ea29d1733569ad86e7ffbfb960e4d05bde55d Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Sat, 18 Apr 2020 17:04:56 +0300 Subject: [PATCH 4/7] Implement widget ReceiveTerminate capability Signed-off-by: Pauli Virtanen --- src/components/views/elements/AppTile.js | 14 +++++++++++--- src/utils/WidgetUtils.js | 1 + src/widgets/WidgetApi.ts | 1 + 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/components/views/elements/AppTile.js b/src/components/views/elements/AppTile.js index 100a31bdcc..18be0eeb67 100644 --- a/src/components/views/elements/AppTile.js +++ b/src/components/views/elements/AppTile.js @@ -344,10 +344,18 @@ export default class AppTile extends React.Component { * @returns {Promise<*>} Resolves when the widget is terminated, or timeout passed. */ _endWidgetActions() { - const timeout = 2000; - const messaging = ActiveWidgetStore.getWidgetMessaging(this.props.app.id); + let promise; - return Promise.race([messaging.terminate(), sleep(timeout)]).finally(() => { + if (this._hasCapability('m.receive_terminate')) { + // Wait for widget to terminate within a timeout + const timeout = 2000; + const messaging = ActiveWidgetStore.getWidgetMessaging(this.props.app.id); + promise = Promise.race([messaging.terminate(), sleep(timeout)]); + } else { + promise = Promise.resolve(); + } + + return promise.finally(() => { // HACK: This is a really dirty way to ensure that Jitsi cleans up // its hold on the webcam. Without this, the widget holds a media // stream open, even after death. See https://github.com/vector-im/riot-web/issues/7351 diff --git a/src/utils/WidgetUtils.js b/src/utils/WidgetUtils.js index 6a0556c2b3..11dd5a88f7 100644 --- a/src/utils/WidgetUtils.js +++ b/src/utils/WidgetUtils.js @@ -420,6 +420,7 @@ export default class WidgetUtils { if (WidgetType.JITSI.matches(appType)) { capWhitelist.push(Capability.AlwaysOnScreen); } + capWhitelist.push(Capability.ReceiveTerminate); return capWhitelist; } diff --git a/src/widgets/WidgetApi.ts b/src/widgets/WidgetApi.ts index e08476edb5..6a29955713 100644 --- a/src/widgets/WidgetApi.ts +++ b/src/widgets/WidgetApi.ts @@ -23,6 +23,7 @@ export enum Capability { Screenshot = "m.capability.screenshot", Sticker = "m.sticker", AlwaysOnScreen = "m.always_on_screen", + ReceiveTerminate = "m.receive_terminate", } export enum KnownWidgetActions { From cf4137d4b23e5af5baf3c3a1f056da789e51d8b7 Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Wed, 22 Apr 2020 19:20:28 +0300 Subject: [PATCH 5/7] Make WidgetAPI an EventEmitter + use for terminate + cleanups Use EventEmitter for emitting events, rename terminate event code, plus misc cleanups from review. Signed-off-by: Pauli Virtanen --- src/WidgetMessaging.js | 2 -- src/components/views/elements/AppTile.js | 22 +++++++++--------- src/widgets/WidgetApi.ts | 29 ++++++++++++++---------- 3 files changed, 28 insertions(+), 25 deletions(-) diff --git a/src/WidgetMessaging.js b/src/WidgetMessaging.js index 6a20c340a5..375e7dd8a3 100644 --- a/src/WidgetMessaging.js +++ b/src/WidgetMessaging.js @@ -89,8 +89,6 @@ export default class WidgetMessaging { /** * Tells the widget that it should terminate now. - * It is not necessarily called in all instances before the widget is removed, - * and the client may force termination with a timeout. * @returns {Promise<*>} Resolves when widget has acknowledged the message. */ terminate() { diff --git a/src/components/views/elements/AppTile.js b/src/components/views/elements/AppTile.js index 18be0eeb67..057643c725 100644 --- a/src/components/views/elements/AppTile.js +++ b/src/components/views/elements/AppTile.js @@ -346,7 +346,7 @@ export default class AppTile extends React.Component { _endWidgetActions() { let promise; - if (this._hasCapability('m.receive_terminate')) { + if (this._hasCapability('im.vector.receive_terminate')) { // Wait for widget to terminate within a timeout const timeout = 2000; const messaging = ActiveWidgetStore.getWidgetMessaging(this.props.app.id); @@ -396,20 +396,20 @@ export default class AppTile extends React.Component { this.setState({deleting: true}); this._endWidgetActions().then(() => { - WidgetUtils.setRoomWidget( + return WidgetUtils.setRoomWidget( this.props.room.roomId, this.props.app.id, - ).catch((e) => { - console.error('Failed to delete widget', e); - const ErrorDialog = sdk.getComponent("dialogs.ErrorDialog"); + ); + }).catch((e) => { + console.error('Failed to delete widget', e); + const ErrorDialog = sdk.getComponent("dialogs.ErrorDialog"); - Modal.createTrackedDialog('Failed to remove widget', '', ErrorDialog, { - title: _t('Failed to remove widget'), - description: _t('An error ocurred whilst trying to remove the widget from the room'), - }); - }).finally(() => { - this.setState({deleting: false}); + Modal.createTrackedDialog('Failed to remove widget', '', ErrorDialog, { + title: _t('Failed to remove widget'), + description: _t('An error ocurred whilst trying to remove the widget from the room'), }); + }).finally(() => { + this.setState({deleting: false}); }); }, }); diff --git a/src/widgets/WidgetApi.ts b/src/widgets/WidgetApi.ts index 6a29955713..c5420dca38 100644 --- a/src/widgets/WidgetApi.ts +++ b/src/widgets/WidgetApi.ts @@ -18,12 +18,13 @@ limitations under the License. // https://github.com/turt2live/matrix-dimension/blob/4f92d560266635e5a3c824606215b84e8c0b19f5/web/app/shared/services/scalar/scalar-widget.api.ts import { randomString } from "matrix-js-sdk/src/randomstring"; +import { EventEmitter } from "events"; export enum Capability { Screenshot = "m.capability.screenshot", Sticker = "m.sticker", AlwaysOnScreen = "m.always_on_screen", - ReceiveTerminate = "m.receive_terminate", + ReceiveTerminate = "im.vector.receive_terminate", } export enum KnownWidgetActions { @@ -64,14 +65,17 @@ export interface FromWidgetRequest extends WidgetRequest { /** * Handles Riot <--> Widget interactions for embedded/standalone widgets. + * + * Emitted events: + * - terminate(wait): client requested the widget to terminate. + * Call the argument 'wait(promise)' to postpone the finalization until + * the given promise resolves. */ -export class WidgetApi { +export class WidgetApi extends EventEmitter { private origin: string; private inFlightRequests: { [requestId: string]: (reply: FromWidgetRequest) => void } = {}; private readyPromise: Promise; private readyPromiseResolve: () => void; - private terminatePromise: Promise; - private terminatePromiseResolve: () => void; /** * Set this to true if your widget is expecting a ready message from the client. False otherwise (default). @@ -79,10 +83,11 @@ export class WidgetApi { public expectingExplicitReady = false; constructor(currentUrl: string, private widgetId: string, private requestedCapabilities: string[]) { + super(); + this.origin = new URL(currentUrl).origin; this.readyPromise = new Promise(resolve => this.readyPromiseResolve = resolve); - this.terminatePromise = new Promise(resolve => this.terminatePromiseResolve = resolve); window.addEventListener("message", event => { if (event.origin !== this.origin) return; // ignore: invalid origin @@ -104,11 +109,15 @@ export class WidgetApi { // Automatically acknowledge so we can move on this.replyToRequest(payload, {}); } else if (payload.action === KnownWidgetActions.Terminate) { - // Reply after resolving - this.terminatePromise.then(() => { + // Finalization needs to be async, so postpone with a promise + let finalizePromise = Promise.resolve(); + const wait = promise => { + finalizePromise = finalizePromise.then(value => promise); + } + this.emit('terminate', wait); + Promise.resolve(finalizePromise).then(() => { this.replyToRequest(payload, {}); }); - this.terminatePromiseResolve(); } else { console.warn(`[WidgetAPI] Got unexpected action: ${payload.action}`); } @@ -127,10 +136,6 @@ export class WidgetApi { return this.readyPromise; } - public addTerminateCallback(action) { - this.terminatePromise = this.terminatePromise.then(action); - } - private replyToRequest(payload: ToWidgetRequest, reply: any) { if (!window.parent) return; From 798f5d401ba89c96fd1467afe343a7caba0da5a2 Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Sat, 11 Apr 2020 15:23:32 +0300 Subject: [PATCH 6/7] Ensure active Jitsi conference is closed on widget pop-out Signed-off-by: Pauli Virtanen --- src/components/views/elements/AppTile.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/components/views/elements/AppTile.js b/src/components/views/elements/AppTile.js index 057643c725..f5664fd613 100644 --- a/src/components/views/elements/AppTile.js +++ b/src/components/views/elements/AppTile.js @@ -671,6 +671,17 @@ export default class AppTile extends React.Component { } _onPopoutWidgetClick() { + // Ensure Jitsi conferences are closed on pop-out, to not confuse the user to join them + // twice from the same computer, which Jitsi can have problems with (audio echo/gain-loop). + if (WidgetType.JITSI.matches(this.props.app.type) && this.props.show) { + this._endWidgetActions().then(() => { + if (this._appFrame.current) { + // Reload iframe + this._appFrame.current.src = this._getRenderedUrl(); + this.setState({}); + } + }); + } // Using Object.assign workaround as the following opens in a new window instead of a new tab. // window.open(this._getPopoutUrl(), '_blank', 'noopener=yes'); Object.assign(document.createElement('a'), From 38962560acce8fdb09705eee3590ae5331028eea Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Tue, 28 Apr 2020 02:18:43 +0300 Subject: [PATCH 7/7] Style fixes Signed-off-by: Pauli Virtanen --- src/components/views/elements/AppTile.js | 11 ++++++----- src/widgets/WidgetApi.ts | 5 +++-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/components/views/elements/AppTile.js b/src/components/views/elements/AppTile.js index f5664fd613..ef0075e800 100644 --- a/src/components/views/elements/AppTile.js +++ b/src/components/views/elements/AppTile.js @@ -39,6 +39,7 @@ import SettingsStore, {SettingLevel} from "../../../settings/SettingsStore"; import {aboveLeftOf, ContextMenu, ContextMenuButton} from "../../structures/ContextMenu"; import PersistedElement from "./PersistedElement"; import {WidgetType} from "../../../widgets/WidgetType"; +import {Capability} from "../../../widgets/WidgetApi"; import {sleep} from "../../../utils/promise"; const ALLOWED_APP_URL_SCHEMES = ['https:', 'http:']; @@ -344,18 +345,18 @@ export default class AppTile extends React.Component { * @returns {Promise<*>} Resolves when the widget is terminated, or timeout passed. */ _endWidgetActions() { - let promise; + let terminationPromise; - if (this._hasCapability('im.vector.receive_terminate')) { + if (this._hasCapability(Capability.ReceiveTerminate)) { // Wait for widget to terminate within a timeout const timeout = 2000; const messaging = ActiveWidgetStore.getWidgetMessaging(this.props.app.id); - promise = Promise.race([messaging.terminate(), sleep(timeout)]); + terminationPromise = Promise.race([messaging.terminate(), sleep(timeout)]); } else { - promise = Promise.resolve(); + terminationPromise = Promise.resolve(); } - return promise.finally(() => { + return terminationPromise.finally(() => { // HACK: This is a really dirty way to ensure that Jitsi cleans up // its hold on the webcam. Without this, the widget holds a media // stream open, even after death. See https://github.com/vector-im/riot-web/issues/7351 diff --git a/src/widgets/WidgetApi.ts b/src/widgets/WidgetApi.ts index c5420dca38..795c6648ef 100644 --- a/src/widgets/WidgetApi.ts +++ b/src/widgets/WidgetApi.ts @@ -111,11 +111,12 @@ export class WidgetApi extends EventEmitter { } else if (payload.action === KnownWidgetActions.Terminate) { // Finalization needs to be async, so postpone with a promise let finalizePromise = Promise.resolve(); - const wait = promise => { + const wait = (promise) => { finalizePromise = finalizePromise.then(value => promise); - } + }; this.emit('terminate', wait); Promise.resolve(finalizePromise).then(() => { + // Acknowledge that we're shut down now this.replyToRequest(payload, {}); }); } else {