angular.js icon indicating copy to clipboard operation
angular.js copied to clipboard

ngSanitize 1.5.0 triggers Internet Explorer memory leak

Open PhilipWallin opened this issue 9 years ago • 26 comments

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:

  • If the IIFE in my simplified script is removed the memory leak wont happen.
  • If "createHTMLDocument" isn't called in an inner function the leak wont happen.
  • If "documentElement" isn't accessed on the newly created HTML document the leak wont happen.
  • If an angular module isn't registred the leak wont happen.
  • When changing any of the conditions, for example using angular-sanitize v1.4.8, memory usage could of course still increase. The difference being that the memory is temporary, will be capped, and will clear if you go to another page. In my example project the leak wont cap, and will remain even if you leave the page.

PhilipWallin avatar Jan 19 '16 15:01 PhilipWallin

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 avatar Jan 19 '16 20:01 Narretz

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 avatar Jan 19 '16 21:01 Narretz

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

Narretz avatar Jan 19 '16 21:01 Narretz

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 :)

PhilipWallin avatar Jan 20 '16 09:01 PhilipWallin

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.

Narretz avatar Jan 20 '16 09:01 Narretz

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.

PhilipWallin avatar Jan 20 '16 10:01 PhilipWallin

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

petebacondarwin avatar Jan 22 '16 11:01 petebacondarwin

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.

PhilipWallin avatar Jan 25 '16 09:01 PhilipWallin

You are correct @PhilipWallin

petebacondarwin avatar Jan 25 '16 10:01 petebacondarwin

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

PhilipWallin avatar Feb 18 '16 14:02 PhilipWallin

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

petebacondarwin avatar Feb 19 '16 11:02 petebacondarwin

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.

PhilipWallin avatar Feb 19 '16 12:02 PhilipWallin

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

sjulescu avatar Sep 26 '16 18:09 sjulescu

@sjulescu - can you provide a running example?

petebacondarwin avatar Oct 03 '16 18:10 petebacondarwin

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.

sjulescu avatar Oct 04 '16 13:10 sjulescu

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

petebacondarwin avatar Oct 04 '16 13:10 petebacondarwin

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.

sjulescu avatar Oct 04 '16 13:10 sjulescu

OK, let me look into that.

petebacondarwin avatar Oct 04 '16 13:10 petebacondarwin

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

Any updates? @petebacondarwin

nathand8 avatar Jan 17 '17 21:01 nathand8

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).

gkalpak avatar Jan 17 '17 22:01 gkalpak

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 avatar Feb 08 '17 06:02 theaspect

Closed PR because seizuring onUnload is a bad idea

theaspect avatar Feb 08 '17 07:02 theaspect

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

larryolsson avatar Mar 07 '17 07:03 larryolsson

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

gkalpak avatar Mar 07 '17 18:03 gkalpak

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

tizub avatar Apr 03 '17 19:04 tizub

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.

gkalpak avatar Apr 11 '17 22:04 gkalpak