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

Fix #11994: TabView prevent detached DOM elements on TabClose #11998

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

melloware
Copy link
Member

Fix #11994: TabView prevent detached DOM elements on TabClose

@melloware melloware added the ⚡ performance Performance related issue or enhancement label May 23, 2024
@tandraschko
Copy link
Member

tandraschko commented May 24, 2024

why did you refactor like this?

#fireTabCloseEvent should just fire the event, everything else was done before in #remove
doing the cleanseDomElement in #remove just would work fine?

@melloware
Copy link
Member Author

So i will double check but since cleanse removes all elements, events, data off all elements when using <p:ajax event="tabClose" it could affect the lifecycle anything that was on that tab expecting the normal widget lifecycle so I chose to do if using tabClose to wait until the AJAX is done to clean up the DOM else just clean it up immediately if not using tabClose.

It felt like the cleaner / safer way to do it but I could be convinced otherwise?

@tandraschko
Copy link
Member

both elements were already destroyed before sending the event in the past and also feels better for me
as there is no delay waiting for the ajax response, its just a notifier and we never had any feature request for something here

@melloware
Copy link
Member Author

Ok I will test and update.

@melloware
Copy link
Member Author

melloware commented May 25, 2024

@tandraschko did some testing in my clients real world app.

Test with just changing to cleanseElement instead of panel.remove()

Here is what it looks like with this PR where the cleans runs BEFORE the AJAX runs. Memory recovered back from 16.5MB to 13.3MB with lots of detached elements still in memory (which were in OverlayPanels) in the tab.

image

Current PR

Here is what it looks like with this PR where the cleans runs AFTER the AJAX runs and all the widget.destroy's properly take place. Memory recovered back to from 16.5MB to 9.5MB and barely any detached elements in memory.

image

I think I know what is going on but still testing.

@melloware melloware force-pushed the PF11994 branch 2 times, most recently from 426fbd9 to ad70f34 Compare May 25, 2024 12:46
@melloware
Copy link
Member Author

@tandraschko ok PR updated I got to the bottom of it. In cleansDomElement we must call jq.remove() before removing all event listeners with jq.off() This is so the widget on("remove) events get fire to register widgets as detached widgets so they clean themselves up.

I have verified with my testing above the memory is now recovered properly in my test scenario.

@tandraschko tandraschko merged commit ff0c381 into primefaces:master Jun 4, 2024
12 checks passed
@melloware melloware deleted the PF11994 branch June 4, 2024 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚡ performance Performance related issue or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TabView: Memory leak on Tab close
2 participants