FIX: Memory Leaks when decorating posts (#7749)

* Remove long-deprecated method

* FIX: Memory Leaks when decorating posts

Previously we'd keep creating mixins dynamically when decorating the
same class.

This code changes the API to recommend an `id` parameter for each
decorator which will avoid leaks. All plugins should be updated to
include this parameter, although if they don't in the meantime it'll
just mean a warning in the console (and a continued leak.)
This commit is contained in:
Robin Ward 2019-06-11 11:21:23 -04:00 committed by Joffrey JAFFEUX
parent 934adb14d2
commit c322cccd53
7 changed files with 81 additions and 47 deletions

View File

@ -9,25 +9,32 @@ export default {
initialize(container) { initialize(container) {
withPluginApi("0.1", api => { withPluginApi("0.1", api => {
const siteSettings = container.lookup("site-settings:main"); const siteSettings = container.lookup("site-settings:main");
api.decorateCooked(highlightSyntax); api.decorateCooked(highlightSyntax, {
api.decorateCooked(lightbox); id: "discourse-syntax-highlighting"
});
api.decorateCooked(lightbox, { id: "discourse-lightbox" });
if (siteSettings.support_mixed_text_direction) { if (siteSettings.support_mixed_text_direction) {
api.decorateCooked(setTextDirections); api.decorateCooked(setTextDirections, {
id: "discourse-text-direction"
});
} }
setupLazyLoading(api); setupLazyLoading(api);
api.decorateCooked($elem => { api.decorateCooked(
const players = $("audio", $elem); $elem => {
if (players.length) { const players = $("audio", $elem);
players.on("play", () => { if (players.length) {
const postId = parseInt($elem.closest("article").data("post-id")); players.on("play", () => {
if (postId) { const postId = parseInt($elem.closest("article").data("post-id"));
api.preventCloak(postId); if (postId) {
} api.preventCloak(postId);
}); }
} });
}); }
},
{ id: "discourse-audio" }
);
}); });
} }
}; };

View File

@ -95,6 +95,6 @@ export function setupLazyLoading(api) {
} }
}); });
}, },
{ onlyStream: true } { onlyStream: true, id: "discourse-lazy-load" }
); );
} }

View File

@ -193,8 +193,14 @@ class PluginApi {
* For example, to add a yellow background to all posts you could do this: * For example, to add a yellow background to all posts you could do this:
* *
* ``` * ```
* api.decorateCooked($elem => $elem.css({ backgroundColor: 'yellow' })); * api.decorateCooked(
* $elem => $elem.css({ backgroundColor: 'yellow' }),
* { id: 'yellow-decorator' }
* );
* ``` * ```
*
* NOTE: To avoid memory leaks, it is highly recommended to pass a unique `id` parameter.
* You will receive a warning if you do not.
**/ **/
decorateCooked(callback, opts) { decorateCooked(callback, opts) {
opts = opts || {}; opts = opts || {};
@ -202,12 +208,13 @@ class PluginApi {
addDecorator(callback); addDecorator(callback);
if (!opts.onlyStream) { if (!opts.onlyStream) {
decorate(ComposerEditor, "previewRefreshed", callback); decorate(ComposerEditor, "previewRefreshed", callback, opts.id);
decorate(DiscourseBanner, "didInsertElement", callback); decorate(DiscourseBanner, "didInsertElement", callback, opts.id);
decorate( decorate(
this.container.factoryFor("component:user-stream").class, this.container.factoryFor("component:user-stream").class,
"didInsertElement", "didInsertElement",
callback callback,
opts.id
); );
} }
} }
@ -930,7 +937,26 @@ export function withPluginApi(version, apiCodeCallback, opts) {
} }
let _decorateId = 0; let _decorateId = 0;
function decorate(klass, evt, cb) { let _decorated = new WeakMap();
function decorate(klass, evt, cb, id) {
if (!id) {
// eslint-disable-next-line no-console
console.warn(
"`decorateCooked` should be supplied with an `id` option to avoid memory leaks."
);
} else {
if (!_decorated.has(klass)) {
_decorated.set(klass, new Set());
}
id = `${id}:${evt}`;
let set = _decorated.get(klass);
if (set.has(id)) {
return;
}
set.add(id);
}
const mixin = {}; const mixin = {};
mixin["_decorate_" + _decorateId++] = function($elem) { mixin["_decorate_" + _decorateId++] = function($elem) {
$elem = $elem || this.$(); $elem = $elem || this.$();
@ -944,10 +970,3 @@ function decorate(klass, evt, cb) {
export function resetPluginApi() { export function resetPluginApi() {
_pluginv01 = null; _pluginv01 = null;
} }
export function decorateCooked() {
// eslint-disable-next-line no-console
console.warn(
"`decorateCooked` has been removed. Use `getPluginApi(version).decorateCooked` instead"
);
}

View File

@ -1,7 +1,9 @@
import { withPluginApi } from "discourse/lib/plugin-api"; import { withPluginApi } from "discourse/lib/plugin-api";
function initializeDetails(api) { function initializeDetails(api) {
api.decorateCooked($elem => $("details", $elem).details()); api.decorateCooked($elem => $("details", $elem).details(), {
id: "discourse-details"
});
api.addToolbarPopupMenuOptionsCallback(() => { api.addToolbarPopupMenuOptionsCallback(() => {
return { return {

View File

@ -2,9 +2,12 @@ import { withPluginApi } from "discourse/lib/plugin-api";
import showModal from "discourse/lib/show-modal"; import showModal from "discourse/lib/show-modal";
function initializeDiscourseLocalDates(api) { function initializeDiscourseLocalDates(api) {
api.decorateCooked($elem => { api.decorateCooked(
$(".discourse-local-date", $elem).applyLocalDates(); $elem => {
}); $(".discourse-local-date", $elem).applyLocalDates();
},
{ id: "discourse-local-date" }
);
api.onToolbarCreate(toolbar => { api.onToolbarCreate(toolbar => {
toolbar.addButton({ toolbar.addButton({

View File

@ -4,22 +4,25 @@ export default {
name: "apply-lazyYT", name: "apply-lazyYT",
initialize() { initialize() {
withPluginApi("0.1", api => { withPluginApi("0.1", api => {
api.decorateCooked($elem => { api.decorateCooked(
const iframes = $(".lazyYT", $elem); $elem => {
if (iframes.length === 0) { const iframes = $(".lazyYT", $elem);
return; if (iframes.length === 0) {
} return;
$(".lazyYT", $elem).lazyYT({
onPlay(e, $el) {
// don't cloak posts that have playing videos in them
const postId = parseInt($el.closest("article").data("post-id"));
if (postId) {
api.preventCloak(postId);
}
} }
});
}); $(".lazyYT", $elem).lazyYT({
onPlay(e, $el) {
// don't cloak posts that have playing videos in them
const postId = parseInt($el.closest("article").data("post-id"));
if (postId) {
api.preventCloak(postId);
}
}
});
},
{ id: "discourse-lazyyt" }
);
}); });
} }
}; };

View File

@ -104,7 +104,7 @@ function initializePolls(api) {
} }
api.includePostAttributes("polls", "polls_votes"); api.includePostAttributes("polls", "polls_votes");
api.decorateCooked(attachPolls, { onlyStream: true }); api.decorateCooked(attachPolls, { onlyStream: true, id: "discourse-poll" });
api.cleanupStream(cleanUpPolls); api.cleanupStream(cleanUpPolls);
} }