angular / components

Component infrastructure and Material Design components for Angular
https://material.angular.io
MIT License
24.33k stars 6.73k forks source link

[Tooltip] Memory leak #8989

Open anthonn opened 6 years ago

anthonn commented 6 years ago

Bug:

matTooltip directive creates detatched DOM trees when used and then disposed.

If using a tooltip in an component, in my case on a normal div element, there will be a detatched dom tree for each time the view with the components including the tooltip is disposed.

After viewing and exiting a view in my application with tooltips (10 times) the following detached DOM trees could be seen:

image (It is a comparison from two snapshots in chrome)

I narrowed the problem down and it seems like the (longpress) event in the tooltip host settings is the problem (line 91).

image

If changing this to any other event eg. keyup:

image

Then this memory leak with detatched DOM trees are removed. I have only tested this workaround by changing in the javascript code (both es5 and es6), but I guess it should be the same if changing in the sourcecode also.

My suggestion is to either fix the longpress event or remove it until a correct longpress functionality could be made, because now the tooltip is not really usefull, not on desktop at least. I unfortunately do not really know how a fix could be done though.

What are the steps to reproduce?

Create a tooltip in a component and then dispose the component. Repeat for more detached DOM trees.

https://stackblitz.com/edit/angular-g2rngj

Which versions of Angular, Material, OS, TypeScript, browsers are affected?

anthonn commented 6 years ago

workaround for desktop - do not import "hammerjs"

After looking into this issue more I found an easy workaround for this issue for desktop applications. If not importing hammerjs to the application the issue with detatched DOM trees is removed for matTooltip. But I guess it is still a problem if the application should run on both mobile and desktop.

I used hammerjs - 2.0.8

SebastianSchenk commented 6 years ago

I'm having the same problem in my project, even though I'm not using material. As far as I can see the problem is how angular is using Hammerjs.

When adding a HostListener to a component with a touch event, angular is calling the buildHammer method, passing in a reference of the component. Now, when the component is destroyed, the Hammerjs instance stays alive with the reference.

So far, I haven't found any indication that angular is taking care of that.

Gil-Epshtain commented 6 years ago

After removing HummerJS the mat-tooltip stop leaking. But how to remove/hide the errors from angular.core about the missing HummerJS files, e.g: Could not find HammerJS. Certain Angular Material components may not work correctly. and Hammer.js is not loaded, can not bind 'longpress' event.

How to tell Angular/Material I don't care about HuummerJS?

SebastianSchenk commented 6 years ago

The issue is also tracked here: angular/angular#22155

You can workaround this issue by overriding the .off function of HammerJs.

 buildHammer(element: HTMLElement): HammerInstance {
    const mc = new this._hammer(element, { domEvents: true, touchAction: 'auto' });

    mc.get('pinch').set({ enable: true });
    mc.get('pinch').recognizeWith(mc.get('pan'));

    mc.get('press').set({ enable: true, time: 500 });

 // START workaround
    mc.off = (function (oldOff) {
      function extendedOff(events: string, handler?: any) {
        const boundOff = oldOff.bind(mc);
        boundOff(events, handler);
        mc.destroy();
      }
 // END workaround

      return extendedOff;
    })(mc.off);

    return mc as HammerInstance;
  }
BaHXeLiSiHg commented 6 years ago

I can not find how to unsubscribe from event in MediaQueryList. Possibly somebody had such problem like in my case? Tried to use solution with HammerJS but it didn't helped.

That's my leak screenshots:

Probably somebody resolved such issue...

I'm using:

