FIX: Memory Leaks when decorating posts (#7749)

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.)

diff --git a/app/assets/javascripts/discourse/initializers/post-decorations.js.es6 b/app/assets/javascripts/discourse/initializers/post-decorations.js.es6
index e9e6921..a9e1cc2 100644
--- a/app/assets/javascripts/discourse/initializers/post-decorations.js.es6
+++ b/app/assets/javascripts/discourse/initializers/post-decorations.js.es6
@@ -9,25 +9,32 @@ export default {
   initialize(container) {
     withPluginApi("0.1", api => {
       const siteSettings = container.lookup("site-settings:main");
-      api.decorateCooked(highlightSyntax);
-      api.decorateCooked(lightbox);
+      api.decorateCooked(highlightSyntax, {
+        id: "discourse-syntax-highlighting"
+      });
+      api.decorateCooked(lightbox, { id: "discourse-lightbox" });
       if (siteSettings.support_mixed_text_direction) {
-        api.decorateCooked(setTextDirections);
+        api.decorateCooked(setTextDirections, {
+          id: "discourse-text-direction"
+        });
       }
 
       setupLazyLoading(api);
 
-      api.decorateCooked($elem => {
-        const players = $("audio", $elem);
-        if (players.length) {
-          players.on("play", () => {
-            const postId = parseInt($elem.closest("article").data("post-id"));
-            if (postId) {
-              api.preventCloak(postId);
-            }
-          });
-        }
-      });
+      api.decorateCooked(
+        $elem => {
+          const players = $("audio", $elem);
+          if (players.length) {
+            players.on("play", () => {
+              const postId = parseInt($elem.closest("article").data("post-id"));
+              if (postId) {
+                api.preventCloak(postId);
+              }
+            });
+          }
+        },
+        { id: "discourse-audio" }
+      );
     });
   }
 };
diff --git a/app/assets/javascripts/discourse/lib/lazy-load-images.js.es6 b/app/assets/javascripts/discourse/lib/lazy-load-images.js.es6
index 035ccf5..12f78fc 100644
--- a/app/assets/javascripts/discourse/lib/lazy-load-images.js.es6
+++ b/app/assets/javascripts/discourse/lib/lazy-load-images.js.es6
@@ -95,6 +95,6 @@ export function setupLazyLoading(api) {
         }
       });
     },
-    { onlyStream: true }
+    { onlyStream: true, id: "discourse-lazy-load" }
   );
 }
diff --git a/app/assets/javascripts/discourse/lib/plugin-api.js.es6 b/app/assets/javascripts/discourse/lib/plugin-api.js.es6
index 7b682b6..1fa2f44 100644
--- a/app/assets/javascripts/discourse/lib/plugin-api.js.es6
+++ b/app/assets/javascripts/discourse/lib/plugin-api.js.es6
@@ -193,8 +193,14 @@ class PluginApi {
    * 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) {
     opts = opts || {};
@@ -202,12 +208,13 @@ class PluginApi {
     addDecorator(callback);
 
     if (!opts.onlyStream) {
-      decorate(ComposerEditor, "previewRefreshed", callback);
-      decorate(DiscourseBanner, "didInsertElement", callback);
+      decorate(ComposerEditor, "previewRefreshed", callback, opts.id);
+      decorate(DiscourseBanner, "didInsertElement", callback, opts.id);
       decorate(
         this.container.factoryFor("component:user-stream").class,
         "didInsertElement",
-        callback
+        callback,
+        opts.id
       );
     }
   }
@@ -930,7 +937,26 @@ export function withPluginApi(version, apiCodeCallback, opts) {
 }
 
 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 = {};
   mixin["_decorate_" + _decorateId++] = function($elem) {
     $elem = $elem || this.$();
@@ -944,10 +970,3 @@ function decorate(klass, evt, cb) {
 export function resetPluginApi() {
   _pluginv01 = null;
 }
-
-export function decorateCooked() {
-  // eslint-disable-next-line no-console
-  console.warn(
-    "`decorateCooked` has been removed. Use `getPluginApi(version).decorateCooked` instead"
-  );
-}
diff --git a/plugins/discourse-details/assets/javascripts/initializers/apply-details.js.es6 b/plugins/discourse-details/assets/javascripts/initializers/apply-details.js.es6
index 246f88e..78773e8 100644
--- a/plugins/discourse-details/assets/javascripts/initializers/apply-details.js.es6
+++ b/plugins/discourse-details/assets/javascripts/initializers/apply-details.js.es6
@@ -1,7 +1,9 @@
 import { withPluginApi } from "discourse/lib/plugin-api";
 
 function initializeDetails(api) {
-  api.decorateCooked($elem => $("details", $elem).details());
+  api.decorateCooked($elem => $("details", $elem).details(), {
+    id: "discourse-details"
+  });
 
   api.addToolbarPopupMenuOptionsCallback(() => {
     return {
diff --git a/plugins/discourse-local-dates/assets/javascripts/initializers/discourse-local-dates.js.es6 b/plugins/discourse-local-dates/assets/javascripts/initializers/discourse-local-dates.js.es6
index 2811719..352ea11 100644
--- a/plugins/discourse-local-dates/assets/javascripts/initializers/discourse-local-dates.js.es6
+++ b/plugins/discourse-local-dates/assets/javascripts/initializers/discourse-local-dates.js.es6
@@ -2,9 +2,12 @@ import { withPluginApi } from "discourse/lib/plugin-api";
 import showModal from "discourse/lib/show-modal";
 
 function initializeDiscourseLocalDates(api) {
-  api.decorateCooked($elem => {
-    $(".discourse-local-date", $elem).applyLocalDates();
-  });
+  api.decorateCooked(
+    $elem => {
+      $(".discourse-local-date", $elem).applyLocalDates();
+    },
+    { id: "discourse-local-date" }
+  );
 
   api.onToolbarCreate(toolbar => {
     toolbar.addButton({
diff --git a/plugins/lazyYT/assets/javascripts/initializers/lazyYT.js.es6 b/plugins/lazyYT/assets/javascripts/initializers/lazyYT.js.es6
index 2531f66..b34dd94 100644
--- a/plugins/lazyYT/assets/javascripts/initializers/lazyYT.js.es6
+++ b/plugins/lazyYT/assets/javascripts/initializers/lazyYT.js.es6
@@ -4,22 +4,25 @@ export default {
   name: "apply-lazyYT",
   initialize() {
     withPluginApi("0.1", api => {
-      api.decorateCooked($elem => {
-        const iframes = $(".lazyYT", $elem);
-        if (iframes.length === 0) {
-          return;
-        }
+      api.decorateCooked(
+        $elem => {
+          const iframes = $(".lazyYT", $elem);
+          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" }
+      );
     });
   }
 };
diff --git a/plugins/poll/assets/javascripts/initializers/extend-for-poll.js.es6 b/plugins/poll/assets/javascripts/initializers/extend-for-poll.js.es6
index 46a54fd..eec0662 100644

[... diff too long, it was truncated ...]

GitHub sha: c322cccd

2 Likes