From 7696f704b267839f84caf4c26106107effda94ec Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Sat, 22 Feb 2020 23:51:30 +0000 Subject: [PATCH] Fix two big DOM leaks which were locking Chrome solid. pillifyLinks leaked Pill components, which if they contained a BaseAvatar would leak a whole DOM tree retained by the BaseAvatar's onClientSync event listener. This tracks the Pill containers so they can be unmounted via unmountPills. BasicMessageComposer set an event listener on selectionchange in onFocus which leaked if onBlur wasn't called. This removes it in unmount. We've also seen Velociraptor retaining full DOM trees from RRs, which this doesn't address as the leak is probably within Velocity, and the plan is to replace it with CSS animations. Should fix https://github.com/vector-im/riot-web/issues/12417 --- .../views/messages/EditHistoryMessage.js | 6 ++- src/components/views/messages/TextualBody.js | 6 ++- .../views/rooms/BasicMessageComposer.js | 1 + src/utils/pillify.js | 38 +++++++++++++++++-- 4 files changed, 44 insertions(+), 7 deletions(-) diff --git a/src/components/views/messages/EditHistoryMessage.js b/src/components/views/messages/EditHistoryMessage.js index a28f8e27d8..679a8f7471 100644 --- a/src/components/views/messages/EditHistoryMessage.js +++ b/src/components/views/messages/EditHistoryMessage.js @@ -20,7 +20,7 @@ import * as HtmlUtils from '../../../HtmlUtils'; import { editBodyDiffToHtml } from '../../../utils/MessageDiffUtils'; import {formatTime} from '../../../DateUtils'; import {MatrixEvent} from 'matrix-js-sdk'; -import {pillifyLinks} from '../../../utils/pillify'; +import {pillifyLinks, unmountPills} from '../../../utils/pillify'; import { _t } from '../../../languageHandler'; import * as sdk from '../../../index'; import {MatrixClientPeg} from '../../../MatrixClientPeg'; @@ -53,6 +53,7 @@ export default class EditHistoryMessage extends React.PureComponent { this.state = {canRedact, sendStatus: event.getAssociatedStatus()}; this._content = createRef(); + this._pills = []; } _onAssociatedStatusChanged = () => { @@ -81,7 +82,7 @@ export default class EditHistoryMessage extends React.PureComponent { pillifyLinks() { // not present for redacted events if (this._content.current) { - pillifyLinks(this._content.current.children, this.props.mxEvent); + pillifyLinks(this._content.current.children, this.props.mxEvent, this._pills); } } @@ -90,6 +91,7 @@ export default class EditHistoryMessage extends React.PureComponent { } componentWillUnmount() { + unmountPills(this._pills); const event = this.props.mxEvent; if (event.localRedactionEvent()) { event.localRedactionEvent().off("status", this._onAssociatedStatusChanged); diff --git a/src/components/views/messages/TextualBody.js b/src/components/views/messages/TextualBody.js index e2b6bec3f5..d74170919e 100644 --- a/src/components/views/messages/TextualBody.js +++ b/src/components/views/messages/TextualBody.js @@ -30,7 +30,7 @@ import { _t } from '../../../languageHandler'; import * as ContextMenu from '../../structures/ContextMenu'; import SettingsStore from "../../../settings/SettingsStore"; import ReplyThread from "../elements/ReplyThread"; -import {pillifyLinks} from '../../../utils/pillify'; +import {pillifyLinks, unmountPills} from '../../../utils/pillify'; import {IntegrationManagers} from "../../../integrations/IntegrationManagers"; import {isPermalinkHost} from "../../../utils/permalinks/Permalinks"; import {toRightOf} from "../../structures/ContextMenu"; @@ -92,6 +92,7 @@ export default createReactClass({ componentDidMount: function() { this._unmounted = false; + this._pills = []; if (!this.props.editState) { this._applyFormatting(); } @@ -103,7 +104,7 @@ export default createReactClass({ // pillifyLinks BEFORE linkifyElement because plain room/user URLs in the composer // are still sent as plaintext URLs. If these are ever pillified in the composer, // we should be pillify them here by doing the linkifying BEFORE the pillifying. - pillifyLinks([this._content.current], this.props.mxEvent); + pillifyLinks([this._content.current], this.props.mxEvent, this._pills); HtmlUtils.linkifyElement(this._content.current); this.calculateUrlPreview(); @@ -146,6 +147,7 @@ export default createReactClass({ componentWillUnmount: function() { this._unmounted = true; + unmountPills(this._pills); }, shouldComponentUpdate: function(nextProps, nextState) { diff --git a/src/components/views/rooms/BasicMessageComposer.js b/src/components/views/rooms/BasicMessageComposer.js index a2a01f4444..5cc51ee626 100644 --- a/src/components/views/rooms/BasicMessageComposer.js +++ b/src/components/views/rooms/BasicMessageComposer.js @@ -490,6 +490,7 @@ export default class BasicMessageEditor extends React.Component { } componentWillUnmount() { + document.removeEventListener("selectionchange", this._onSelectionChange); this._editorRef.removeEventListener("input", this._onInput, true); this._editorRef.removeEventListener("compositionstart", this._onCompositionStart, true); this._editorRef.removeEventListener("compositionend", this._onCompositionEnd, true); diff --git a/src/utils/pillify.js b/src/utils/pillify.js index 24cc8a3c67..f708ab7770 100644 --- a/src/utils/pillify.js +++ b/src/utils/pillify.js @@ -1,5 +1,5 @@ /* -Copyright 2019 The Matrix.org Foundation C.I.C. +Copyright 2019, 2020 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. @@ -21,7 +21,20 @@ import SettingsStore from "../settings/SettingsStore"; import {PushProcessor} from 'matrix-js-sdk/src/pushprocessor'; import * as sdk from '../index'; -export function pillifyLinks(nodes, mxEvent) { +/** + * Recurses depth-first through a DOM tree, converting matrix.to links + * into pills based on the context of a given room. Returns a list of + * the resulting React nodes so they can be unmounted rather than leaking. + * + * @param {Node[]} nodes - a list of sibling DOM nodes to traverse to try + * to turn into pills. + * @param {MatrixEvent} mxEvent - the matrix event which the DOM nodes are + * part of representing. + * @param {Node[]} pills: an accumulator of the DOM nodes which contain + * React components which have been mounted as part of this. + * The initial caller should pass in an empty array to seed the accumulator. + */ +export function pillifyLinks(nodes, mxEvent, pills) { const room = MatrixClientPeg.get().getRoom(mxEvent.getRoomId()); const shouldShowPillAvatar = SettingsStore.getValue("Pill.shouldShowPillAvatar"); let node = nodes[0]; @@ -45,6 +58,7 @@ export function pillifyLinks(nodes, mxEvent) { ReactDOM.render(pill, pillContainer); node.parentNode.replaceChild(pillContainer, node); + pills.push(pillContainer); // Pills within pills aren't going to go well, so move on pillified = true; @@ -102,6 +116,7 @@ export function pillifyLinks(nodes, mxEvent) { ReactDOM.render(pill, pillContainer); roomNotifTextNode.parentNode.replaceChild(pillContainer, roomNotifTextNode); + pills.push(pillContainer); } // Nothing else to do for a text node (and we don't need to advance // the loop pointer because we did it above) @@ -111,9 +126,26 @@ export function pillifyLinks(nodes, mxEvent) { } if (node.childNodes && node.childNodes.length && !pillified) { - pillifyLinks(node.childNodes, mxEvent); + pillifyLinks(node.childNodes, mxEvent, pills); } node = node.nextSibling; } } + +/** + * Unmount all the pill containers from React created by pillifyLinks. + * + * It's critical to call this after pillifyLinks, otherwise + * Pills will leak, leaking entire DOM trees via the event + * emitter on BaseAvatar as per + * https://github.com/vector-im/riot-web/issues/12417 + * + * @param {Node[]} pills - array of pill containers whose React + * components should be unmounted. + */ +export function unmountPills(pills) { + for (const pillContainer of pills) { + ReactDOM.unmountComponentAtNode(pillContainer); + } +}