@samitouri / QOS-React / commits / 1a49b5f563

[Fiber] Fix infinite loop in scheduler and add more tests (#8172)

* Make test more complete * Add a failing test for scheduling in reverse order It hangs forever because we don't clear next pointer when unscheduling a root. Therefore, when it's scheduled again, it brings all its previous chain with it, potentially creating a cycle. * Clear the next pointer when unscheduling a root Fixes a potential infinite cycle when we reschedule a root. * Add new tests to Fiber test tracker

Dan Abramov committed Nov 1, 2016 at 17:44 UTC 1a49b5f563ebcc5ef95bfee5c76dc9adde190797
3 files changed +64 -4
scripts/fiber/tests-passing.txt
+5
@@ -794,6 +794,11 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalReflection-test.js
794 * handles isMounted when an unmount is deferred
795 * finds no node before insertion and correct node before deletion
796
797 +src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js
798 +* schedules multiple roots
799 +* works on deferred roots in the order they were scheduled
800 +* works on roots with animation priority before deferred work
801 +
802 src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js
803 * can update child nodes of a host instance
804 * can update child nodes of a fragment
src/renderers/shared/fiber/ReactFiberScheduler.js
+9 -1
@@ -77,14 +77,22 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
77 function findNextUnitOfWork() {
78 // Clear out roots with no more work on them.
79 while (nextScheduledRoot && nextScheduledRoot.current.pendingWorkPriority === NoWork) {
80 + // Unschedule this root.
81 nextScheduledRoot.isScheduled = false;
82 + // Read the next pointer now.
83 + // We need to clear it in case this root gets scheduled again later.
84 + const next = nextScheduledRoot.nextScheduledRoot;
85 + nextScheduledRoot.nextScheduledRoot = null;
86 + // Exit if we cleared all the roots and there's no work to do.
87 if (nextScheduledRoot === lastScheduledRoot) {
88 nextScheduledRoot = null;
89 lastScheduledRoot = null;
90 nextPriorityLevel = NoWork;
91 return null;
92 }
87 - nextScheduledRoot = nextScheduledRoot.nextScheduledRoot;
93 + // Continue with the next root.
94 + // If there's no work on it, it will get unscheduled too.
95 + nextScheduledRoot = next;
96 }
97 let root = nextScheduledRoot;
98 let highestPriorityRoot = null;
src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js
+50 -3
@@ -70,6 +70,36 @@ describe('ReactIncrementalScheduling', () => {
70 expect(ReactNoop.getChildren('a')).toEqual([span('a:6')]);
71 });
72
73 + it('works on deferred roots in the order they were scheduled', () => {
74 + ReactNoop.renderToRootWithID(<span prop="a:1" />, 'a');
75 + ReactNoop.renderToRootWithID(<span prop="b:1" />, 'b');
76 + ReactNoop.renderToRootWithID(<span prop="c:1" />, 'c');
77 + ReactNoop.flush();
78 + expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]);
79 + expect(ReactNoop.getChildren('b')).toEqual([span('b:1')]);
80 + expect(ReactNoop.getChildren('c')).toEqual([span('c:1')]);
81 +
82 + // Schedule deferred work in the reverse order
83 + ReactNoop.renderToRootWithID(<span prop="c:2" />, 'c');
84 + ReactNoop.renderToRootWithID(<span prop="b:2" />, 'b');
85 + // Ensure it starts in the order it was scheduled
86 + ReactNoop.flushDeferredPri(15);
87 + expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]);
88 + expect(ReactNoop.getChildren('b')).toEqual([span('b:1')]);
89 + expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]);
90 + // Schedule last bit of work, it will get processed the last
91 + ReactNoop.renderToRootWithID(<span prop="a:2" />, 'a');
92 + // Keep performing work in the order it was scheduled
93 + ReactNoop.flushDeferredPri(15);
94 + expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]);
95 + expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]);
96 + expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]);
97 + ReactNoop.flushDeferredPri(15);
98 + expect(ReactNoop.getChildren('a')).toEqual([span('a:2')]);
99 + expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]);
100 + expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]);
101 + });
102 +
103 it('works on roots with animation priority before deferred work', () => {
104 ReactNoop.renderToRootWithID(<span prop="a:1" />, 'a');
105 ReactNoop.renderToRootWithID(<span prop="b:1" />, 'b');
@@ -85,7 +115,8 @@ describe('ReactIncrementalScheduling', () => {
115 ReactNoop.renderToRootWithID(<span prop="b:2" />, 'b');
116 ReactNoop.renderToRootWithID(<span prop="c:2" />, 'c');
117 });
88 - // Handle roots with animation work first
118 + // We're flushing deferred work
119 + // Still, roots with animation work are handled first
120 ReactNoop.flushDeferredPri(15);
121 expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]);
122 expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]);
@@ -94,10 +125,26 @@ describe('ReactIncrementalScheduling', () => {
125 expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]);
126 expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]);
127 expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]);
97 - // Then handle deferred root
128 +
129 + // More deferred and animation work just got scheduled!
130 + ReactNoop.renderToRootWithID(<span prop="c:3" />, 'c');
131 + ReactNoop.performAnimationWork(() => {
132 + ReactNoop.renderToRootWithID(<span prop="b:3" />, 'b');
133 + });
134 + // Animation is still handled first
135 + ReactNoop.flushDeferredPri(15);
136 + expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]);
137 + expect(ReactNoop.getChildren('b')).toEqual([span('b:3')]);
138 + expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]);
139 +
140 + // Finally we handle deferred root in the order it was scheduled
141 ReactNoop.flushDeferredPri(15);
142 expect(ReactNoop.getChildren('a')).toEqual([span('a:2')]);
100 - expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]);
143 + expect(ReactNoop.getChildren('b')).toEqual([span('b:3')]);
144 expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]);
145 + ReactNoop.flushDeferredPri(15);
146 + expect(ReactNoop.getChildren('a')).toEqual([span('a:2')]);
147 + expect(ReactNoop.getChildren('b')).toEqual([span('b:3')]);
148 + expect(ReactNoop.getChildren('c')).toEqual([span('c:3')]);
149 });
150 });