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

Commit

Permalink
Fix two big DOM leaks which were locking Chrome solid.
Browse files Browse the repository at this point in the history
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 element-hq/element-web#12417
  • Loading branch information
ara4n committed Feb 22, 2020
1 parent d088879 commit 7696f70
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 7 deletions.
6 changes: 4 additions & 2 deletions src/components/views/messages/EditHistoryMessage.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -53,6 +53,7 @@ export default class EditHistoryMessage extends React.PureComponent {
this.state = {canRedact, sendStatus: event.getAssociatedStatus()};

this._content = createRef();
this._pills = [];
}

_onAssociatedStatusChanged = () => {
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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);
Expand Down
6 changes: 4 additions & 2 deletions src/components/views/messages/TextualBody.js
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -92,6 +92,7 @@ export default createReactClass({

componentDidMount: function() {
this._unmounted = false;
this._pills = [];
if (!this.props.editState) {
this._applyFormatting();
}
Expand All @@ -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();

Expand Down Expand Up @@ -146,6 +147,7 @@ export default createReactClass({

componentWillUnmount: function() {
this._unmounted = true;
unmountPills(this._pills);
},

shouldComponentUpdate: function(nextProps, nextState) {
Expand Down
1 change: 1 addition & 0 deletions src/components/views/rooms/BasicMessageComposer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
38 changes: 35 additions & 3 deletions src/utils/pillify.js
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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];
Expand All @@ -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;

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

0 comments on commit 7696f70

Please sign in to comment.