[Memory-leak]: SWC Menu.ts removes the menu items on disconnect, but it doesn't clear out it's lookup set

[kumarhimanshu5128409]

Code of conduct

• I agree to follow this project's code of conduct.

Impacted component(s)

sp-menu

Expected behavior

On disconnecting SWC Menu.ts should clear out it's lookup set.

Actual behavior

On disconnecting SWC Menu.ts is not clearing lookup set.

Screenshots

No response

What browsers are you seeing the problem in?

No response

How can we reproduce this issue?

Reproducible on http://new.express.adobe.com/
simply placing your cursor in the search input ... you should see a menu popup with suggestions ..
if you type then click out of the search input to dismiss the menu,
and then do a heap snapshot,
you should see the retainer.

Sample code that illustrates the problem

No response

Logs taken while reproducing problem

No response

[jblas]

I think I figured out the source of the menu-item leaks we've been seeing sometimes, that don't get addressed by clearing this.childItemSet.clear() in the Menu's disconnectedCallback() method.

Our project is currently using 0.31.0 of the Menu package, and on the SWC main branch the code looks slightly different from 0.31.0 so I'm not sure this is still an issue or not, but I'll explain anyways what I'm seeing.

The basic setup of the search results menu in our app is this:

sp-menu [1]
    sp-menu-group [2].     <--- Note that sp-menu-group IS-A menu
        shadow-root
            sp-menu [3]
                shadow-root
                    slot
        sp-menu-item [A]
        sp-menu-item [B]
        ...

When any of the sp-menu-item elements are added to the sp-menu-group, the connectedCallback() for the menu-item dispatches an addOrUpdateEvent on itself so the event propagates like this:

sp-menu-item [A] --> sp-menu [3] --> sp-menu [2] --> sp-menu [1]

https://github.com/adobe/spectrum-web-components/blob/v0.31.0/packages/menu/src/MenuItem.ts#L517

Which eventually triggers each Menu's addChildItem() which adds the menu-item to the childItemSet of each menu in the propagation hierarchy.

Note that sp-menu [3] is in the event propagation hierarchy because the menu-items get slotted underneath it.

Now on removal/disconnect of any of these sp-menu-items a removeEvent will be triggered, but it is dispatched on the parentElement of the menu-item which in this case would be sp-menu [2]:

https://github.com/adobe/spectrum-web-components/blob/v0.31.0/packages/menu/src/MenuItem.ts#L526

So what we end up seeing is a propagation like this:

sp-menu [2] --> sp-menu[1]

which means the menu-items are never removed from the sp-menu [3] childItemSet because it never sees the removeEvent. The fact that it remains in sp-menu [3]'s childItemSet is enough to keep all of the menu-items retained in memory.

As I mentioned I don't know if this is an issue still on the SWC main or not, because I no longer see a removeEvent.

[Westbrook]

All of this is quite different in the latest version of this code. For the amount of time spent investigating consumption of the older version of this pattern, I'd have thought that you could have updated to the latest version of the library a number of times over by now. Without a reproduction of a leak of this nature in the current version, we'll likely be closing this issue before long as "Can't repro".

[jblas]

@Westbrook: Our application is large, with many teams, many different schedules and priorities, and many moving parts. It's a bit more complicated than just dropping in the latest version because, doing so affects everyone. Someone did make an attempt, but in doing so there were apparently several issues that would've needed to be addressed. In the mean time some of us are trying to make forward progress on fixing memory issues that we see.

[Westbrook]

Totally get the realities of your project. I’m just sharing the realities of ours. While we believe some or most of these issues have been addressed in the newer version, it’s likely not all and more than likely has created new and different issues, so I just worry the efforts you’re making now will be needed a second time if appropriate repro and test harnessing can’t be generated as part of the process.

Maybe you’re getting this coverage into your app in a way that can facilitate easier adoption of new versions in the future, but it would be even better for us to be able to catch/address them at the source and greatly appreciate your support in doing so.