Will review stuff in ~1 week, currently in holiday

im not sure about this one
detachedWidgets is currently for the AJAX case, so the widget will be destroyed after the next AJAX request and not instant

i think we should destroy it immediately

OK so just to be sure if I have a p:sticky target="pnl" and then I do an AJAX update where I no longer render the pnl the p:sticky holds the references to the panel in detached elements.

however if the Sticky registers the target for removal and I update the pnl to no longer be rendered it adds both the pnl and the sticky to the detached widgets and they both get cleaned up in the same AJAX right?

Or are you saying when its removed from the DOM the Sticky should be detroyed immediately and not added to detachedWidgets ?

ahh
so you mean its still removed in the current ajax request (not in the next) as the detachedWidget cleanup is done after all elements where removed?

Correct!

OK did the following.

1.Renamed to destroyOnElementRemoval
2. Added new removalIdentifiers because we need to track any of the ID's removed else isDetachedWidget() returns the wrong answer.

In my Example above with Sticky and Panel if the Panel is removed we are now adding the Sticky to detached widgets which was good!

However in destroyDetachedWidgets the widget.isDetached() was returning false for the Sticky because its DOM element was not detached it was its target. So now when destroyOnElementRemoval is called we add the ID to an array and we checked that array in isDetached to see if any of the DOM elements we are watching have been detached then isDetached() will return true and clean up the widget.

TBH i wonder if this could destroy some usecases

e.g. a OverlayPanel binded to a button
i know that most of this cases require to update both components
but i can remember that we had some components, that worked if you only update 1 of those 2 married components
this case would be broken with this PR

You are right I will only add it to the components I have seen cause a problem at my client. Namely Sticky and a couple of others.

I removed it from OverlayPanel, ContextMenu, Spotlight, BlockUI but i left it on the others as for things like Draggable, Droppable, Resizable etc the rest it makes no sense not to destroy them when their target is destroyed. as the memory never gets cleaned up.

TBH im not sure if we should merge this
a component should be destroyed, when the DOM element doesnt get rerender on AJAX update
this means its not rendered on server side, and therefore can be removed on client side

with this change the state is different and can make problems like #11413 11409

Understood this one is risky because of the scenarios you mentioned.

Is there some way i can make this an option??

The scenario with that SPA app is each time they open a new Tab.... the tab has a Sticky Menu so if they scroll down the page the menu sticks. They close the tab and the Sticky is now holding memory because its target is not in the DOM but it still has a reference to the target. If they open and close 10 tabs now 10 sticky's are holding memory and it never gets cleaned up.

Even if they re-open a tab they had open that tab creates a new Sticky and new Menu but the original one is holding on to memory. So if they do this for an 8 hour shift the browser eventually crashes.

will review next days

i cant speak for sticky for now (never used it) but e.g. take keyfilter

inputText and keyFilter are propably on same level in DOM
you update the parent container, both are getting destroyed

if have it in different container or and you dont update both, you get a memory leak - but its a user error, not a primefaces bug

maybe we just add some logging here

You are totally right about KeyFilter!

So here is what I propose to not make this change so drastic. Let me back it up to just Sticky and we can evaluate others in the future. The Sticky is the only one that is really causing the major leak at my client so just solving that one would be good?

OK i stripped it down to just Tooltip and Sticky (tooltip already had this behavior for the most part).

can you also exclude unrelated changes here and create a seperate PR? resizable and dragdop doenst use destroyOnElementRemoval

Done... Minimal changes now.

TBH sticky looks very similar to the keyfilter case i mentioned above?
can you post the example code for it?

(im only talking about the destroyOnElementRemoval, overwriting #destroy makes sense anyway and should fix the leak if the user correctly updates both)

This is easy to replicate using the showcase:

1. Navigate to Sticky example: https://www.primefaces.org/showcase/ui/misc/sticky.xhtml
2. Open F12 console and lets remove the Toolbar from the DOM with $('#tb').remove();
3. Now take a Memory Dump and you will see the Toolbar was removed from DOM but the p:sticky still has a JS reference to it in its target property which causes the garbage collector to never collect it.
4. Run PrimeFaces.ajax.Response.destoyDetachedWidgets()
5. Now take a Memory Dump

The 6 buttons, all their DIVS and everything inside the toolbar is now being held in memory as "Detached DOM" elements.

yeah but thats not a "valid case"
we not talking about that users are removing elements with JS and therefore we are getting leaks?
for me a valid case is like this:

 <h:form>
      <div jsf:id="someContainer" rendered="#{el....}>
          <p:toolbar id="tb"/>
          <p:sticky />
      </div>
 </h:form>

if you update form, both are rendered/not-rendered and in the same scope

if the scope is different, then you will get a mem leak; exactly the same like with keyfilter.

OK let me ask them because in that giant SPA app they are dynamically adding and removing Tabs from a TabView and the sticky is inside the tab but it is definitely not being cleaned up properly even thought they have updated the TabView which now has 1 less tab in it. The only way I can get their memory to clean up is to provide them this script which runs on AJAX calls. Basically find all targets and triggers ....

  for (key in PrimeFaces.widgets) {
    let widget = PrimeFaces.widgets[key];
    if (widget && widget.target instanceof jQuery) {
      widget.target.off("remove.widget").on("remove.widget", function () {
        PrimeFaces.detachedWidgets.push(widget.widgetVar);
      });
    }

    if (widget && widget.trigger instanceof jQuery) {
      widget.trigger.off("remove.widget").on("remove.widget", function () {
        PrimeFaces.detachedWidgets.push(widget.widgetVar);
      });
    }
  }

yeah, please ask them, im not sure if my thoughts cover all cases but:

 <h:form>
      <div jsf:id="someContainer" rendered="#{el....}>
          <p:toolbar id="tb"/>
      </div>
      <p:sticky />
 </h:form>

if you take this example and update=someContainer, sticky also wont be automatically reattach to the toolbar as both needs life in the same scope/lifecycle
i think the whole thing is very very tricky

should a component auto reattach to the target component? how are the relations between components, parents, child and so on
this really really needs some detailed specs

nevertheless, maybe its better to provide a own PR with destroy implementations (also keyfilter, dragdrog and so on first)
and really a issue for the releation stuff

OK i will put this one back to draft for now as you are right components should be updated together and finding out why the Sticky is not being destroyed when the Tab is detroyed is really the core issue.

i think we should close this for now - it requires much more discussions etc. and this should be done in the ticket.