@samitouri / QOS-React / commits / bb88ce95a8

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.

Andrew Clark committed Apr 11, 2021 at 04:09 UTC bb88ce95a87934a655ef842af776c164391131ac

2 files changed +18 -2

packages/react-reconciler/src/ReactFiberWorkLoop.new.js

+9 -1

@@ -2131,7 +2131,12 @@ function commitRootImpl(root, renderPriorityLevel) {

2131

2132 export function flushPassiveEffects(): boolean {

2133 // Returns whether passive effects were flushed.

2134 - if (pendingPassiveEffectsLanes !== NoLanes) {

2134 + // TODO: Combine this check with the one in flushPassiveEFfectsImpl. We should

2135 + // probably just combine the two functions. I believe they were only separate

2136 + // in the first place because we used to wrap it with

2137 + // `Scheduler.runWithPriority`, which accepts a function. But now we track the

2138 + // priority within React itself, so we can mutate the variable directly.

2139 + if (rootWithPendingPassiveEffects !== null) {

2140 const renderPriority = lanesToEventPriority(pendingPassiveEffectsLanes);

2141 const priority = lowerEventPriority(DefaultEventPriority, renderPriority);

2142 const prevTransition = ReactCurrentBatchConfig.transition;

@@ -2169,6 +2174,9 @@ function flushPassiveEffectsImpl() {

2174 const root = rootWithPendingPassiveEffects;

2175 const lanes = pendingPassiveEffectsLanes;

2176 rootWithPendingPassiveEffects = null;

2177 + // TODO: This is sometimes out of sync with rootWithPendingPassiveEffects.

2178 + // Figure out why and fix it. It's not causing any known issues (probably

2179 + // because it's only used for profiling), but it's a refactor hazard.

2180 pendingPassiveEffectsLanes = NoLanes;

2181

2182 invariant(

packages/react-reconciler/src/ReactFiberWorkLoop.old.js