Errors thrown when detaching a ref should not interrupt unmount

If a ref throws while detaching, it should not prevent componentWillUnmount from being called. Additionally, errors thrown by removeChild should not be ignored, even as the result of unmounting a failed subtree.

Author: acdlite

scripts/fiber/tests-passing.txt

@@ -1119,6 +1119,7 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js
 * catches reconciler errors in a boundary during update
 * recovers from uncaught reconciler errors
 * unmounts components with uncaught errors
+* does not interrupt unmounting if detaching a ref throws
 
 src/renderers/shared/fiber/__tests__/ReactIncrementalReflection-test.js
 * handles isMounted even when the initial render is deferred

src/renderers/shared/fiber/ReactFiberCommitWork.js

@@ -46,13 +46,28 @@ module.exports = function<T, P, I, TI, C>(
  const insertBefore = config.insertBefore;
  const removeChild = config.removeChild;
 
- function detachRef(current : Fiber) {
- const ref = current.ref;
- if (ref) {
- ref(null);
+ // Capture errors so they don't interrupt unmounting.
+ function safelyCallComponentWillUnmount(current, instance) {
+ try {
+ instance.componentWillUnmount();
+ } catch (error) {
+ captureError(current, error);
+ }
+ }
+
+ // Capture errors so they don't interrupt unmounting.
+ function safelyDetachRef(current : Fiber) {
+ try {
+ const ref = current.ref;
+ if (ref) {
+ ref(null);
+ }
+ } catch (error) {
+ captureError(current, error);
  }
  }
 
+ // Only called during update. It's ok to throw.
  function detachRefIfNeeded(current : ?Fiber, finishedWork : Fiber) {
  if (current) {
  const currentRef = current.ref;
@@ -298,21 +313,21 @@ module.exports = function<T, P, I, TI, C>(
  }
  }
 
+ // User-originating errors (lifecycles and refs) should not interrupt
+ // deletion, so don't let them throw. Host-originating errors should
+ // interrupt deletion, so it's okay
  function commitUnmount(current : Fiber) : void {
  switch (current.tag) {
  case ClassComponent: {
- detachRef(current);
+ safelyDetachRef(current);
  const instance = current.stateNode;
  if (typeof instance.componentWillUnmount === 'function') {
- const error = tryCallComponentWillUnmount(instance);
- if (error) {
- captureError(current, error);
- }
+ safelyCallComponentWillUnmount(current, instance);
  }
  return;
  }
  case HostComponent: {
- detachRef(current);
+ safelyDetachRef(current);
  return;
  }
  case CoroutineComponent: {
@@ -423,15 +438,6 @@ module.exports = function<T, P, I, TI, C>(
  }
  }
 
- function tryCallComponentWillUnmount(instance) {
- try {
- instance.componentWillUnmount();
- return null;
- } catch (error) {
- return error;
- }
- }
-
  return {
  commitPlacement,
  commitDeletion,

src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js

@@ -21,6 +21,11 @@ describe('ReactIncrementalErrorHandling', () => {
  ReactNoop = require('ReactNoop');
  });
 
+ function div(...children) {
+ children = children.map(c => typeof c === 'string' ? { text: c } : c);
+ return { type: 'div', children, prop: undefined };
+ }
+
  function span(prop) {
  return { type: 'span', children: [], prop };
  }
@@ -860,4 +865,55 @@ describe('ReactIncrementalErrorHandling', () => {
  ]);
  expect(ReactNoop.getChildren()).toEqual([]);
  });
+
+ it('does not interrupt unmounting if detaching a ref throws', () => {
+ var ops = [];
+
+ class Bar extends React.Component {
+ componentWillUnmount() {
+ ops.push('Bar unmount');
+ }
+ render() {
+ return <span prop="Bar" />;
+ }
+ }
+
+ function barRef(inst) {
+ if (inst === null) {
+ ops.push('barRef detach');
+ throw new Error('Detach error');
+ }
+ ops.push('barRef attach');
+ }
+
+ function Foo(props) {
+ return (
+ <div>
+ {props.hide ? null : <Bar ref={barRef} />}
+ </div>
+ );
+ }
+
+ ReactNoop.render(<Foo />);
+ ReactNoop.flush();
+ expect(ops).toEqual(['barRef attach']);
+ expect(ReactNoop.getChildren()).toEqual([
+ div(
+ span('Bar'),
+ ),
+ ]);
+
+ ops = [];
+
+ // Unmount
+ ReactNoop.render(<Foo hide={true} />);
+ expect(() => ReactNoop.flush()).toThrow('Detach error');
+ expect(ops).toEqual([
+ 'barRef detach',
+ // Bar should unmount even though its ref threw an error while detaching
+ 'Bar unmount',
+ ]);
+ // Because there was an error, entire tree should unmount
+ expect(ReactNoop.getChildren()).toEqual([]);
+ });
 });