From e22a98458b78c224756791023157938391416cc9 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Sun, 11 Apr 2021 03:18:21 -0500 Subject: [PATCH] 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. --- .../react-reconciler/src/ReactFiberWorkLoop.new.js | 10 +++++++++- .../react-reconciler/src/ReactFiberWorkLoop.old.js | 10 +++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index df1b2653e204a..bd65591558bee 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2131,7 +2131,12 @@ function commitRootImpl(root, renderPriorityLevel) { export function flushPassiveEffects(): boolean { // Returns whether passive effects were flushed. - if (pendingPassiveEffectsLanes !== NoLanes) { + // TODO: Combine this check with the one in flushPassiveEFfectsImpl. We should + // probably just combine the two functions. I believe they were only separate + // in the first place because we used to wrap it with + // `Scheduler.runWithPriority`, which accepts a function. But now we track the + // priority within React itself, so we can mutate the variable directly. + if (rootWithPendingPassiveEffects !== null) { const renderPriority = lanesToEventPriority(pendingPassiveEffectsLanes); const priority = lowerEventPriority(DefaultEventPriority, renderPriority); const prevTransition = ReactCurrentBatchConfig.transition; @@ -2169,6 +2174,9 @@ function flushPassiveEffectsImpl() { const root = rootWithPendingPassiveEffects; const lanes = pendingPassiveEffectsLanes; rootWithPendingPassiveEffects = null; + // TODO: This is sometimes out of sync with rootWithPendingPassiveEffects. + // Figure out why and fix it. It's not causing any known issues (probably + // because it's only used for profiling), but it's a refactor hazard. pendingPassiveEffectsLanes = NoLanes; invariant( diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 07f8a38b8bc0a..6df35c9bf5160 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -2131,7 +2131,12 @@ function commitRootImpl(root, renderPriorityLevel) { export function flushPassiveEffects(): boolean { // Returns whether passive effects were flushed. - if (pendingPassiveEffectsLanes !== NoLanes) { + // TODO: Combine this check with the one in flushPassiveEFfectsImpl. We should + // probably just combine the two functions. I believe they were only separate + // in the first place because we used to wrap it with + // `Scheduler.runWithPriority`, which accepts a function. But now we track the + // priority within React itself, so we can mutate the variable directly. + if (rootWithPendingPassiveEffects !== null) { const renderPriority = lanesToEventPriority(pendingPassiveEffectsLanes); const priority = lowerEventPriority(DefaultEventPriority, renderPriority); const prevTransition = ReactCurrentBatchConfig.transition; @@ -2169,6 +2174,9 @@ function flushPassiveEffectsImpl() { const root = rootWithPendingPassiveEffects; const lanes = pendingPassiveEffectsLanes; rootWithPendingPassiveEffects = null; + // TODO: This is sometimes out of sync with rootWithPendingPassiveEffects. + // Figure out why and fix it. It's not causing any known issues (probably + // because it's only used for profiling), but it's a refactor hazard. pendingPassiveEffectsLanes = NoLanes; invariant(