From 43645a658625317d56ce526591bc143ab31b6009 Mon Sep 17 00:00:00 2001
From: Sebastian Markbage <sema@fb.com>
Date: Thu, 6 Oct 2016 19:17:26 -0700
Subject: [PATCH] Reset effect list when we recompute children

We only use the effect list when we reuse our progressed children.
Otherwise it needs to reset to null.

In all other branches, other than bailoutOnLowPriority, we
revisit the children which recreates this list.

We should also not add fibers to their own effect list since it
becomes difficult to perform work on self without touching the
children too. Nothing else does that neither.

Since that means that the root isn't added to an effect list we
need to special case the root.
---
 src/renderers/shared/fiber/ReactFiber.js      |  5 +--
 .../shared/fiber/ReactFiberBeginWork.js       | 11 ++++++
 .../shared/fiber/ReactFiberScheduler.js       | 36 +++++++++++--------
 3 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/src/renderers/shared/fiber/ReactFiber.js b/src/renderers/shared/fiber/ReactFiber.js
index d9e049f7f8..6c1d557325 100644
--- a/src/renderers/shared/fiber/ReactFiber.js
+++ b/src/renderers/shared/fiber/ReactFiber.js
@@ -221,8 +221,9 @@ exports.cloneFiber = function(fiber : Fiber, priorityLevel : PriorityLevel) : Fi
   // extra memory if needed.
   let alt = fiber.alternate;
   if (alt) {
-    // Whenever we clone, we do so to get a new work in progress.
-    // This ensures that we've reset these in the new tree.
+    // If we clone, then we do so from the "current" state. The current state
+    // can't have any side-effects that are still valid so we reset just to be
+    // sure.
     alt.effectTag = NoEffect;
     alt.nextEffect = null;
     alt.firstEffect = null;
diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js
index 932dfcfce7..50b4e0cd2c 100644
--- a/src/renderers/shared/fiber/ReactFiberBeginWork.js
+++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js
@@ -403,9 +403,15 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>, g
 
   function bailoutOnLowPriority(current, workInProgress) {
     if (current) {
+      // TODO: If I have started work on this node, mark it as finished, then
+      // return do other work, come back and hit this node... we killed that
+      // work. It is now in an inconsistent state. We probably need to check
+      // progressedChild or something.
       workInProgress.child = current.child;
       workInProgress.memoizedProps = current.memoizedProps;
       workInProgress.output = current.output;
+      workInProgress.firstEffect = null;
+      workInProgress.lastEffect = null;
     }
     return null;
   }
@@ -416,6 +422,11 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>, g
       return bailoutOnLowPriority(current, workInProgress);
     }
 
+    // If we don't bail out, we're going be recomputing our children so we need
+    // to drop our effect list.
+    workInProgress.firstEffect = null;
+    workInProgress.lastEffect = null;
+
     if (workInProgress.progressedPriority === priorityLevel) {
       // If we have progressed work on this priority level already, we can
       // proceed this that as the child.
diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js
index ce9385c2e3..9b64df10b3 100644
--- a/src/renderers/shared/fiber/ReactFiberScheduler.js
+++ b/src/renderers/shared/fiber/ReactFiberScheduler.js
@@ -143,6 +143,13 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
       effectfulFiber.nextEffect = null;
       effectfulFiber = next;
     }
+
+    // Finally if the root itself had an effect, we perform that since it is not
+    // part of the effect list.
+    if (finishedWork.effectTag !== NoEffect) {
+      const current = finishedWork.alternate;
+      commitWork(current, finishedWork);
+    }
   }
 
   function resetWorkPriority(workInProgress : Fiber) {
@@ -179,20 +186,6 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
       workInProgress.pendingProps = null;
       workInProgress.updateQueue = null;
 
-      // If this fiber had side-effects, we append it to the end of its own
-      // effect list.
-      if (workInProgress.effectTag !== NoEffect) {
-        // Schedule a side-effect on this fiber, AFTER the children's
-        // side-effects. We can perform certain side-effects earlier if
-        // needed, by doing multiple passes over the effect list.
-        if (workInProgress.lastEffect) {
-          workInProgress.lastEffect.nextEffect = workInProgress;
-        } else {
-          workInProgress.firstEffect = workInProgress;
-        }
-        workInProgress.lastEffect = workInProgress;
-      }
-
       const returnFiber = workInProgress.return;
 
       if (returnFiber) {
@@ -208,6 +201,21 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
           }
           returnFiber.lastEffect = workInProgress.lastEffect;
         }
+
+        // If this fiber had side-effects, we append it AFTER the children's
+        // side-effects. We can perform certain side-effects earlier if
+        // needed, by doing multiple passes over the effect list. We don't want
+        // to schedule our own side-effect on our own list because if end up
+        // reusing children we'll schedule this effect onto itself since we're
+        // at the end.
+        if (workInProgress.effectTag !== NoEffect) {
+          if (returnFiber.lastEffect) {
+            returnFiber.lastEffect.nextEffect = workInProgress;
+          } else {
+            returnFiber.firstEffect = workInProgress;
+          }
+          returnFiber.lastEffect = workInProgress;
+        }
       }
 
       if (next) {