Skip to content

Commit

Permalink
Bugfix: Don't rely on finishedLanes for passive effects (#21233)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
acdlite authored Apr 11, 2021
1 parent 343710c commit bb88ce9
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 2 deletions.
10 changes: 9 additions & 1 deletion packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand Down
10 changes: 9 additions & 1 deletion packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand Down

0 comments on commit bb88ce9

Please sign in to comment.