Material: tooltip: memory leak on destroy

Created on 22 Feb 2018 · 7Comments · Source: angular/material

Bug, feature request, or proposal:

commit #11087 introduced a memory leak in md-tooltip

What is the expected behavior?

no memory leak

What is the current behavior?

memory leak in md-tooltip

CodePen and steps to reproduce the issue:

CodePen Demo which shows your issue:

https://codepen.io/pbininda/pen/vdrZZX

Detailed Reproduction Steps:

Step 1: No memory leaked

load the CodePen in Chrome and open chrome dev tools
Hit the "TOGGLE BUTTON" 10 times (making the ng-if shown true 5 times)
In the dev tools take a memory snapshot and search for "Canary"
No living instances of "Canary" are found.

Step 2: Trigger memory leak

load the CodePen in Chrome and open chrome dev tools
Hit the "TOGGLE BUTTON" 10 times (making the ng-if shown true 5 times)
- Each time when the "BUTTON WITH TOOLTIP" is shown, hover over the "BUTTON WITH TOOLTIP" to show the "tooltip"
In the dev tools take a memory snapshot and search for "Canary"
5 live instances of "Canary" are shown.

What is the use-case or motivation for changing an existing behavior?

We have a big application using angular-material. This memory leak keeps a huge chain of objects alive whenever a user hovers over a button.

Which versions of AngularJS, Material, OS, and browsers are affected?

Material version 1.1.7

Is there anything else we should know? Stack Traces, Screenshots, etc.

Reverting the commit #11087 fixes the problem.

Below is a screenshot of the heap snapshot showing 5 leaked instances of "Canary"

urgent Pull Request fixed memory leak regression bug

Source

pbininda

All 7 comments

Are you sure it was the linked commit? - I do not see anything which could lead to memory leak.
But maybe we should take a look at #10651
Edit: but also there it does not look like something which could introduce missing memory cleanup

IMM0rtalis on 22 Feb 2018

Looking at the retainers chain (see screenshot), it seems the onDomAdded callback function that is registered by the tooltip, keeps the $scope object alive through the closure.
Anyway, if I undo the changes of the linked commit, the leak goes away.

pbininda on 22 Feb 2018

Ok, I can now reproduce the problem, but the problem seems like not the tooltip itself. If I replace the onDomAdded-function with the following i have still the leak.

onDomAdded: function() {
            var i = 1 + 3;
          }

Update of research: ANY function passed inside the config leads to this problem.

IMM0rtalis on 22 Feb 2018

👍1

I still think the linked commit is not the problem!
The last two hours I tracked the problem down and I found some problems which just got visible whenever the config has ANY closures.

1. The config together with scope is cached in the trackedPanels-object with the following two lines:

  var panelRef = new MdPanelRef(this._config, this._$injector);
  this._trackedPanels[config.id] = panelRef;

There is some missbehavior with the trackedPanels and config anyway and I think the trackedPanels should handle the scope somehow. For me it is linked to #10894

The config for the panel is stored as object-property but I do not see any reason why. At least it should free the config-values for the garbage-collector since it has the scope linked there.

delete this._config;

So in the end for now the fastest fix without loosing any at the moment working functionality is the following create method (just the changes described above changed):

MdPanelService.prototype.create = function(preset, config) {
  if (typeof preset === 'string') {
    preset = this._getPresetByName(preset);
  } else if (typeof preset === 'object' &&
      (angular.isUndefined(config) || !config)) {
    config = preset;
    preset = {};
  }

  preset = preset || {};
  config = config || {};

  // If the passed-in config contains an ID and the ID is within _trackedPanels,
  // return the tracked panel after updating its config with the passed-in
  // config.
  if (angular.isDefined(config.id) && this._trackedPanels[config.id]) {
    var trackedPanel = this._trackedPanels[config.id];
    angular.extend(trackedPanel.config, config);
    return trackedPanel;
  }

  // Combine the passed-in config, the _defaultConfigOptions, and the preset
  // configuration into the `_config`.
  this._config = angular.extend({
    // If no ID is set within the passed-in config, then create an arbitrary ID.
    id: config.id || 'panel_' + this._$mdUtil.nextUid(),
    scope: this._$rootScope.$new(true),
    attachTo: this._$rootElement
  }, this._defaultConfigOptions, config, preset);

  // Create the panelRef and add it to the `_trackedPanels` object.
  var panelRef = new MdPanelRef(this._config, this._$injector);
  // removing the following line fixes the multiple memory leak of the scope
  this._trackedPanels[config.id] = panelRef;

  // Add the panel to each of its requested groups.
  if (this._config.groupName) {
    if (angular.isString(this._config.groupName)) {
      this._config.groupName = [this._config.groupName];
    }
    angular.forEach(this._config.groupName, function(group) {
      panelRef.addToGroup(group);
    });
  }

  this._config.scope.$on('$destroy', angular.bind(panelRef, panelRef.detach));

 // adding the following line fixes the scope in the memory even if the tooltip is not there anymore
  delete this._config;

  return panelRef;
};

PS: I do not have time until the 5.3.2018 to fix the complete trackedPanels-messup which would fix this as well.

IMM0rtalis on 22 Feb 2018

👍1

Can someone please test https://github.com/angular/material/pull/11145 to see if it solves this leak for you?