_onResetCallbacks hold reference to stylesheet object on window, adversely impacting memory usage of Teams

[anshulchanchlani]

Environment Information

• Package version(s): "@fluentui/react": "^8.15.1"
• Browser and OS versions: Edge/Chrome, Windows

Please provide a reproduction of the bug in a codepen:

Actual behavior:

Investigating a customer incident for LPC in Teams, I found that there are several instances of '_onResetCallbacks' employed by the memoize and other functions, coming from Fluent, that LivePersonaCard experience consumes, which hold stylesheet object on the window and thus isn't collected by Browser's garbage collector which then increases the Teams memory usage significantly.

Pointer to code - https://github.com/microsoft/fluentui/blob/080da667db6a7ed87a0421103804e94710ec8947/packages/merge-styles/src/Stylesheet.ts#L197

Excerpt from Memory heap snapshots:

Expected behavior:

_onResetCallbacks shouldn't bloat/leak memory by holding onto references

Priorities and help requested:

Are you willing to submit a PR to fix? (Yes, No)

Requested priority: (Blocking, High, Normal, Low) High

Products/sites affected: (if applicable) Teams, Live Persona Card

[gouttierre]

@anshulchanchlani - Thanks for filing this issue with us. To set expectations the developers will review this issue once capacity allows and they will require to see the code to better assist.

@layershifter - I am looping you in as you are the lead for the merge-styles cc @khmakoto

[layershifter]

@layershifter - I am looping you in as you are the lead for the merge-styles

Sorry, but I don't own merge-styles, I am working on makeStyles for v9.

[lekevin-microsoft]

Hi there, Outlook Web App is actually seeing this same behavior as well with _onResetCallbacks. If it's okay, I'd like to start this conversation again as I see it's paused since November. Let me know if I should make my own ticket.

The issue: Outlook Web is seeing a large memory leak within its Ribbon section, and we believe it's due to _onResetCallbacks from Fluent side. The leak (this is after garbage collection button was clicked, to ensure these were truly leaked):

Repro for Outlook Web:

1. Open Outlook Web (outlook-sdf.office.com/mail)

2. Click between "Home" and "View" tab a few times. They should look like this: .

3. (If you don't see the tabs, click the module switcher on the top right -- its a little "V" dropdown) and click "Single Line Ribbon". It should look like this:

4. Go to F12 > Detached Elements > "Get Detached Elements". (Icon looks like a "refresh" icon.)

5. Click on one of those <div style="position: absolute; visibility: hidden;">, if you expand it should show the Ribbon tabs eventually.

6. Notice that every time you click between "Home" and "View", we're retaining another <div style="position: absolute; visibility: hidden;"> DOM tree.

7. Clicking on "Analyze" on the top of the Detached Elements pane will give you a retainer list to see what's retaining.

8. We're seeing this:
   

Expected behavior: We shouldn't be keeping these DOM elements around.

Impact Currently, every single time a user switches Ribbon tabs, or any times a tab needs to rerender for whatever reason (such as clicking "New Mail" which will render Message / Format / Insert tabs), it leaks and we keep those references around forever. The size of a "Home / View" tab is 272 bytes, the size of "Home / View / Message / Insert / Format" (Compose state tabs) is 328 bytes. This easily gets up to the dozens and eventually hundreds over a long session.

[dzearing]

Sometimes when we look at memory retainers, we see red herrings. I think this may be one of those cases. In order to determine exactly the root cause here, you need to isolate a repro. If you can repro a leak in isolation, it's Fluent's concern. If not, the app or component is doing something which is exacerbating if not causing the conditions leading to a leak. We need to isolate this much better.

@anshulchanchlani If you have an isolated repro, please provide. It's still unclear if the app is doing something, LPC is doing something, or Fluent is doing something. If you can isolate, then we can clearly know which layer is doing the "bad thing" causing the problem.

@lekevin-microsoft Same to you - Can you please isolate? Make a quick repo which exercises the exact fluent scenario causing the leak. If you can do this, changes can be made. Otherwise, it's unclear what to address. What exactly is adding the divs? Did you set up dom mutation breakpoints to find who is adding them and when/why? How exactly are they retained?

The onReset function on the Stylesheet management object allows callers register a callback which will be executed when the reset function is called. The _onResetCallbacks array holds references to callbacks registered with onReset. Fluent code itself only registers 1 callback through the onReset from the id generation logic in the utilities package. So possibly the app is registering something as well.

The __stylesheet__ reference points to the singleton stylesheet instance, which retains references to the style elements it has created. But I'm not sure how that relates to the random divs added.

When you shift from one tab to another inside a Tabs/Pivot component, and you're seeing memory grow, maybe new rules are getting registered. But if you continue seeing memory grow from switching over and over between the same tabs where the content is exactly the same, then maybe there's an issue.

[lekevin-microsoft]

Thanks @dzearing , appreciate the reply!

That's interesting what you said about __stylesheet__... What tabs are doing is that, to recalculate the size and know whether they should shrink/grow, they actually render a temporary div with its own style. So, on each tab click, we render a new temporary div (which should be removed from memory but we're seeing something keeping it alive). Now, I bet what's happening after your clue about stylesheet is that we create a style for this div, and the stylesheet singleton isn't removing them. Not saying it's a fluent bug, but that would explain how it relates to the random divs added.

I'll look into the issue more with this knowledge about __stylesheet__. @dzearing if anything I wrote gives a huge clue, please do chime in!

[dzearing]

@lekevin-microsoft interesting! So, some things to consider:

1. style elements will be only created when unique rules are encountered. So for example, if "width" is measured from getBoundingClientRect and returns a different value each time, then yes, we would create new rules. But if the same values are passed in, no we won't create new style rules.
2. A style element has no reference to the div element so that wouldn't necessarily be the reason the divs are being preserved or leaked. It would just show new elements and memory get used on each click of the tabs.
3. Memoization helpers around style computation can grow infinitely very easily if their key inputs mutate each time. For example, if memoizeFunction is used to wrap a function which takes an object as input, and that object is a new reference each time the function is called, that could cause new keys to be created and retain memory. This doesn't really relate to detached elements leaking, but more about memory usage growing over time.

Detached elements leak when something (usually a global state object like __stylesheet__) holds on to the head of a chain of things that ends up closuring a variable referencing the element.

Consider a hypothetical scenario like this:

function initialize() {
 let divs = [];
  stylesheet.onReset( () => {
    // clean up the divs when the stylesheet resets.
    for (const div of divs) {
      div.parentNode.removeChild(div);
    }
  });
});

The divs array is closured in the same scope as the reset function passed into stylesheet.onReset. So... even if you remove them elsewhere in the code, the stylesheet object retains a reference to the reset function passed in, which retains a reference to the array, which retains a reference to the elements so they can't be GC'd.

I'm not saying this is your scenario; just illustrates how sneaky leaks can be. It's not the stylesheet's fault in this case, but simply closuring the collection (could be in file scope too) ends up pinning the blame in the wrong place. Sometimes nulling or clearing things can help remove retainers; in the case above, whatever detaches the elements must also clear the array.

[msft-fluent-ui-bot]

Because this issue has not had activity for over 180 days, we're automatically closing it for house-keeping purposes.

Still require assistance? Please, create a new issue with up-to date details.