angular / angular.js

AngularJS - HTML enhanced for web apps!
https://angularjs.org
MIT License
58.82k stars 27.5k forks source link

ngSanitize 1.5.0 triggers Internet Explorer memory leak #13800

Open PhilipWallin opened 8 years ago

PhilipWallin commented 8 years ago

The changes from v1.4.8 to v1.5.0 of angular-sanitize triggers a memory leak in our project. When using Internet Explorer 11 with other javascript libraries each pageload will increase the memory usage, until the browser stop working. I've put together an example project which just have the minimum amounts of script tags in the header to reproduce the problem. Every time I refresh the page Internet Explorer 11 will leak memory, and will eventually (at around 1.2GB memory usage for me) get random renderer and javascript errors. If I remove either of "es6-shim.js", "kendo.all.min.js", or "angular-sanitize.js" the memory leak wont happen. In the example project I have simplified the angular-sanitize code as much as I could to show what functionality is triggering the memory leak for me.

(As the file attachment seems to be broken, here is a link to the example project: https://dl.dropboxusercontent.com/u/775659/Leaker.zip)

Notes:

Narretz commented 8 years ago

Thank for the detailed write-up. I can confirm that there's a memory leak, and that the steps to prevent it work. I find removing the IIFE especially interesting. I've also experienced that the leak isn't as bad if you simply remove the createThingy function and create the doc straight like this var thingy = window.document.implementation.createHTMLDocument("doccy") So it looks like IE11 doesn't like it when this is inside a function. I wonder, how big of an increase do you see per page load? For me it was around 20MB in your initial example, and with the fn removed around 10MB. Maybe we can remove the IIFE in the sanitize code, too. However, I think there's always one outer IIFE around the whole sanitize module, so we might not get it that way.

You should definitely open a bug report about this at MSDN Connect. They don't have a great track record with responses, but it's better than nothing.

Narretz commented 8 years ago

The only reliable solution I've found is indeed to remove all IIFE from the angular sanitize code (there are 2). But we can't remove the outer one, because the keeps all our stuff private.

Narretz commented 8 years ago

Another thing, are you using Kendo Professional or Open Source Edition?

PhilipWallin commented 8 years ago

In my tests the memory growth is around 25-30 MB each refresh. I'm using Kendo Professional in the project.

In my simplified script example I can add the whole inner IIFE to trigger the leak, and add just the code of the inner IIFE to avoid the leak. This, however, does not seem to work as easily on the full angular-sanitize.

I will report this to Microsoft too, but I figured that was... more of a long term solution :)

Narretz commented 8 years ago

The thing is that the trigger isn't that common (since you need both kendo ui and ES6 Shim), so I'd like to refrain from trying some brute force approach in ngSanitize. It would be interesting to find out what exactly triggers it in ES6 Shim / Kendo. Problem is that they their distributables are huge.

PhilipWallin commented 8 years ago

I figured as much, which is why it's really annoying finding a bug which is triggered by three different libraries. I considered binary searching for triggers in the other projects too, but deemed it to not be worth the time, as even if I found that X + Y + Z triggers a problem it would in the end still be an IE bug. In best case I could prove it's a common scenario, which would make at least one of the libraries want to avoid it.

petebacondarwin commented 8 years ago

I don't think that this issue should block release of 1.5.0 - I am moving it to the 1.5.x milestone.

PhilipWallin commented 8 years ago

There shouldn't be any problem running older version of sanitize (1.4.8) with newer Angular (1.5.0), right? I guess this could change in the future, but I need a short term solution for now.

petebacondarwin commented 8 years ago

You are correct @PhilipWallin

PhilipWallin commented 8 years ago

From what I understand this issue is caused by circular references in the Javascript, maybe circular reference through closures? https://msdn.microsoft.com/en-us/library/bb250448(v=vs.85).aspx

petebacondarwin commented 8 years ago

That is often the cause of memory leaks but the trick is how to find those circular references, when you have three libraries involved!

