Fix a portal unmounting crash for renderers with distinct Instance and Container (#13220)
* Fix Portal unmount Before that change, currentParent is not set as a container even if it should so it break on react-native and probably other custom renderers * Assert that *ToContainer() methods receive containers * Add regression tests * Add comments
Thibault Malbranche committed
Jul 17, 2018 at 02:35 UTC
21ac62c77ac68c66b2a1f0adcc60f7760b884a0e
4 files changed
+255
-31
packages/react-noop-renderer/src/createReactNoop.js
+112
-27
@@ -45,8 +45,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
45
let failInBeginPhase = false;
46
let failInCompletePhase = false;
47
48
- function appendChild(
49
- parentInstance: Instance | Container,
48
+ function appendChildToContainerOrInstance(
49
+ parentInstance: Container | Instance,
50
child: Instance | TextInstance,
51
): void {
52
const index = parentInstance.children.indexOf(child);
@@ -56,8 +56,34 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
56
parentInstance.children.push(child);
57
}
58
59
- function insertBefore(
60
- parentInstance: Instance | Container,
59
+ function appendChildToContainer(
60
+ parentInstance: Container,
61
+ child: Instance | TextInstance,
62
+ ): void {
63
+ if (typeof parentInstance.rootID !== 'string') {
64
+ // Some calls to this aren't typesafe.
65
+ // This helps surface mistakes in tests.
66
+ throw new Error(
67
+ 'appendChildToContainer() first argument is not a container.',
68
+ );
69
+ }
70
+ appendChildToContainerOrInstance(parentInstance, child);
71
+ }
72
+
73
+ function appendChild(
74
+ parentInstance: Instance,
75
+ child: Instance | TextInstance,
76
+ ): void {
77
+ if (typeof (parentInstance: any).rootID === 'string') {
78
+ // Some calls to this aren't typesafe.
79
+ // This helps surface mistakes in tests.
80
+ throw new Error('appendChild() first argument is not an instance.');
81
+ }
82
+ appendChildToContainerOrInstance(parentInstance, child);
83
+ }
84
+
85
+ function insertInContainerOrInstanceBefore(
86
+ parentInstance: Container | Instance,
87
child: Instance | TextInstance,
88
beforeChild: Instance | TextInstance,
89
): void {
@@ -72,8 +98,36 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
98
parentInstance.children.splice(beforeIndex, 0, child);
99
}
100
75
- function removeChild(
76
- parentInstance: Instance | Container,
101
+ function insertInContainerBefore(
102
+ parentInstance: Container,
103
+ child: Instance | TextInstance,
104
+ beforeChild: Instance | TextInstance,
105
+ ) {
106
+ if (typeof parentInstance.rootID !== 'string') {
107
+ // Some calls to this aren't typesafe.
108
+ // This helps surface mistakes in tests.
109
+ throw new Error(
110
+ 'insertInContainerBefore() first argument is not a container.',
111
+ );
112
+ }
113
+ insertInContainerOrInstanceBefore(parentInstance, child, beforeChild);
114
+ }
115
+
116
+ function insertBefore(
117
+ parentInstance: Instance,
118
+ child: Instance | TextInstance,
119
+ beforeChild: Instance | TextInstance,
120
+ ) {
121
+ if (typeof (parentInstance: any).rootID === 'string') {
122
+ // Some calls to this aren't typesafe.
123
+ // This helps surface mistakes in tests.
124
+ throw new Error('insertBefore() first argument is not an instance.');
125
+ }
126
+ insertInContainerOrInstanceBefore(parentInstance, child, beforeChild);
127
+ }
128
+
129
+ function removeChildFromContainerOrInstance(
130
+ parentInstance: Container | Instance,
131
child: Instance | TextInstance,
132
): void {
133
const index = parentInstance.children.indexOf(child);
@@ -83,6 +137,32 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
137
parentInstance.children.splice(index, 1);
138
}
139
140
+ function removeChildFromContainer(
141
+ parentInstance: Container,
142
+ child: Instance | TextInstance,
143
+ ): void {
144
+ if (typeof parentInstance.rootID !== 'string') {
145
+ // Some calls to this aren't typesafe.
146
+ // This helps surface mistakes in tests.
147
+ throw new Error(
148
+ 'removeChildFromContainer() first argument is not a container.',
149
+ );
150
+ }
151
+ removeChildFromContainerOrInstance(parentInstance, child);
152
+ }
153
+
154
+ function removeChild(
155
+ parentInstance: Instance,
156
+ child: Instance | TextInstance,
157
+ ): void {
158
+ if (typeof (parentInstance: any).rootID === 'string') {
159
+ // Some calls to this aren't typesafe.
160
+ // This helps surface mistakes in tests.
161
+ throw new Error('removeChild() first argument is not an instance.');
162
+ }
163
+ removeChildFromContainerOrInstance(parentInstance, child);
164
+ }
165
+
166
let elapsedTimeInMs = 0;
167
168
const sharedHostConfig = {
@@ -234,12 +314,12 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
314
textInstance.text = newText;
315
},
316
237
- appendChild: appendChild,
238
- appendChildToContainer: appendChild,
239
- insertBefore: insertBefore,
240
- insertInContainerBefore: insertBefore,
241
- removeChild: removeChild,
242
- removeChildFromContainer: removeChild,
317
+ appendChild,
318
+ appendChildToContainer,
319
+ insertBefore,
320
+ insertInContainerBefore,
321
+ removeChild,
322
+ removeChildFromContainer,
323
324
resetTextContent(instance: Instance): void {},
325
}
@@ -348,6 +428,20 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
428
}
429
},
430
431
+ getOrCreateRootContainer(
432
+ rootID: string = DEFAULT_ROOT_ID,
433
+ isAsync: boolean = false,
434
+ ) {
435
+ let root = roots.get(rootID);
436
+ if (!root) {
437
+ const container = {rootID: rootID, children: []};
438
+ rootContainers.set(rootID, container);
439
+ root = NoopRenderer.createContainer(container, isAsync, false);
440
+ roots.set(rootID, root);
441
+ }
442
+ return root.current.stateNode.containerInfo;
443
+ },
444
+
445
createPortal(
446
children: ReactNodeList,
447
container: Container,
@@ -363,14 +457,9 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
457
458
renderLegacySyncRoot(element: React$Element<any>, callback: ?Function) {
459
const rootID = DEFAULT_ROOT_ID;
366
- let root = roots.get(rootID);
367
- if (!root) {
368
- const container = {rootID: rootID, children: []};
369
- rootContainers.set(rootID, container);
370
- const isAsync = false;
371
- root = NoopRenderer.createContainer(container, isAsync, false);
372
- roots.set(rootID, root);
373
- }
460
+ const isAsync = false;
461
+ const container = ReactNoop.getOrCreateRootContainer(rootID, isAsync);
462
+ const root = roots.get(container.rootID);
463
NoopRenderer.updateContainer(element, root, null, callback);
464
},
465
@@ -379,13 +468,9 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
468
rootID: string,
469
callback: ?Function,
470
) {
382
- let root = roots.get(rootID);
383
- if (!root) {
384
- const container = {rootID: rootID, children: []};
385
- rootContainers.set(rootID, container);
386
- root = NoopRenderer.createContainer(container, true, false);
387
- roots.set(rootID, root);
388
- }
471
+ const isAsync = true;
472
+ const container = ReactNoop.getOrCreateRootContainer(rootID, isAsync);
473
+ const root = roots.get(container.rootID);
474
NoopRenderer.updateContainer(element, root, null, callback);
475
},
476
packages/react-reconciler/src/ReactFiberCommitWork.js
+7
-1
@@ -598,8 +598,11 @@ function commitPlacement(finishedWork: Fiber): void {
598
599
// Recursively insert all host nodes into the parent.
600
const parentFiber = getHostParentFiber(finishedWork);
601
+
602
+ // Note: these two variables *must* always be updated together.
603
let parent;
604
let isContainer;
605
+
606
switch (parentFiber.tag) {
607
case HostComponent:
608
parent = parentFiber.stateNode;
@@ -670,13 +673,15 @@ function commitPlacement(finishedWork: Fiber): void {
673
}
674
675
function unmountHostComponents(current): void {
673
- // We only have the top Fiber that was inserted but we need recurse down its
676
+ // We only have the top Fiber that was deleted but we need recurse down its
677
// children to find all the terminal nodes.
678
let node: Fiber = current;
679
680
// Each iteration, currentParent is populated with node's host parent if not
681
// currentParentIsValid.
682
let currentParentIsValid = false;
683
+
684
+ // Note: these two variables *must* always be updated together.
685
let currentParent;
686
let currentParentIsContainer;
687
@@ -722,6 +727,7 @@ function unmountHostComponents(current): void {
727
// When we go into a portal, it becomes the parent to remove from.
728
// We will reassign it back when we pop the portal on the way up.
729
currentParent = node.stateNode.containerInfo;
730
+ currentParentIsContainer = true;
731
// Visit children because portals might contain host components.
732
if (node.child !== null) {
733
node.child.return = node;
packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.internal.js
+6
-2
@@ -539,9 +539,13 @@ describe('ReactDebugFiberPerf', () => {
539
});
540
541
it('supports portals', () => {
542
- const noopContainer = {children: []};
542
+ const portalContainer = ReactNoop.getOrCreateRootContainer(
543
+ 'portalContainer',
544
+ );
545
ReactNoop.render(
544
- <Parent>{ReactNoop.createPortal(<Child />, noopContainer, null)}</Parent>,
546
+ <Parent>
547
+ {ReactNoop.createPortal(<Child />, portalContainer, null)}
548
+ </Parent>,
549
);
550
ReactNoop.flush();
551
expect(getFlameChart()).toMatchSnapshot();
packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.internal.js
+130
-1
@@ -24,7 +24,7 @@ describe('ReactIncrementalSideEffects', () => {
24
});
25
26
function div(...children) {
27
- children = children.map(c => (typeof c === 'string' ? {text: c} : c));
27
+ children = children.map(c => (typeof c === 'string' ? text(c) : c));
28
return {type: 'div', children, prop: undefined};
29
}
30
@@ -32,6 +32,10 @@ describe('ReactIncrementalSideEffects', () => {
32
return {type: 'span', children: [], prop};
33
}
34
35
+ function text(t) {
36
+ return {text: t};
37
+ }
38
+
39
it('can update child nodes of a host instance', () => {
40
function Bar(props) {
41
return <span>{props.text}</span>;
@@ -240,6 +244,131 @@ describe('ReactIncrementalSideEffects', () => {
244
expect(ReactNoop.getChildren()).toEqual([div('Trail')]);
245
});
246
247
+ it('can delete a child when it unmounts inside a portal', () => {
248
+ function Bar(props) {
249
+ return <span prop={props.children} />;
250
+ }
251
+
252
+ const portalContainer = ReactNoop.getOrCreateRootContainer(
253
+ 'portalContainer',
254
+ );
255
+ function Foo(props) {
256
+ return ReactNoop.createPortal(
257
+ props.show ? [<div key="a" />, <Bar key="b">Hello</Bar>, 'World'] : [],
258
+ portalContainer,
259
+ null,
260
+ );
261
+ }
262
+
263
+ ReactNoop.render(
264
+ <div>
265
+ <Foo show={true} />
266
+ </div>,
267
+ );
268
+ ReactNoop.flush();
269
+ expect(ReactNoop.getChildren()).toEqual([div()]);
270
+ expect(ReactNoop.getChildren('portalContainer')).toEqual([
271
+ div(),
272
+ span('Hello'),
273
+ text('World'),
274
+ ]);
275
+
276
+ ReactNoop.render(
277
+ <div>
278
+ <Foo show={false} />
279
+ </div>,
280
+ );
281
+ ReactNoop.flush();
282
+ expect(ReactNoop.getChildren()).toEqual([div()]);
283
+ expect(ReactNoop.getChildren('portalContainer')).toEqual([]);
284
+
285
+ ReactNoop.render(
286
+ <div>
287
+ <Foo show={true} />
288
+ </div>,
289
+ );
290
+ ReactNoop.flush();
291
+ expect(ReactNoop.getChildren()).toEqual([div()]);
292
+ expect(ReactNoop.getChildren('portalContainer')).toEqual([
293
+ div(),
294
+ span('Hello'),
295
+ text('World'),
296
+ ]);
297
+
298
+ ReactNoop.render(null);
299
+ ReactNoop.flush();
300
+ expect(ReactNoop.getChildren()).toEqual([]);
301
+ expect(ReactNoop.getChildren('portalContainer')).toEqual([]);
302
+
303
+ ReactNoop.render(<Foo show={false} />);
304
+ ReactNoop.flush();
305
+ expect(ReactNoop.getChildren()).toEqual([]);
306
+ expect(ReactNoop.getChildren('portalContainer')).toEqual([]);
307
+
308
+ ReactNoop.render(<Foo show={true} />);
309
+ ReactNoop.flush();
310
+ expect(ReactNoop.getChildren()).toEqual([]);
311
+ expect(ReactNoop.getChildren('portalContainer')).toEqual([
312
+ div(),
313
+ span('Hello'),
314
+ text('World'),
315
+ ]);
316
+
317
+ ReactNoop.render(null);
318
+ ReactNoop.flush();
319
+ expect(ReactNoop.getChildren()).toEqual([]);
320
+ expect(ReactNoop.getChildren('portalContainer')).toEqual([]);
321
+ });
322
+
323
+ it('can delete a child when it unmounts with a portal', () => {
324
+ function Bar(props) {
325
+ return <span prop={props.children} />;
326
+ }
327
+
328
+ const portalContainer = ReactNoop.getOrCreateRootContainer(
329
+ 'portalContainer',
330
+ );
331
+ function Foo(props) {
332
+ return ReactNoop.createPortal(
333
+ [<div key="a" />, <Bar key="b">Hello</Bar>, 'World'],
334
+ portalContainer,
335
+ null,
336
+ );
337
+ }
338
+
339
+ ReactNoop.render(
340
+ <div>
341
+ <Foo />
342
+ </div>,
343
+ );
344
+ ReactNoop.flush();
345
+ expect(ReactNoop.getChildren()).toEqual([div()]);
346
+ expect(ReactNoop.getChildren('portalContainer')).toEqual([
347
+ div(),
348
+ span('Hello'),
349
+ text('World'),
350
+ ]);
351
+
352
+ ReactNoop.render(null);
353
+ ReactNoop.flush();
354
+ expect(ReactNoop.getChildren()).toEqual([]);
355
+ expect(ReactNoop.getChildren('portalContainer')).toEqual([]);
356
+
357
+ ReactNoop.render(<Foo />);
358
+ ReactNoop.flush();
359
+ expect(ReactNoop.getChildren()).toEqual([]);
360
+ expect(ReactNoop.getChildren('portalContainer')).toEqual([
361
+ div(),
362
+ span('Hello'),
363
+ text('World'),
364
+ ]);
365
+
366
+ ReactNoop.render(null);
367
+ ReactNoop.flush();
368
+ expect(ReactNoop.getChildren()).toEqual([]);
369
+ expect(ReactNoop.getChildren('portalContainer')).toEqual([]);
370
+ });
371
+
372
it('does not update child nodes if a flush is aborted', () => {
373
function Bar(props) {
374
return <span prop={props.text} />;