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

Closed panel element stay indefinitely on Detached DOM tree #216

Closed
Jmales opened this issue Feb 15, 2017 · 18 comments
Closed

Closed panel element stay indefinitely on Detached DOM tree #216

Jmales opened this issue Feb 15, 2017 · 18 comments
Labels
Angular.js Angular 1.x related issues. High Priority Mission Critical Issues kind/bug Identifies an issue or a PR that mentions or fixes a bug

Comments

@Jmales
Copy link

Jmales commented Feb 15, 2017

I'm using Angular with Golden-layout, and using $compile to put my directives in the html I can get everything to work as expected.

However, using Profiling Tools from Chrome I noticed than upon closing of a panel the elements are not garbage collected, since all of them stay in the Detached DOM tree, even after waiting for a while. This causes my app to get more and more un-responsive.

If I search de DOM for the elements I don't find them. So upon closing does golden-layout destroy or not the element? And if so, why aren't mine destroyed?

@ButchMonkey
Copy link
Contributor

Looking through the code briefly, the panels are removed using jQuery remove which should remove the elements and unbind any event listeners.
Using remove should prevent any elements form being left behind.

Would it be possible to create a small codepen example for testing?

@Jmales
Copy link
Author

Jmales commented Feb 16, 2017

@ButchMonkey I discovered that the golden-layout examples on angular also leak. I added just a small function "TopRatedControllerTag()" to the example, and after the "Selected User" panel is closed you can search for "Tag" in the Snapshot. You will notice that the function is still there and also that there are a lot of Detached DOM tree items.

Link for codepen example: http://codepen.io/JPMP/pen/XpQzrw

Should I open a new issue on this?

Please help me solve this

@ButchMonkey
Copy link
Contributor

I took a look and can see what you mean. I will do some investigation into the code and look for a fix.

@Jmales
Copy link
Author

Jmales commented Feb 16, 2017

@ButchMonkey Thank you mate! Will be waiting for your answer.

Also, to help you: I was using golden-layout 1.5.6. Tried upgrading to 1.5.7 and the same thing happens.

@ButchMonkey
Copy link
Contributor

At a glance I'm guessing the ItemContainer isn't being destroyed properly. But not too sure yet

@Jmales
Copy link
Author

Jmales commented Feb 16, 2017

For what is worth it, from the analysis I've been doing I am pretty sure that's the problem (or at least one of them).

Are you the only guy maintaining golden-layout currently? I guess this issue should have top priority in being solved by the GL team right? I mean, one of my panels occupies 10Mb because it's doing big d3 charts. Imagine the memory leak on that after a while...

If we can't solve this I'll be forced to use another library similar to GL for my Electron app. I would hate that however, since so far I loved GL.

@ButchMonkey
Copy link
Contributor

There are a few, not just me :) I'm just helping out where I can.
Agreed that this is high priority. After a while if the user is moving or opening / closing panels the memory leak can grow pretty badly.

@ButchMonkey ButchMonkey added kind/bug Identifies an issue or a PR that mentions or fixes a bug High Priority Mission Critical Issues labels Feb 17, 2017
@Jmales
Copy link
Author

Jmales commented Feb 22, 2017

@ButchMonkey Any progress in solving the bug?

@ButchMonkey
Copy link
Contributor

I can see what's causing the memory leaks and which elements aren't getting destroyed. Just finding a way to destroy them correctly is proving a bit tricky.

@Jmales
Copy link
Author

Jmales commented Feb 22, 2017

Okok, at least you are trying :) GL with that mate

@Jmales
Copy link
Author

Jmales commented Mar 20, 2017

Any news on this?

@andyklaiber
Copy link
Contributor

I submitted a PR that should fix it. #291

@bZichett
Copy link

bZichett commented May 14, 2018

So I gather there are probably other instances of hanging DOM references or is this actually fixed? Any ideas? Im willing to look into the code next weekend. If anyone has any tips, it would be much appreciated