PhilipWallin commented 8 years ago

I cannot really say I completely understand the problem, but it might be useful for you to know that downgrading Sanitize version didn't really solve my problem, I only thought it did. It did solve the problem where pressing F5 would retain memory, but moving between pages still do increase memory consumption until crash. For you it might be good to know that in some cases the current code structure could cause this behavior in IE11. I've kind of given up on the problem due to its complexity.

sjulescu commented 8 years ago

I’m facing the same situation. Any proposed solution or workaround available? Thanks

petebacondarwin commented 8 years ago

@sjulescu - can you provide a running example?

sjulescu commented 8 years ago

We have a large application, so providing an example is more complicated. I can provide the result of our analyses and please let me know if the example is still necessary. We are using AngularJS v1.5.6 and the leak is reproducible only on IE11.

So, we have a large application with several pages, on each page we are loading more than 50 scripts. We've seen that loading a page is causing huge memory leaks. After analyzing we realized that the combination of: angular-sanitize.js and jquery.i18n.properties-1.0.9.js are causing the leaks. Going further with the analyze for angular-sanitize we identified that the declaration of var inertBodyElement as global variable is causing the leak. To be 100% sure we temporary added in the angular-sanitize a function to nullify the inertBodyElement that we call later from our code when navigating from the page. Something like: window.cleanInertBodyElement = function() { inertBodyElement = null; } The result was that the memory leak is gone.

A similar thing we identified in the jquery.i18n.properties-1.0.9 with a global variable: cbSplit.

petebacondarwin commented 8 years ago

Are you saying that the leak only occurs if "both" inertBodyElement and cbSplit are in the global scope?

sjulescu commented 8 years ago

At the beginning we were thinking that the combination is causing the leak, but the combination is just amplifying it. We have the leak using only angular-sanitize or only jquery.i18n.properties-1.0.9.

petebacondarwin commented 8 years ago

OK, let me look into that.

nathand8 commented 7 years ago

Also having this problem. It's beginning to affect users to where they have to close IE 11 every hour.

Any updates? @petebacondarwin

gkalpak commented 7 years ago

Does anyone have a reproduction using just angular-sanitize (which @sjulescu mentioned is enough to cause the leak)? Using @PhilipWallin's .zip (from https://github.com/angular/angular.js/issues/13800#issue-127477784), I can only see the 25MB-30MB increase in total memory even without angular or angular-sanitize. It seems to be mostly caused by kendo-ui (although es6-shim amplifies it a bit).

theaspect commented 7 years ago

Solution by @sjulescu adding this to angular-sanitize.js 1.5.8 seems fix an issue

window.cleanInertBodyElement = function() {
    inertBodyElement = null;
};

window.onunload = function() {
    cleanInertBodyElement();
}

Also in onUnload event iterate all objects in windows and document scope and manually delete it

theaspect commented 7 years ago

Closed PR because seizuring onUnload is a bad idea

larryolsson commented 7 years ago

Hi, we are also experiencing this problem. Again it is difficult to produce an example since the app is large and complex. Any updates?

gkalpak commented 7 years ago

We can't do much without an actual reproduction (without 3rd-party dependencies).

tizub commented 7 years ago

Hi. The problem seems to be present only when there is a polyfill on the page (ES6 in my case), along with angular-sanitize. I tried with many different polyfill implementations I could find, all of them triggered the issue: core-js, es6-shim, etc. As Angular 2 requires one (core-js suggested by the docs), we can't have an Angular 1 app in 1.5.x and an Angular 2 app in the same page without having this issue. In a way, this might not be considered as "using third party libraries"... Attached is a minimal example app to illustrate this bug. Hope it helps! ie-memory-leak.zip

gkalpak commented 7 years ago

Not sure how using a 3rd party lib can be considered "not using a 3rd party lib". It is not an issue of terminology. The problem with issues appearing only in the presense of 3rd party libs is that the problem is likely to be in that lib ang not AngularJS, which makes is very difficult for us to debug and identify.