VNode leak using Context

[korDen]

• [ ] Check if updating to the latest Preact version resolves the issue

Describe the bug This is a followup to another leak: VNode leak in Context · Issue #4905 · preactjs/preact. There is a VNode memory leak occurring when using Context Provider with state updates. After clicking a button that triggers a state change, the previous VNode hierarchy is still retained in memory, which can be observed in Chrome Dev Tools. This results in duplicate VNodes including 2 <button>s, 2 <App>s, 2 <Context>s, 2 <RouterContent>s and 2 Text nodes (both the old and new text content).

To Reproduce

StackBlitz reproduction link: https://stackblitz.com/edit/vitejs-vite-21mvd37v?file=src%2Fmain.tsx,index.html

Repro:

import { render, createContext } from 'preact';
import { useState, useContext } from 'preact/hooks';

const RouterContext = createContext(null);

function RouterContent() {
  const { params, setRouterState } = useContext(RouterContext);
  return (
    <button onClick={() => setRouterState({ params: params + "!" })}>
      {`No account found for username "${params}"`}
    </button>
  );
}

function App() {
  const [routerState, setRouterState] = useState({ params: 'a' });

  const contextValue = { 
    params: routerState.params,
    setRouterState
  };

  return (
    <RouterContext.Provider value={contextValue}>
      <RouterContent />
    </RouterContext.Provider>
  );
}

render(<App />, document.getElementById('app'));

which compiles to (sometime its easier to check correctness of .js vs .ts):

import { jsx as _jsx } from "preact/jsx-runtime";
import { render, createContext } from 'preact';
import { useState, useContext } from 'preact/hooks';
const RouterContext = createContext(null);
function RouterContent() {
    const { params, setRouterState } = useContext(RouterContext);
    return (_jsx("button", { onClick: () => setRouterState({ params: params + "!" }), children: `No account found for username "${params}"` }));
}
function App() {
    const [routerState, setRouterState] = useState({ params: 'a' });
    const contextValue = {
        params: routerState.params,
        setRouterState
    };
    return (_jsx(RouterContext.Provider, { value: contextValue, children: _jsx(RouterContent, {}) }));
}
render(_jsx(App, {}), document.getElementById('app'));

Steps to reproduce the behavior:

1. Run the web app (preferably with the previous memory leak patch applied -- I tested with the patch applied).
2. Open Chrome Dev Tools and navigate to the Components tab (React DevTools extension)
3. Click on the button that says No account found for username "a"
4. Observe in Dev Tools that previous VNode hierarchy is still retained alongside the new one
5. See duplicate components and text nodes indicating a memory leak

Expected behavior When the state updates and the component re-renders, the previous VNode hierarchy should be properly cleaned up and garbage collected. Only the current VNode tree should exist in memory, not duplicates of all components and text nodes from previous renders.

[korDen]

Hopefully this visualization will help:

Semi-transparent nodes are the leaks. Edges are how the nodes are connected to each other.

Even though Fragment's _vnode._children was updated to contain the new App VNode, the Fragment component instance's props still references the original props object with the original children array.

Note that Fragment.children is correct and point to the right array that points to the new VNode but Fragment.props.children point to the old array, creating a leak.

[korDen]

The issue here is that props.children point to the original hierarchy and never change. But Fragment creates its own _children which does change and is accurate.

This is probably a bad fix, but this worked:

oldDom = diffChildren(...);
if (newVNode.type == Fragment) {
  delete c.props.children; // Fragment doesnt use props.children so might as well unset it
//  c.props.children = newVNode._children; // or update to be consistent. Fragment._children is only created once so this one-time patch should work.
}

cant think of a solution that wouldnt mutate props since props is whats leaking and Fragment itself doesnt rerender so props never change.

[JoviDeCroock]

Being clear about this, this isn't as much of a memory leak as it's a residual tree due to strict equality bailouts. Not to say that we shouldn't fix this but it doesn't infinitely grow so it's not that bad of a side-effect. I think that fixing this in X is not worth the effort as there are a few avenues available but those would be breaking changes

• Clone every outcome of jsx/createElement
• Leverage a backing-tree
• Delete props.children on unmount/update/... --> would need Suspense re-implemented

To provide a better visualization of what's going on

We wrap every render invocation in a Fragment so we always have a stable root-node, this will hold _children which will get updated when the tree updates as well as when the tree unmounts, ... The issue is that this also holds a copy of props.children which is harder to update and when the RouterProvider renders its children it will return a RouterContent which Strictly bails.

This means that a sub-tree won't get updated, it will however get updated upon the next render invocation as RouterContent is set up to render due to an updated context value.

props.children will keep pointing at an old RouterContent because we strictly bailed, however we do end up updating RouterContent because of its context update. The context update runs through a copy, the solution could be to look at _parent.props.children as well during the updatePointers step but again, as this is not severe I am not sure whether the bang is worth the buck.

While this is retained memory, it won't grow endlessly and neither will it negatively impact performance as it's tiny by default. The origin of it is also only limited to 1 because it starts at the root-node and is caused by its props.children.

With all due respect, I don't consider this an issue and will even vary based on the GC method implemented by your JS-engine. In mark-and-sweep (the majority) these things happen and duplicates will stick around and actively trying to erase will lead to performance regressions due to more runs.

[korDen]

`Delete props.children on unmount/update/... --> would need Suspense re-implemented

will this be safer? c.props.children = newVNode._children; this is actually more accurate because newVNode._children is updated in-place with correct references after every reconciliation.

In my local tests it does make a noticeable difference, leading to shorter GC pauses and less frequent collections.

[rschristian]

Feel free to make a PR, our CI does a pretty good job at checking for correctness & perf. Can certainly use it to test out ideas if you got 'em.

[JoviDeCroock]

That would be safer but you'll quickly notice that there isn't really a good way about this. As I said, this is not a leak as it doesn't grow, let's dig more into that though, the biggest part of the issue is that strict equality means that potentially (in practice this is very rare as your context consumers aren't direct children of your Provider) props.children is outdated compared to an updated child.

<Provider> --> updates through state update and triggers child updates
  <Children> --> strictly bails but then updates through context update

The above scenario is what happens, props.children points at the old shape of Children while it has updated over a context-update causing a discrepancy. Technically context-children will be far removed from the parent and this should not happen.

This means that if we touch props.children on oldVNode of the strict bailing children that we will be fiddling with something that might not be relevant. When we don't fiddle with it the updates will happen correctly but use an older VNode as the base-shape (not problematic as updates are always progressive).

This also means that the vnode carrying props.children isn't guaranteed to be the direct parent of a component that does a bail update loop, hence me mentioning that this case is extremely rare.

I won't argue that it's better to get this resolved but it might not be ideal to perform this in X, the case is extremely rare and the byte-size might not warrant a fix.