Bugfix: Don't rely on finishedLanes for passive effects (facebook#2…

…1233)

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.

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(

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(