Fix memory leak in Fastboot

[davewasmer]

The Ember module here is shared across app instances in a Fastboot environment. That means that each new app instance gets the same Ember reference here. Because we saved a reference to the old onError function, and because the onError function closes over a reference to the fastboot and rollbar services, that means that each time this instance initializer ran it retained a reference to an entire app instance that should have been GC'd.

The new onError retains the old onError, which retains the fastboot & rollbar services, which retain the app instance containers they were instantiated through, which retains basically an entire copy of the app.

Removing the reference to the old onError here stops the leak by breaking that retainer chain.

[Exelord]

Thanks, Dave for pointing this out! :)

There is one issue with this solution. If we remove the old onerror users won't be able to use their own login in onerror. For example, if my app has specified:

Ember.onerror = function(e) {
  if (e.type === "myType") {
    // show notification
  }

  throw e;
}

and I will install this addon, it will overwrite the entire logic. it won't be possible to define your own logic, and it can produce conflicts with other add-ons using this behavior.

Maybe the fix would be to move it to initializers so it will only be defined once globally, not with every instance. What do you think?

[davewasmer]

Yea, I was trying to think of a way to preserve the old behavior without closing over the reference and came up empty, but I didn't think of just changing this to an initializer. That makes much more sense, since Ember.onError is a global, shared-across-app-instances thing.

[Exelord]

Yes... but now we would have think how to get owner in .onerror without appInstance to be able to get the service.

[davewasmer]

Hmm, right. Seems like, because Rollbar requires instantiating their client class (i.e. it's not just a global state thing, you could have multiple rollbar clients), and Ember's onError hook is global, there's a fundamental incompatibility here. Really, seems like Ember's onError hook should be per app instance.

One option might be to create a "global" rollbar instance for fastboot environments.

In other words, if we could avoid looking up the fastboot and rollbar services here, and convert this to an initializer (vs instance initializer), it should be okay, since it won't retain any references to app instance containers & will only run once.

But - we can't lookup the consuming app's Rollbar config without access to a container, which means retained references.

I honestly don't see a way around this without requiring consuming apps that use fastboot to define their own initializer so they can import their Rollbar config. Perhaps we have to settle for that, and disable this initializer in fastboot mode (which, of course, we couldn't check via the service, we'd have to do something like if (!(global || window).FastBoot))

[st-h]

@davewasmer are Browser-Only or Node-Only Initializers an option to address your last point?

[davewasmer]

Yea, we could wrap the initializer in a Fastboot check, but the downside there is that you lose Rollbar reporting in the Fastboot environment. That's what I've done on our fork (had to do something since this leak was crashing our fastboot app every couple hours), but wasn't sure that's what you'd want to do. Happy to PR that change if you're cool with it though.

[sdhull]

🤔 can it be moved from an instance-initializer to an initializer?

[Exelord]

@sdhull unfortunately not :/ But maybe we can save a state on an application object (not appInstance) if the error handler has been already registered to not reregister it? What do you think?
@davewasmer

Or we can use a shoebox to pass the state