Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible memory leak when using {{#if}} helper with a reactive variable #4289

Closed
xamfoo opened this issue Apr 28, 2015 · 6 comments
Closed

Comments

@xamfoo
Copy link

xamfoo commented Apr 28, 2015

If I use the {{#if}} helper like this

<template name="list">
  <ul>
    {{#each items}}
      {{#if show}}
        <li class="item">item</li>
      {{/if}}
    {{/each}}
  </ul>
</template>

with show being a reactive variable,

Template.list.helpers({
  items: function () { return Items; },

  show: function () { return Session.get('show'); }
});

changing show seems to cause a memory leak in the browser.

Session.set('show', false);
// wait
Session.set('show', true);

In my testing, doing this repeatedly can increase memory use from 29.3mb to 588mb. There is also a corresponding increase in the number of nodes. (See screenshot of timeline profiler)

2015-04-28-184755_1365x244_scrot

Test environment:

  • Meteor 1.1.0.2
  • Chromium Version 41.0.2272.118 (64-bit) on a Linux box
  • Developer tools > Timeline > Click record > GC > Toggle reactive variable multiple times > GC > Click stop

Repo used to produce issue: https://github.com/xamfoo/meteor-issue

@eshell
Copy link

eshell commented Apr 28, 2015

what if you wrap {{#if}} around the {{#each}} instead? If you wan't to show or hide all the items individually, you should put the {{if}} first so {{each}} isn't being executed when it doesn't have to be.

<template name="list">
{{#if show}}
  <ul>
    {{#each items}}
        <li class="item">item</li>
    {{/each}}
  </ul>
{{/if}}
</template>

the way your doing it now makes it so even if you don't wanna show items, it still iterates through every item in {{#each}}.

@xamfoo
Copy link
Author

xamfoo commented Apr 28, 2015

I don't think the issue comes from {{#each}} because when I tried removing {{#each}} memory usage still increases.

If you wan't to show or hide all the items individually, you should put the {{if}} first so {{each}} isn't being executed when it doesn't have to be.

I agree. However, we may still want to put the {{#if}} helper inside {{#each}} when

  • Checking if the iterated item is inside a white list
  • Temporarily hiding some items without regenerating the whole DOM

@glasser
Copy link
Contributor

glasser commented Apr 29, 2015

I ran your reproduction and yes, it absolutely looks like there's a leak there. Another way to see is to (after GCing) take a heap snapshot; I saw a number of HTMLLIElements which was a large multiple of 1000 (and which went up as I clicked more times).

This seems pretty serious!

I'm about to head out for the day; can you try running this on older versions of Meteor to see when this was introduced, if it didn't always exist? Note that before 1.0.4 you can't use onCreated (but the body of that block could just be at top level).

@xamfoo
Copy link
Author

xamfoo commented Apr 29, 2015

can you try running this on older versions of Meteor to see when this was introduced, if it didn't always exist?

This issue seems to exist in Meteor version >= 0.8.3

Seems ok - 0.8.0, 0.8.1, 0.8.2
Issue exists - 0.8.3, 0.9, 1.0.4.1, 1.1.0.2

The code in the repo has been updated to run on older Meteor versions. I ran it like this:

meteor run --release 0.8.3

@glasser
Copy link
Contributor

glasser commented Apr 29, 2015

Thanks for the great reproduction. I tracked down this particular leak and I think it's fixed.

@xamfoo
Copy link
Author

xamfoo commented Apr 29, 2015

That was fast. Thanks!

martijnwalraven pushed a commit that referenced this issue Feb 22, 2016
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants