Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DOM Nodes Leaking #545

Closed
blowsie opened this issue Oct 31, 2013 · 102 comments
Closed

DOM Nodes Leaking #545

blowsie opened this issue Oct 31, 2013 · 102 comments
Assignees

Comments

@blowsie
Copy link

blowsie commented Oct 31, 2013

I noticed that when using ui.router and click back and forth between pages the DOM Node Count continues to go up and up.

Tested on the ui.router sample app.
http://angular-ui.github.io/ui-router/sample/#/

image

I'm trying to work out what is causing the leak, but haven't found it yet.

Any ideas?

@nateabele
Copy link
Contributor

That's pretty bizarre. I just tried navigating around the sample app and then going backwards and forwards through history (using the keyboard, not the nav buttons... not that that should make a difference). For me, the memory climbs up to around 15mb, then drops to ~7mb and stays there (automatically, I didn't force GC or anything). DOM nodes and event listeners also flatline at 249 and 22, respectively.

@nateabele
Copy link
Contributor

I wonder if you could maybe reproduce your steps with code? Like, copy the sample app into a Plunkr and use $state.transitionTo() and the history API to simulate what you're doing manually, then see if the effect is the same. Then we can try it in our browsers as well to see if the effects are reproducible.

@blowsie
Copy link
Author

blowsie commented Oct 31, 2013

Is the sample app on plunkr for me to fork?

@timkindberg
Copy link
Contributor

