preact icon indicating copy to clipboard operation
preact copied to clipboard

Possible memory leak

Open Z2Y opened this issue 3 years ago • 14 comments

Describe the bug When i use preact with a virtualized list, I found memroy not gabage collected. this may cased by a chrome issue: The MouseEventManager keeps removed DOM nodes alive until subsequent click https://bugs.chromium.org/p/chromium/issues/detail?id=1177010 if there is no click event for a while , (such as scrolling infinite card list ), the memory usage grows until next click.

Z2Y avatar Aug 13 '22 05:08 Z2Y

Please share more information about your setup. We cannot see your screen and don't know what your code looks like, so it's pretty much impossible for us to provide help with the issue you're seeing. Specifically the virtual list implementation that's used in your project and how you're using it might be of interest.

marvinhagemeister avatar Aug 14 '22 07:08 marvinhagemeister

1660469565(1)

the event handler hold the ref of a unmountd vnode, ant the vnode hold the parent vnode. if the detached dom not gabage collected , it will cause memory leak.

Z2Y avatar Aug 14 '22 09:08 Z2Y

From the linked issue this is a chrome bug and not a Preact one, there's not much we can do about the implementation of a browser event handler as this would occur in any library. The screenshot attached also does not seem to relate to the bug as this is a VDOM structure.

The event-handler in question is a native one as we aren't creating our own events so the only things interacting here should be your callback and the browser event-handler.

If this is a Preact bug could you show us how this related to the chrome bug or how the screenshot is relevant to what you are saying - currently you say that the event handler holds the ref of an unmounted vnode, do you mean that the vnode holds a reference to the dom-node which in turn has event-handlers? Theoretically this makes sense as a virtualization library usually won't unmount dom-nodes rather than just hiding them so I don't think it's a memory leak as much as an implementation detail in the virtualization library.

Do note that the above hinges on assumption that I made, if these are incorrect please provide more information

JoviDeCroock avatar Aug 14 '22 10:08 JoviDeCroock

I managed to create a demo to reproduce it https://codesandbox.io/s/preact-vlist-demo-ehphz9 steps: Add some items and then scroll up and down, As the chrome devetool show , the number of detached HtmlButtonElement grows up. WeChat0a55e3857971618997c10743f53c67a5

same code using react has no such issue https://codesandbox.io/s/react-vlist-demo-f3egjs

Z2Y avatar Aug 15 '22 03:08 Z2Y

@Z2Y this happens because SomeButtonItem closes over props.children, which retains it. The issue is already fixed in Preact 11, where props.children is a pure JSX element with no tree structure: https://codesandbox.io/s/preact-vlist-demo-forked-o26dg7?file=/src/index.js

In the meantime, here's a workaround that adds some additional unmount cleanup that likely addresses the issue:

import { options } from 'preact';

let oldUnmount = options.unmount;
options.unmount = (vnode) => {
    if (oldUnmount) oldUnmount(vnode);
    if (vnode.__e) vnode.__e.l = undefined;
    if (vnode.__c) vnode.__c.__H = undefined;
    vnode.__ = vnode.__k = vnode.props = undefined;
};

developit avatar Aug 17 '22 19:08 developit

@developit Dose the workaround has other side effects? Since there are some work after options.unmount. https://github.com/preactjs/preact/blob/1cb4b3811599095ea64710faaa260350a1bc7c5b/src/diff/index.js#L499-L527

Currently I add these vnode to a set then do the cleanup later.

options.unmount = (vnode) => {
    if (oldUnmount) oldUnmount(vnode);
    unmountedVnodes.add(vnode); // then do cleanup in a setInterval
};

Z2Y avatar Aug 18 '22 03:08 Z2Y

@Z2Y the workaround won't break anything, I only have it clearing properties that Preact ignores during unmounting.

Your Set solution is good though, you could iterate over the properties of each and set them to undefined.

developit avatar Aug 18 '22 03:08 developit

@developit that workaround might be dangerous in suspense scenario's no?

JoviDeCroock avatar Sep 02 '22 08:09 JoviDeCroock

I upgrade to latest preact v10.11.0, but the problem still exist.

Z2Y avatar Sep 13 '22 03:09 Z2Y