@nopmop
Copy link
Contributor

nopmop commented May 15, 2018

I just saw this past issue cause it got updated by @bZichett and came up in the list, and since it's about memory-leaks it's important. I'll involve FYI @ndelangen, @WolframHempel .

@andyklaiber I just saw your PR #291, and I saw that @akaJes reviewed the fact that the PR was interrupting the chain of events being fired on drag and concurring in breaking touch-support for which PR #225 was the initial commit point (where @ButchMonkey and @mattgodbolt were primarily involved). I later briefly documented the bug in 2 comments here.

FYI all...: the broken touch-support was fixed in the ES6 tree, in the following way: DragProxy.js calls _undisplayTree() rather than _updateTree() onDrag, and calls _updateTree() only onDrop. As a result of this the DOM-elements involved in the drag are never removed prior to the drop but only hidden, and the logic by which the drop-areas & targets are computed has changed slightly. As a side-effect of this change memory management could be improved now but nobody has tested it. Bottom line: If anyone wants to test for leaks please do it on the ES6 tree so that we can follow the line of improvements.
Thanks.

@bZichett should you decide to look into the code this weekend please work onto the ES6 tree, in which you can make use of the Tracing system that I added (PR is still outstanding #439). Thanks.

@martin31821 martin31821 added the Angular.js Angular 1.x related issues. label Jan 28, 2020
@MatthewMarichiba
Copy link

MatthewMarichiba commented Oct 10, 2020

Hi! Any progress on this? I'm working on an Angular9 project and seeing this same behavior.
I see the container destroy callback getting triggered when containers close. I've tried manually dropping the HTML element in the callback (ex: container.getElement().remove();), among other things, all to no avail.

Here's an example snapshot from chrome devtools, through several iterations of open-close cycles, adding and closing containers in GoldenLayout. Each step up is an Open action, and as shown, the DOM nodes never get cleaned up after close.
image

As our app is getting heavier usage, this memory leak is dogging us more and more. An update on whether this will be addressed would be appreciated, or any clues as to what might be causing this.

Thanks!

FYI, I think the GoldenLayout codepen Angular demo exhibits this same behavior. Open the codepen and look at the DOM elements in devtools. Closing either of the GoldenLayout containers does not show a corresponding drop in DOM elements, as you would expect.
http://golden-layout.com/examples/#9cc03061dd363f9b3d014ad4c6b7937d

@martin31821
Copy link
Member

@MatthewMarichiba do you use the ngx-golden-layout binding?
I'm using it in several applications with this binding and do not have problems with memory leaks.

Please make sure you correctly destroy your Angular ComponentRef.

@MatthewMarichiba
Copy link

Thanks @martin31821. We are not using ngx-golden-layout. I wasn't part of the decision initially, so not sure why, but retrofitting now would be heavy handed. Could you elaborate on "correctly destroy your Angular ComponentRef," in case I'm missing something? To instantiate the angular component & attach it to the GL container we do something like:

// ...preamble to get the ModuleFactory and ComponentFactory for the component...
const componentRef = ngContainerRef.createComponent(compFactory);
glContainer.getElement().html($(componentRef.location.nativeElement));

And later in the GL destroy callback, I do

componentRef.destroy();

If you see anything at fault there, I'm all ears. Thanks!

@sergiocapozzi77
Copy link

Any update on this issue?
I'm using golden layout ngx-golden-layout with angular and when closing a tab I can see ComponentRef is still referenced in somewhere in code keeping it alive as a DetachedObject

a1
a2

@MatthewMarichiba seems we have the same problem...each tab of my application reference a component which use quite a lot of memory, so keeping it alive is quite a problem. Did you find any workaround?

@pbklink pbklink closed this as completed Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Angular.js Angular 1.x related issues. High Priority Mission Critical Issues kind/bug Identifies an issue or a PR that mentions or fixes a bug
Projects
None yet
Development

No branches or pull requests

9 participants