Fix unwinding starting with a wrong Fiber on error in the complete phase (#13237)
* Add a repro case for profiler unwinding This currently fails the tests due to an unexpected warning. * Add a regression test for context stack * Simplify the first test case * Update nextUnitOfWork inside completeUnitOfWork() The bug was caused by a structure like this: </Provider> </div> </errorInCompletePhase> We forgot to update nextUnitOfWork so it was still pointing at Provider when errorInCompletePhase threw. As a result, we would try to unwind from Provider (rather than from errorInCompletePhase), and thus pop the Provider twice.
Dan Abramov committed
Jul 19, 2018 at 02:16 UTC
2c560cb9952c05fb0a45aed87723c9a8bf6ff8d4
3 files changed
+34
-1
packages/react-reconciler/src/ReactFiberScheduler.js
+2
-1
@@ -791,11 +791,12 @@ function completeUnitOfWork(workInProgress: Fiber): Fiber | null {
791
792
if ((workInProgress.effectTag & Incomplete) === NoEffect) {
793
// This fiber completed.
794
- let next = completeWork(
794
+ nextUnitOfWork = completeWork(
795
current,
796
workInProgress,
797
nextRenderExpirationTime,
798
);
799
+ let next = nextUnitOfWork;
800
stopWorkTimer(workInProgress);
801
resetExpirationTime(workInProgress, nextRenderExpirationTime);
802
if (__DEV__) {
packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js
+21
@@ -1128,6 +1128,27 @@ describe('ReactNewContext', () => {
1128
]);
1129
});
1130
1131
+ it('unwinds after errors in complete phase', () => {
1132
+ const Context = React.createContext(0);
1133
+
1134
+ // This is a regression test for stack misalignment
1135
+ // caused by unwinding the context from wrong point.
1136
+ ReactNoop.render(
1137
+ <errorInCompletePhase>
1138
+ <Context.Provider />
1139
+ </errorInCompletePhase>,
1140
+ );
1141
+ expect(ReactNoop.flush).toThrow('Error in host config.');
1142
+
1143
+ ReactNoop.render(
1144
+ <Context.Provider value={10}>
1145
+ <Context.Consumer>{value => <span prop={value} />}</Context.Consumer>
1146
+ </Context.Provider>,
1147
+ );
1148
+ ReactNoop.flush();
1149
+ expect(ReactNoop.getChildren()).toEqual([span(10)]);
1150
+ });
1151
+
1152
describe('fuzz test', () => {
1153
const Fragment = React.Fragment;
1154
const contextKeys = ['A', 'B', 'C', 'D', 'E', 'F', 'G'];
packages/react/src/__tests__/ReactProfiler-test.internal.js
+11
@@ -999,6 +999,17 @@ describe('Profiler', () => {
999
);
1000
expect(ReactNoop.flush).toThrow('Error in host config.');
1001
1002
+ // A similar case we've seen caused by an invariant in ReactDOM.
1003
+ // It didn't reproduce without a host component inside.
1004
+ ReactNoop.render(
1005
+ <React.unstable_Profiler id="profiler" onRender={jest.fn()}>
1006
+ <errorInCompletePhase>
1007
+ <span>hi</span>
1008
+ </errorInCompletePhase>
1009
+ </React.unstable_Profiler>,
1010
+ );
1011
+ expect(ReactNoop.flush).toThrow('Error in host config.');
1012
+
1013
// So long as the profiler timer's fiber stack is reset correctly,
1014
// Subsequent renders should not error.
1015
ReactNoop.render(