Repeated $document event handlers during unit tests

[andyrooger]

Problem

A number of ui-bootstrap's services register event handlers on places like document when they are created. These are never removed, and so build up during unit tests, with a duplicate for every unit test that directly or indirectly injects the service.

Cause

Each test a new injector is created, which means that each test that uses one of the offending services creates a new version of the service. Thus each test registers a new version of the event handler but no test ever removes old ones.

Effect

This is not going to be a problem for most production environments unless angular apps are being destroyed and recreated, but it is definitely an issue for almost anyone running unit tests.

As a specific example, $uibModal's document keydown handler keeps enough in its closure to hold onto $rootScope and many of the other modal services. One set of this data is kept around for every test that $uibModal is directly or indirectly injected into. This can cause large memory usage and slow downs after a number of tests.

I could also see these extra mistakely running handlers causing hard to track bugs that only occur in the tests, though I have not confirmed this.

Example

This plunker highlights the problem for $uibModal, but it occurs elsewhere in the library too.

Fix

• Rely on something like this line in ngMock to wipe out events on $document. Would need to add this code to any unit tests that even indirectly refer to ui-bootstrap services.
• Allow cleanup on $rootScope.$destroy(). This isn't currently done by default in ngMock but seems like a good way to clean up. I've submitted a pull request to angular for this.

Not sure about good current solutions, so if anyone has ideas I'm happy to write a pull request.

[icfantv]

I'm on mobile at the moment so I can't look at code right now. I'm fairly certain we have handlers for $destroy events but certainly am not going to swear that we aren't missing some. The tricky thing is that the $destroy handlers aren't going to fire if the component isn't destroyed. But again, we'd have to take a look.

[wesleycho]

This is a good catch - this also adds to memory pressure in tests. They need to be garbage collected in the afterEach for the relevant specs.

[andyrooger]

Thanks for the fast responses guys.

@icfantv Yep. Looking through 0.14.3, I see just the following services being problematic as they can't be destroyed. Everything else looks to be cleaned up fine.

• $uibTooltip -

  bootstrap/src/tooltip/tooltip.js

  Line 74 in 763cfd9

  $document.on('keypress', function(e) {

• $uibModalStack -

  bootstrap/src/modal/modal.js

  Line 351 in 763cfd9

  $document.bind('keydown', function(evt) {

@wesleycho Are you saying this should be the responsibility of user's tests to clean up, rather than the library?

[RobJacobs]

If I'm not mistaken, the intent is to add handlers at a global level in these 2 cases. To avoid re-adding handlers we could add them as non-anonymous functions as the browser will recognize the function has already been registered with the event. e.g.:

$document.on('keypress', function(e) {
  if (e.which === 27) {
    var last = openedTooltips.top();
    if (last) {
      last.value.close();
      openedTooltips.removeTop();
      last = null;
    }
  }
});

becomes:

var keypressHandler = function(e) {
  if (e.which === 27) {
    var last = openedTooltips.top();
    if (last) {
      last.value.close();
      openedTooltips.removeTop();
      last = null;
    }
  }
};

$document.on('keypress', keypressHandler);

[wesleycho]

@RobJacobs that's fine, but what we need is something like

afterEach(function() {
  $document.off('keypress');
});

so that when the module is reinstantiated and the service runs, the old bindings don't remain present, which adds to the memory pressure since the old event listeners are never gc'd, which prevents all of the methods/variables from the old service instantiation from being gc'd properly.

[wesleycho]

Took a closer look at the code - adding as an anonymous function is not enough I think, since it is a new function with each module instantiation in angular, which happens with each test.

I think we need to do the afterEach cleanup on $document.

[andyrooger]

Perfect. Thanks for the fast work all.