Properly reset Profiler nested-update flag (#20253)

Previously this flag was not being reset correctly if a concurrent update followed a nested (sync) update. This PR fixes the behavior and adds a regression test.
facebook · Nov 13, 2020 · 7548dd5 · 7548dd5
1 parent bd8bc5a
commit 7548dd5
Show file tree

Hide file tree

Showing 5 changed files with 74 additions and 0 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -199,6 +199,7 @@ import {
 import {
   markNestedUpdateScheduled,
   recordCommitTime,
+  resetNestedUpdateFlag,
   startProfilerTimer,
   stopProfilerTimerIfRunningAndRecordDelta,
   syncNestedUpdateFlag,
@@ -746,6 +747,10 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
 // This is the entry point for every concurrent task, i.e. anything that
 // goes through Scheduler.
 function performConcurrentWorkOnRoot(root) {
+  if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
+    resetNestedUpdateFlag();
+  }
+
   // Since we know we're in a React event, we can clear the current
   // event time. The next update will compute a new event time.
   currentEventTime = NoTimestamp;

diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
@@ -213,6 +213,7 @@ import {
   markNestedUpdateScheduled,
   recordCommitTime,
   recordPassiveEffectDuration,
+  resetNestedUpdateFlag,
   startPassiveEffectTimer,
   startProfilerTimer,
   stopProfilerTimerIfRunningAndRecordDelta,
@@ -770,6 +771,10 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
 // This is the entry point for every concurrent task, i.e. anything that
 // goes through Scheduler.
 function performConcurrentWorkOnRoot(root) {
+  if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
+    resetNestedUpdateFlag();
+  }
+
   // Since we know we're in a React event, we can clear the current
   // event time. The next update will compute a new event time.
   currentEventTime = NoTimestamp;

diff --git a/packages/react-reconciler/src/ReactProfilerTimer.new.js b/packages/react-reconciler/src/ReactProfilerTimer.new.js
@@ -68,6 +68,13 @@ function markNestedUpdateScheduled(): void {
   }
 }
 
+function resetNestedUpdateFlag(): void {
+  if (enableProfilerNestedUpdatePhase) {
+    currentUpdateIsNested = false;
+    nestedUpdateScheduled = false;
+  }
+}
+
 function syncNestedUpdateFlag(): void {
   if (enableProfilerNestedUpdatePhase) {
     currentUpdateIsNested = nestedUpdateScheduled;
@@ -206,6 +213,7 @@ export {
   recordCommitTime,
   recordLayoutEffectDuration,
   recordPassiveEffectDuration,
+  resetNestedUpdateFlag,
   startLayoutEffectTimer,
   startPassiveEffectTimer,
   startProfilerTimer,

diff --git a/packages/react-reconciler/src/ReactProfilerTimer.old.js b/packages/react-reconciler/src/ReactProfilerTimer.old.js
@@ -68,6 +68,13 @@ function markNestedUpdateScheduled(): void {
   }
 }
 
+function resetNestedUpdateFlag(): void {
+  if (enableProfilerNestedUpdatePhase) {
+    currentUpdateIsNested = false;
+    nestedUpdateScheduled = false;
+  }
+}
+
 function syncNestedUpdateFlag(): void {
   if (enableProfilerNestedUpdatePhase) {
     currentUpdateIsNested = nestedUpdateScheduled;
@@ -206,6 +213,7 @@ export {
   recordCommitTime,
   recordLayoutEffectDuration,
   recordPassiveEffectDuration,
+  resetNestedUpdateFlag,
   startLayoutEffectTimer,
   startPassiveEffectTimer,
   startProfilerTimer,

diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js
@@ -700,6 +700,54 @@ describe('Profiler', () => {
         expect(updateCall[5]).toBe(43); // commit time
       });
 
+      it('should clear nested-update flag when multiple cascading renders are scheduled', () => {
+        loadModules({
+          enableSchedulerTracing,
+          useNoopRenderer: true,
+        });
+
+        function Component() {
+          const [didMount, setDidMount] = React.useState(false);
+          const [didMountAndUpdate, setDidMountAndUpdate] = React.useState(
+            false,
+          );
+
+          React.useLayoutEffect(() => {
+            setDidMount(true);
+          }, []);
+
+          React.useEffect(() => {
+            if (didMount && !didMountAndUpdate) {
+              setDidMountAndUpdate(true);
+            }
+          }, [didMount, didMountAndUpdate]);
+
+          Scheduler.unstable_yieldValue(`${didMount}:${didMountAndUpdate}`);
+
+          return null;
+        }
+
+        const onRender = jest.fn();
+
+        ReactNoop.act(() => {
+          ReactNoop.render(
+            <React.Profiler id="root" onRender={onRender}>
+              <Component />
+            </React.Profiler>,
+          );
+        });
+        expect(Scheduler).toHaveYielded([
+          'false:false',
+          'true:false',
+          'true:true',
+        ]);
+
+        expect(onRender).toHaveBeenCalledTimes(3);
+        expect(onRender.mock.calls[0][1]).toBe('mount');
+        expect(onRender.mock.calls[1][1]).toBe('nested-update');
+        expect(onRender.mock.calls[2][1]).toBe('update');
+      });
+
       describe('with regard to interruptions', () => {
         it('should accumulate actual time after a scheduling interruptions', () => {
           const callback = jest.fn();