[Bug] Memory Leak of routeRecognizer states in acceptance tests

[step2yeung]

🐞 Describe the Bug

In acceptance tests on ember 3.24 (and latest), routeRecognizer instances are being retained by EmberRouter: (Heap snapshot from super rentals tests, of the states from routeRecognizer being retained)

Once an instance of the routeRecognizer is retained, the number of State in the states array adds up quickly in large apps, causing the browser to crash due to OOM. (In our large ember app, a single routeRecognizer instance stores ~60,000 states)

🔬 Minimal Reproduction

Running an app using ember 3.24+, create an acceptance test.
Using Chrome's Memory tab, run the test and capture a heap dump
Inspect the heap dump for 'State' constructors, notice instances of routeRecognizer class

😕 Actual Behavior

_routerMicrolib is hanging on to references states from routeRecognizer.

🤔 Expected Behavior

References to routeRecognizer should be cleaned up in between tests.

🌍 Environment

Ember: 3.24.4 or higher
Node.js/npm: v14.15.1
OS: Mac
Browser: Chrome

➕ Additional Context

N/A

[step2yeung]

Thinking of a potential fix that looks like adding a destroy() to tildeio/router.js/lib/router/router.ts

  /**
    Clears the current and target route routes and triggers exit
    on each of them starting at the leaf and traversing up through
    its ancestors.
  */
  reset() {
    if (this.state) {
      forEach<InternalRouteInfo<T>>(this.state.routeInfos.slice().reverse(), function (routeInfo) {
        let route = routeInfo.route;
        if (route !== undefined) {
          if (route.exit !== undefined) {
            route.exit();
          }
        }
        return true;
      });
    }

    this.oldState = undefined;
    this.state = new TransitionState();
    this.currentRouteInfos = undefined;
  }

  destroy() {
    this.recognizer = null;
  }

and calling this method in reset() in ember.js/packages/@ember/-internals/routing/lib/system/router.ts

  reset() {
    this._didSetupRouter = false;
    this._initialTransitionStarted = false;
    if (this._routerMicrolib) {
      this._routerMicrolib.reset();
      this._routerMicrolib.destroy();
    }
  }

The fix proposed here is similar to what @hmajoros proposed in #19684

[step2yeung]

Alot of router related objects are still retained, so maybe the fix is more about cleaning up the router vs just routeRecognizer: (from acceptance tests of super-rental)

[step2yeung]

Adding some info shared by @krisselden:

the core issue is statically held objects are being used as keys in a weakmap as seen in the Retainer window

templates used to have be a factory that created an instance and the instance had a factory that created it
when you passed the { instantiate: false } option, we used to not link it to the factory
the container is for stateful things created by a factory that owns it
some refactoring has violated this
3bbc08d#diff-084b63c67dc5de8185211b482%5B%E2%80%A6%5Dcc9e92b9bed270323faba48cea9dR83

I believe that when things were added to the container with instantiate: false
they did not get assigned a back ref to their factory

[richgt]

@step2yeung / @krisselden / @rwjblue - any update on the solution for this?

[chriskrycho]

I believe this may be resolved by #20025; I'll report on the results of my testing of that later today!

---

Later: Unfortunately, I have confirmed that #20025 does not resolve this. It seems to be an unrelated leak. 😩

---

I should elaborate: on reflection, it's impossible that the thing we hit in #20025 could have fixed it; that bug was introduced after 3.24.

[chriskrycho]

A bit of follow-on info: I did some investigation on this today and found that this leak is very old. I chased it back to the version of SuperRentals running Ember 3.3 before I quit chasing, and I suspect it goes much further back than that. @rwjblue and @chadhietala and I all spent some time looking at the actual flow and debugging and found a set of particularly quirky things (using ?seed to get a randomized seed for the order the tests in the suite ran):

• when running the test suite for SuperRentals, at the end of the suite there is always a router instance¹ associated with first and last acceptance tests run, even after a manual GC
• when running only the acceptance tests for SuperRentals, the first and last tests again have a router instance associated with them; and sometimes (but not always) one or more other tests did as well, even after a manual GC

We aren't yet sure why this is the case—especially for the first instance. We'll keep digging as we have time.

Footnotes

1. Specifically, we were checking the app router in this case—but there is also one each for every other Router-related class except EmberRouter, of which there is always, but only, one after the test suite finishes. ↩