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

Bugfix: Don't rely on finishedLanes for passive effects #21233

Merged
merged 1 commit into from
Apr 11, 2021

Commits on Apr 11, 2021

  1. Bugfix: Don't rely on finishedLanes for passive effects

    I recently started using `pendingPassiveEffectsLanes` to check if there were any pending
    passive effects (530027a). `pendingPassiveEffectsLanes` is the value of
    `root.finishedLanes` at the beginning of the commit phase. When there
    are pending passive effects, it should always be a non-zero value,
    because it represents the lanes used to render the effects.
    
    But it turns out that `root.finishedLanes` isn't always correct.
    Sometimes it's `NoLanes` even when there's a new commit.
    
    I found this while investigating an internal bug report. The only repro
    I could get was via a headless e2e test runner; I couldn't get one in an
    actual browser, or other interactive environment. I used the e2e test to
    bisect and confirm the fix. But I don't know yet know how to write a
    regression test for the precise underlying scenario. I can probably
    reverse engineer one by studying the code; after a quick glance
    at `performConcurrentWorkOnRoot` and `performSyncWorkOnRoot`, it's not
    hard to see how this might happen.
    
    In the meantime, I'll revert the recent change that exposed the bug.
    
    I was surprised that this had never come up before, since the code that
    assigns `root.finishedLanes` is in an extremely hot path, and it hasn't
    changed in a while. The reason is that, before 530027a,
    `root.finishedLanes` was only used by the DevTools profiler, which is
    probably why we had never noticed any issues. In addition to fixing the
    inconsistency, we might also consider making `finishedLanes` a
    profiling-only field.
    acdlite committed Apr 11, 2021
    Configuration menu
    Copy the full SHA
    e22a984 View commit details
    Browse the repository at this point in the history