From 23142b0deceba43e278bd6748235c2babdd6ef2e Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 12 Apr 2021 19:49:41 -0500 Subject: [PATCH] Bugfix: Don't rely on `finishedLanes` for passive effects (#21233) 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 0792ac2b29647..b60353ccba874 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2007,7 +2007,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 previousPriority = getCurrentUpdatePriority(); @@ -2042,6 +2047,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 760b971767b49..b9cb967801b1f 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -2007,7 +2007,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 previousPriority = getCurrentUpdatePriority(); @@ -2042,6 +2047,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(