It is not :(

@blowsie
Copy link
Author

blowsie commented Oct 31, 2013

Created this test page, but this time had no memory issues.
http://embed.plnkr.co/eVQDn9/preview

I will have to create a copy of the sample application to demonstrate this.

@blowsie
Copy link
Author

blowsie commented Oct 31, 2013

In the meantime, if you throw this into the console it should reproduce the leak

http://angular-ui.github.io/ui-router/sample/#/about

var LeakTest = function() {
var self = this;
      self.leak = null;
     self.start = function() {
        self.leak = setInterval(function() {

          setTimeout(function() {
          $("[ui-sref='contacts.list']").click()
          }, 0);
          setTimeout(function() {
             $("[ui-sref='about']").click()
          }, 300);

        }, 600);
      };
      self.stop = function() {
        clearInterval(self.leak);
      };
    };
var test = new LeakTest();
test.start()

image

@blowsie
Copy link
Author

blowsie commented Oct 31, 2013

Perhaps you could test using the code above for now?

@nateabele
Copy link
Contributor

Thanks for putting that together. Yeah, I'm sort of seeing it, though I don't get anywhere near the same growth curve. After letting it run for around 3 minutes, my document count stays at 1, nodes top out around 35k, and listeners are a little over 3100.

I'll be honest, I'm not super experienced at leak-tracking in browser-land, and because there's no real memory problem here (it always stays between 7 and 25mb for me), although I definitely consider it a legitimate issue, it's not very pressing in terms of priority.

I'll leave this open for discussion purposes until someone comes along to identify the root cause. Also, I just rewrote the view layer, so hopefully when I test that against the sample, it'll magically go away on its own. One can hope, right?

@blowsie
Copy link
Author

blowsie commented Oct 31, 2013

I can explain how these leaks are often created but I haven't been able to find them within ui-router.

Generally these DOM Node Count leaks occur when you have JavaScript references to them for example:

$("body").append("<div id='myDiv'></div>");
var thisLeak = $("#myDiv");
$("body").empty();

The code above will actually keep the thisLeak reference to the dom node and prevent it from being removed from the dom node count.

To clean this up you must do something like thisLeak = null or delete thisLeak

Hopefully this may give you some clues as to where the issues are.


Running this 1000 times produces results like this.
image

@nateabele
Copy link
Contributor

Right, I'm pretty familiar with that, but I'm also pretty sure we don't retain node references anywhere. That's all handled by the directive system internally.

@chylvina
Copy link

The same thing happens at http://docs.angularjs.org. I have filed an issue:
angular/angular.js#4864

@dlukez
Copy link
Contributor

dlukez commented Nov 11, 2013

image

Seems to garbage collect back down to a relatively small size but DOM nodes etc. stay high

EDIT: Tested on Chrome v30.0.1599.101 m

@chylvina
Copy link

Hi, try this: angular/angular.js#4864 (comment)
I got this result:
ss

Referring to angular/angular.js#2624, Don't worried about the detached dom nodes, The memory is collected when click gc.

@blowsie
Copy link
Author

blowsie commented Nov 25, 2013

I really am worrying about the dom nodes. They are causing a huge memory leak, even with batarang disabled.

image

@g00fy-
Copy link

g00fy- commented Dec 11, 2013

I am having similar issues, I think it may be V8 leak.
Strange things happen, for instance, we found that in our app an empty template with <input type="text" placeholder"somehting"> leaks (dom nodes count) while same template without placeholder does not (no extra directives used).

Is there any way I can check if it is my code that is leaking or ui-router/angular ?

@RossJayJones
Copy link

Getting the same issue on the sample app. See chrome dev tools profile below. Created by moving between the home page and about page. Seems pretty severe. Got to this while trying to trace a memory leak in an application we are building using ui-router

image

@nateabele
Copy link
Contributor

Are you on the latest version? The DOM handling in 0.2.10 has been completely rewritten.

@RossJayJones
Copy link

Seems to be specific to animations. I will supply a proper test when I get a chance to work on it. I am on the latest version

@perrygovier
Copy link

I can confirm it's not specific to v8. I see the same thing when running a script like this in a hybrid app on iOS.

@SimplGy
Copy link

SimplGy commented Jun 19, 2014

You can test this in an isolated way by just reloading the current state.

$state.go($state.current, null, {reload:true})

This leaks for me. I have complicating factors but reading this thread makes me think it's about the state change. I can build an isolated case if there's uncertainty.

@cuipengfei
Copy link

My app is experiencing leaking too.
After switching between different lists(ng-grid), the memory usage goes to 900 plus MB, and the app becomes almost stuck.

@blowsie
Copy link
Author

blowsie commented Jul 25, 2014

@cuipengfei I suggest in your case that you disable ng-grid to try and isolate the issue.

@cuipengfei
Copy link

yes, I tried that. the leaking is still there, but in much smaller dose.
when ng-grid was on, it takes about 40 MB every time I call state.go. when it's off, it takes about 2-4 MB.

@SimplGy
Copy link

SimplGy commented Jul 25, 2014

@cuipengfei: I had the same experience. It leaks whatever is there, and if that content has a big memory footprint (large images, ng-grid, slickgrid), the leak is just larger.

Try it in incognito mode and see if the leak goes away. It did for me. They have it isolated to a Chrome bug I think: angular/angular.js#4864

@cuipengfei
Copy link

@SimpleAsCouldBe
Yes, it leaks whatever is there. And I just tried in incognito mode, it is still leaking.

@anton-107
Copy link

+1

@loureirorg
Copy link

loureirorg commented Jun 25, 2016

@pocesar I see, but just as a curiosity on your example, the extra detached dom objects are being generated by "jqLite", which is a lite version of jQuery (embedded on Angular itself). Angular's jqlite also uses a cache mechanism (jqcache) which on your example is referencing those detached dom objects.

@loureirorg
Copy link

Just a guess, but maybe this is the culprit:
https://github.com/angular/angular.js/blob/master/src/jqLite.js#L124

@Olgagr
Copy link

Olgagr commented Jun 26, 2016

@loureirorg I don't use jQuery in the project, but of course jqLite is there with its caching. However, if these detached trees are created because of caching, why are there 3 copies of them ? Doesn't it negate the idea of caching? Moreover, when I leave the state and for example go to sibling state, all of these detached nodes are gone. That's why I'm thinking this is something inside ui-router and not in Angular-jqLite.

@cstenac
Copy link

cstenac commented Jul 18, 2016

A bit late to the game, but I thought this might be useful for future reference. The following pen produces a leak on 0.2.17 which is fixed since 0.2.18:
http://codepen.io/zorgluby/pen/LkQNpw

Click several times on the "Transition" button then take a Heap snapshot:

  • In 0.2.17, you'll notice several instances of Uint8Array in the heap, indicating that the previous scopes of the ItemSubController haven't properly been destroyed
  • In 0.2.18, only one instance of UInt8Array, as expected

The bug was only reproducible with a reload transition to the same state.
The extra items were still retained via the scope hierarchy because the $destroy was not being called on the previous scopes (but it's unclear for me why since the nodes seemed properly removed - I guess it might be related to the animation stuff). As far as I can see, you really need(ed) to have "deep" states for the issue to happen.

@SeeeD
Copy link

SeeeD commented Aug 11, 2016

+1

@toni-mota
Copy link

Same memory issue with nested views in 0.2.15 and 0.2.18... any news about it?

@LeonanCarvalho
Copy link

3 years and we still have the same issues.

@smcauliffe
Copy link

I just realized this is happening in one of my apps - memory increases with every state change and is never reduced. Angular 1.5.9, ui-router 0.3.2. Anyone have any idea how to work around?

@Olgagr
Copy link

Olgagr commented Nov 28, 2016

@smcauliffe Do you have nested states ?

@smcauliffe
Copy link

@Olgagr yes

@Olgagr
Copy link

Olgagr commented Nov 28, 2016

@smcauliffe See above (#545 (comment)). I had the same issue. In my case the fastest solution was to remove nested states.

@slawrence
Copy link

slawrence commented Dec 6, 2016

Same problem. Eliminating the nested state resolved it. (edit: using version 1.0.0-beta.3)

@quocanhbk09
Copy link

Any new on this issue?
Our project is quite big, so we got a serious problem with memory leak with continuously increasing memory when switching state. (increase about 4-5MB for each switch) we use version 0.2.18, and after updating it to 0.3.2, the issue still happends.

@slawrence Sorry for stupid question, but does "eliminating the nested state" you mentioned mean that not using nested state? or there is a way to properly clean the nested state to prevent memory leak?

Thanks

@marcoblos
Copy link

marcoblos commented Jan 19, 2017

@quocanhbk09 I have same issue than you and all others in this thread and I have same doubt about the slawrence response... @slawrence what did you want with "Eliminating the nested state"... How do you resolve nested states?

angular version: 1.5.8
ui-Router version: 0.2.17

@henk23
Copy link

henk23 commented Jan 19, 2017

@marcoblos The way I understand it is this: Do not use nested states. Try to make your app work without them. (I hope I understand it wrong, because that is not a real solution to the problem.)

@smcauliffe
Copy link

Unfortunately, this seems to be the only "fix" to the problem - which is that the nested state feature is unusable. I would love to be corrected.

@marcoblos
Copy link

@henk23 @smcauliffe I understood about slawrence's response, but I was really hoping I was wrong about the solution.. ;/

@enjoiful
Copy link

"Don't use nested states" is an extremely crummy work around when it is advertised as a core feature of the router. Had I known nested states would cause massive memory leaks, I would have chosen a different router. It seems daunting for me to rewrite 100+ routes in our large app. Even worse considering the code duplication that we'll incur when "de-nesting" all states, as well as losing the thoughtful hierarchy that nested states provides.

At the very least the nested states memory leak should be advertised on the readme as busted until fixed. Literally in the repo header: "The de-facto solution to flexible routing with nested views in AngularJS"

christopherthielen added a commit to christopherthielen/ui-router-dom-leak-test that referenced this issue Feb 2, 2017
@christopherthielen
Copy link
Contributor

christopherthielen commented Feb 2, 2017

An official response

I'd like to officially respond to this ticket. If there are DOM nodes leaking, UI-Router should be fixed.

So, to help reproduce this problem of leaking DOM nodes, I've created a test harness. I've noticed that with AngularJS 1.2.27 and earlier, the DOM nodes do indeed leak. By switching to 1.2.28+, I've been unable to reproduce leaking DOM nodes no matter what version of UI-Router I use.

screen shot 2017-02-02 at 3 03 51 pm

screen shot 2017-02-02 at 3 04 38 pm

I have noticed that HEAP memory usage might increase, but not DOM nodes. The heap increase doesn't seem to be increasing fast enough to become a problem (1-2 MB for 300 state changes)

Note that HEAP is used by anything and everything in a web page, including user code and many things completely outside UI-Router's control such as browser URL history, JIT compiled JS, etc. An increase in HEAP usage over time might be completely normal.

A Test Harness

So, I've created a test harness for you to use.
Reading back in this issue, I'm told the original UI-Router sample app demonstrates the issue. So, the test harness is based on that sample app.

https://christopherthielen.github.io/ui-router-dom-leak-test/#/leaktest-instructions

The purpose of this harness is so we can all be using the same controlled environment. However, please tweak this test harness. Use various combinations of UI-Router and AngularJS versions. Modify the app code. Do whatever you need to do to trigger the leaks. When you have reproduced the leaking DOM nodes, create a PR to my repository with instructions, and respond here.

Garbage Collection

You must run garbage collection at the beginning and end of any test run. While a single state change might take 5 MB of heap, that memory should be reclaimed by garbage collection. Normally, it's up to the browser to initiate garbage collection. The browser may choose to delay a full GC for a very long time, and the results might be very misleading. By running GC at the beginning and end of any test run, we can normalize the actual heap memory and nodes in use.

@christopherthielen
Copy link
Contributor

Interestingly, when I use Chrome Canary, I always show leaking nodes

screen shot 2017-02-02 at 4 16 51 pm

@spc16670
Copy link

spc16670 commented Feb 4, 2017

This thread is long and I have not read it all but I thought I would share what I had to do in my Ionic app to make sure my scopes are cleaned up on state change.

I noticed $destroy event is not fired in my controller attached to an abstract state:

        .state('a-1', {
		url: '/a-1'
		, abstract: true
		, cache: false
		, templateUrl: 'views/a-1.html'
		, controller: 'A1Controller'
	})

I had to do the following to make sure the scope is cleaned up properly:

    $scope.$on("$ionicView.beforeLeave", function(event, data){
        if (data.abstractView === true) {
            $scope.$destroy();
        }
   });

Hope this helps.

@dacsy
Copy link

dacsy commented Oct 12, 2017

Hi all
I am running in to this issues for my old application, my application have been built and release for a few years since 2014. We are having the same problem with angular 1.2.15 and ui-router 0.2.13
I have been researching and reading this topic, but finally could not find the final version should I upgrade to solve the problem.
@christopherthielen mentions angular 1.2.28+ and ui-router 0.4.2 it also a big change for me to upgrade to this version cause the old application and quite big also. But seems it also not solve the problem on those version yet
Is anyone have other idea can help to solve my problem ? Thank you

@christopherthielen
Copy link
Contributor

This issue is closed because I have seen no evidence of DOM nodes leaking. If you have evidence, please post the evidence.

@blowsie
Copy link
Author

blowsie commented Nov 9, 2017

@christopherthielen what about your own evidence? What are you referring to here?

@peebeebee
Copy link

I posted here that there was a problem with UI-Router, but it was another internal library that was causing it. Sorry @christopherthielen :)

LeonanCarvalho referenced this issue in ui-router/core Jan 24, 2018
The treeChanges object has references to the PathNodes from the previous transition (`.treeChanges("from")`).
The PathNode object has resolve data inside it.
The previous transition is reachable via a resolve (via tokens: `"$transition$"` or `Transition`).
Through this chain, all other transitions are reachable and not eligible for GC.

This change cleans out the previous Transition object from the resolves, but only
after a transition has been evicted from the `successfulTransitions` queue.
The `successfulTransitions` queue currently has a max size of 1, so the transition
is cleaned up once it is no longer the current nor previous transition
(it is cleaned when it's the previous previous transition);

Fixes #55
Fixes angular-ui/ui-router#3603
Fixes ui-router/angular#21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests