Memory leak in instance-initializer when in test mode

[maxfierke]

https://github.com/Exelord/ember-rollbar-client/blob/88c4d35657ae383a56128c00fd7038fba81cc4d4/addon/instance-initializers/rollbar.js#L6-L14

I was investigating some memory leaks in our test suite and found that the use of services within Ember.onerror and recursive wrapping of Ember.onerror was resulting in application containers being retained in memory across test runs, even after they'd been marked destroyed.

Since the Rollbar instance-initializer is called for every new instance of the application, and there are many instances of the application across a test suite run, ember-rollbar-client was recursively wrapping it's own Ember.onerror, with each closure containing a references to the rollbar and fastboot services across multiple containers and preventing those containers from being GC'd. With thousands of tests, this results in a large amount of app containers being retained in memory.

We were able to workaround it by overriding the initializer in our app to avoid recursive wrapping of Ember.onerror and avoiding wrapping of Ember.onerror unless Rollbar is enabled.

import Ember from 'ember';

// This replaces the instance-initializer from ember-rollbar-client, which causes
// memory leaks due to its retaining of appInstance container inside Ember.onerror
export function initialize(appInstance) {
  const fastbootService = appInstance.lookup('service:fastboot');
  const rollbarService = appInstance.lookup('service:rollbar');
  const oldOnError = Ember.onerror;
  const enabled = rollbarService && rollbarService.get('enabled');
  const isFastBoot = fastbootService && fastbootService.get('isFastBoot');

  if (enabled) {
    // This still has the potential to hold onto a container too long,
    // but we just don't enable rollbar in test mode.
    Ember.onerror = (...args) => {
      oldOnError(...args);

      rollbarService.error(...args);

      if (Ember.testing && !isFastBoot) {
        throw args[0];
      }
    };
  }
}

export default { initialize };

I'd be happy to investigate this further and open a PR, but wanted to open the discussion around whether runtime changing of enabled is something that's supported, and whether there are any different ideas on fixing this. Our workaround also bulldozes over any prior definition of Ember.onerror, so that might not be tenable for some applications.

[Exelord]

Hmm... looks like we should resign of initializers and move the logic somewhere else.

Thanks for reporting this! Related to #47