Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression test: SuspenseList memory leak #20433

Merged
merged 1 commit into from
Dec 11, 2020

Commits on Dec 11, 2020

  1. Regression test: SuspenseList causes lost unmount

    @sebmarkbage reminded me that the complete phase of SuspenseList
    will sometimes enter the begin phase of the children without calling
    `createWorkInProgress` again, instead calling `resetWorkInProgress`.
    
    This was raised in the context of considering whether facebook#20398 might
    have accidentally caused a SuspenseList bug. (I did look at this code
    at the time, but considering how complicated SuspenseList is it's not
    hard to imagine that I made a mistake.)
    
    Anyway, I think that PR is fine; however, reviewing it again did lead me
    to find a different bug. This new bug is actually a variant of the bug
    fixed by facebook#20398.
    
    `resetWorkInProgress` clears a fiber's static flags. That's wrong, since
    static flags -- like PassiveStatic -- are meant to last the lifetime of
    the component.
    
    In more practical terms, what happens is that if a direct child of
    SuspenseList contains a `useEffect`, then SuspenseList will cause the
    child to "forget" that it contains passive effects. When the child
    is deleted, its unmount effects are not fired :O
    
    This is the second of this type of bug I've found, which indicates to me
    that it's too easy to accidentally clear static flags.
    
    Maybe we should only update the `flags` field using helper functions,
    like we do with `lanes`.
    
    Or perhaps we add an internal warning somewhere that detects when a
    fiber has different static flags than its alternate.
    acdlite committed Dec 11, 2020
    Configuration menu
    Copy the full SHA
    54ddad8 View commit details
    Browse the repository at this point in the history