"dependencies": {
    "@angular/animations": "6.0.6",
    "@angular/cdk": "6.3.0",
    "@angular/common": "6.0.6",
    "@angular/compiler": "6.0.6",
    "@angular/core": "6.0.6",
    "@angular/forms": "6.0.6",
    "@angular/http": "6.0.6",
    "@angular/material": "6.3.0",
    "@angular/platform-browser": "6.0.6",
    "@angular/platform-browser-dynamic": "6.0.6",
    "@angular/router": "6.0.6",
    "@progress/kendo-angular-buttons": "4.1.1",
    "@progress/kendo-angular-charts": "3.2.1",
    "@progress/kendo-angular-dateinputs": "3.4.2",
    "@progress/kendo-angular-dropdowns": "3.0.2",
    "@progress/kendo-angular-excel-export": "2.1.0",
    "@progress/kendo-angular-grid": "3.5.0",
    "@progress/kendo-angular-inputs": "3.1.3",
    "@progress/kendo-angular-intl": "1.4.1",
    "@progress/kendo-angular-l10n": "1.2.0",
    "@progress/kendo-angular-popup": "2.4.1",
    "@progress/kendo-angular-upload": "4.1.2",
    "@progress/kendo-data-query": "1.3.1",
    "@progress/kendo-drawing": "1.5.6",
    "@progress/kendo-theme-default": "2.54.0",
    "core-js": "2.5.7",
    "crypto-js": "3.1.9-1",
    "jsplumb": "2.4.3",
    "leaflet": "1.2.0",
    "leaflet.markercluster": "1.1.0",
    "lzma": "2.3.2",
    "material-design-icons": "3.0.1",
    "moment": "2.22.2",
    "ng2-dnd": "5.0.2",
    "ng2-responsive": "0.8.4",
    "ngx-markdown": "6.1.0",
    "roboto-fontface": "0.8.0",
    "rxjs": "6.2.1",
    "rxjs-compat": "6.2.1",
    "rxjs-tslint-rules": "4.7.2",
    "signalr-no-jquery": "0.1.8",
    "web-animations-js": "2.3.1",
    "webpack": "4.12.2",
    "zone.js": "0.8.26"
  },
  "devDependencies": {
    "@angular-devkit/build-angular": "0.6.8",
    "@angular-devkit/build-optimizer": "0.6.8",
    "@angular-devkit/core": "0.6.8",
    "@angular/cli": "^6.0.8",
    "@angular/compiler-cli": "6.0.6",
    "@angular/language-service": "6.0.6",
    "@types/hammerjs": "2.0.35",
    "@types/jasmine": "2.8.8",
    "@types/jasminewd2": "2.0.3",
    "@types/leaflet": "1.2.0",
    "@types/leaflet.markercluster": "1.0.1",
    "@types/node": "9.4.0",
    "@types/uglify-js": "2.6.30",
    "codelyzer": "4.3.0",
    "hammerjs": "git+http://git@github.com/hammerjs/hammer.js.git",
    "jasmine-core": "3.1.0",
    "jasmine-spec-reporter": "4.2.1",
    "karma": "2.0.2",
    "karma-chrome-launcher": "2.2.0",
    "karma-cli": "1.0.1",
    "karma-coverage": "1.1.2",
    "karma-coverage-istanbul-reporter": "2.0.0",
    "karma-jasmine": "1.1.2",
    "karma-jasmine-html-reporter": "1.1.0",
    "karma-spec-reporter": "0.0.32",
    "karma-teamcity-reporter": "1.1.0",
    "line-awesome": "github:icons8/line-awesome",
    "protractor": "5.3.2",
    "ts-node": "7.0.0",
    "tslint": "5.10.0",
    "typescript": "2.7.2",
    "uglify-es": "3.3.5"
  }
andreialecu commented 6 years ago

There is a PR to fix this here: https://github.com/angular/angular/pull/22156

Unfortunately it's been overlooked, so maybe someone can ping the Angular team.

BaHXeLiSiHg commented 6 years ago

I've tried this fix with HammerJS, but memory leak still remains.

jo-me commented 5 years ago

I've removed the hammerjs import in in main.ts and the reference in package.json. I can see the warning messages in the dev console, but the memory leak is still there.

core.es5.js:178 Could not find HammerJS. Certain Angular Material components may not work correctly. core.js:3126 The "longpress" event cannot be bound because Hammer.JS is not loaded and no custom loader has been specified.

I'm on Angular 6.1.10 and material 6.4.7. Also tried in combination with material 7.0.4 with the same result.

Is the PR mentioned above only available for Angular 7?

jo-me commented 5 years ago

Hmm.. I updated to Angular 7.0.4 and still see the leak.

Can anyone confirm this?

anthonn commented 5 years ago

I forked my old stackblitz and updated all dependencies so angular and angular material now is 7.0.4 and I could not see the same problem as I saw before.

New fork: https://stackblitz.com/edit/angular-g2rngj-hvgz3n

But maybe there is other issues, but the one I reported about at first seems to be fixed/gone.

jo-me commented 5 years ago

@AnthonNilsson interesting. I've taken the time to adapt a stackblitz thing based on angular/material 7 using a router like in my normal setup: https://stackblitz.com/edit/angular-material-tooltip-memleak

Now, if you switch tabs back and forth between Login and Home tab, you will notice that the HomeComponent will not show up in a heap dump. However, if you trigger the tooltip on the Home tab and then go back to Login, the HomeComponent will still be on the heap. This is a leak to me..

anthonn commented 5 years ago

@jo-me I agree with what you are saying and what is happening with the example you have. I think though this is another problem, not the one I reported about in the first place.

The problem I first had when I opened this issue was that you didn't even have to trigger the tooltip, the component that had the tooltip ended up as a memoryleak anyways. But from what I saw this behavior is gone now. But it looks like there still exist another issue when triggering the tooltip as you showed with you example.

jo-me commented 5 years ago

Thanks, I have created a separate issue, but it almost seems like this is a lost cause if you look at how many of these bugs related to tooltip memory issues have been created already ...

anthonn commented 5 years ago

Ok, I saw that #14247 was closed due to it beeing a dupe of this. I do not think it is a dupe though and instead two seperate problems as I wrote earlier, but I reopen this in order to not having both closed.

LijuanZ commented 5 years ago

I'm on Angular material 7.3.3 without hammerJS and still seeing the leak issue after tooltip is triggered. Does anybody know if there is possible workaround?

lluisnieto commented 5 years ago

Interested. Still happening in Angular 8. I removed hammerjs from package.json, removed node_modules and reinstalled. This message is still coming. Are you sure is related to HammerJS?

GGoek commented 4 years ago

It's still happening in Angular 9 (without HammerJS). Any new ideas?