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
This commit is contained in:
Matthew Hodgson 2020-02-22 23:51:30 +00:00
parent d088879c12
commit 7696f704b2
4 changed files with 44 additions and 7 deletions

View file

@ -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);

View file

@ -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) {

View file

@ -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);

View file

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