From 583d77e943949de003616a4b3e55545ede53bc7c Mon Sep 17 00:00:00 2001
From: David Glasser <glasser@davidglasser.net>
Date: Wed, 29 Apr 2015 00:19:30 -0700
Subject: [PATCH] Fix Blaze memory leak

This leak occured whenever a DOM element with attributes got removed
from the DOM without destroying its containing view.  For example, an
element in an `{{#if}}` whose condition is toggled back and forth; a
single Blaze.View is used for the `{{#if}}` as its condition changes.

The retention chain was as follows:

The element was saved in the `elem` variable of `materializeTag` in
materializer.js.

Among other places, `elem` was saved in the `attrUpdater` in that
function, which is used by the `updateAttributes` function.

This function is passed to `view.autorun`, which registered this
`onViewDestroyed` handler:

   self.onViewDestroyed(function () { locals.c.stop(); });

That callback retains a references to the computation, and thus to the
DOM element.

Before this commit, that onViewDestroyed callback is not removed from
the Blaze.View when the computation is stopped (eg, because the DOM
element is removed from the DOM, triggering
`updaterComputation.stop()`), so in this case the DOM element is leaked.

(This still has a tiny leak in that you end up with lots of nulls
inside `_callbacks.destroyed`; using a better data structure for
callbacks is a game for another day.)

Fixes #4289.
---
 packages/blaze/view.js | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/packages/blaze/view.js b/packages/blaze/view.js
index 94e2757aa4..d867658883 100644
--- a/packages/blaze/view.js
+++ b/packages/blaze/view.js
@@ -123,6 +123,19 @@ Blaze.View.prototype.onViewDestroyed = function (cb) {
   this._callbacks.destroyed = this._callbacks.destroyed || [];
   this._callbacks.destroyed.push(cb);
 };
+Blaze.View.prototype.removeViewDestroyedListener = function (cb) {
+  var destroyed = this._callbacks.destroyed;
+  if (! destroyed)
+    return;
+  var index = _.lastIndexOf(destroyed, cb);
+  if (index !== -1) {
+    // XXX You'd think the right thing to do would be splice, but _fireCallbacks
+    // gets sad if you remove callbacks while iterating over the list.  Should
+    // change this to use callback-hook or EventEmitter or something else that
+    // properly supports removal.
+    destroyed[index] = null;
+  }
+};
 
 /// View#autorun(func)
 ///
@@ -203,7 +216,11 @@ Blaze.View.prototype.autorun = function (f, _inViewScope, displayName) {
     (self.name || 'anonymous') + ':' + (displayName || 'anonymous');
   locals.c = Tracker.autorun(locals.f);
 
-  self.onViewDestroyed(function () { locals.c.stop(); });
+  var stopComputation = function () { locals.c.stop(); };
+  self.onViewDestroyed(stopComputation);
+  locals.c.onStop(function () {
+    self.removeViewDestroyedListener(stopComputation);
+  });
 
   return locals.c;
 };
@@ -267,7 +284,7 @@ Blaze._fireCallbacks = function (view, which) {
     Tracker.nonreactive(function fireCallbacks() {
       var cbs = view._callbacks[which];
       for (var i = 0, N = (cbs && cbs.length); i < N; i++)
-        cbs[i].call(view);
+        cbs[i] && cbs[i].call(view);
     });
   });
 };