Add error handling for Element lifecycle user callbacks (#173148)

This is for #172289, but since this fix is speculative so I'll wait for
the confirmation from the original reporters before closing the issue.

As a bonus this fixes #65655

The framework Element rebuild logic relies heavily on
`Element._lifecycleState` being correct. When a user-supplied lifecycle
callback (e.g., `State.deactivate`) fails the framework currently only
ensures that every `Element` in the tree has the right lifecycle state,
so an out-of-tree `Element` that is supposed to be disposed may still
have an `active` state and continue being rebuilt by the BuildScope
(because it's in the dirty list). See the comments in #172289

Also related:
#100777
Internal:
b/425298525 b/431537277
b/300829376 b/415724119 b/283614822

# TODO (in a different PR)
The original issue could also be caused by incorrect
`Element.updateChild` calls. If an `Element` subclass calls
`Element.updateChild` to add child but forgets to update its child list
accordingly (such that `visitChildren` misses that child), you'll get a
child Element that thinks it's a child of the parent but the parent
doesn't recognize the child so won't take that child into account during
reparenting or unmounting. This is a programmer error that we should try
to catch in the framework.

## Pre-launch Checklist

- [ ] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [ ] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [ ] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [ ] I signed the [CLA].
- [ ] I listed at least one issue that this PR fixes in the description
above.
- [ ] I updated/added relevant documentation (doc comments with `///`).
- [ ] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [ ] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [ ] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

**Note**: The Flutter team is currently trialing the use of [Gemini Code
Assist for
GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code).
Comments from the `gemini-code-assist` bot should not be taken as
authoritative feedback from the Flutter team. If you find its comments
useful you can update your code accordingly, but if you are unsure or
disagree with the feedback, please feel free to wait for a Flutter team
member's review for guidance on which automated comments should be
addressed.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md

packages/flutter/lib/src/rendering/object.dart

@@ -2059,7 +2059,7 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge
     assert(child._parent == this);
     assert(child.attached == attached);
     assert(child.parentData != null);
-    if (!(_isRelayoutBoundary ?? true)) {
+    if (!(child._isRelayoutBoundary ?? true)) {
       child._isRelayoutBoundary = null;
     }
     child.parentData!.detach();

packages/flutter/lib/src/widgets/framework.dart

@@ -2053,13 +2053,54 @@ abstract class MultiChildRenderObjectWidget extends RenderObjectWidget {
 
 // ELEMENTS
 
-enum _ElementLifecycle { initial, active, inactive, defunct }
+enum _ElementLifecycle {
+  /// The [Element] is created but has not yet been incorporated into the element
+  /// tree.
+  initial,
+
+  /// The [Element] is incorporated into the Element tree, either via
+  /// [Element.mount] or [Element.activate].
+  active,
+
+  /// The previously `active` [Element] is removed from the Element tree via
+  /// [Element.deactivate].
+  ///
+  /// This [Element] may become `active` again if a parent reclaims it using
+  /// a [GlobalKey], or `defunct` if no parent reclaims it at the end of the
+  /// build phase.
+  inactive,
+
+  /// The [Element] encountered an unrecoverable error while being rebuilt when it
+  /// was `active` or while being incorporated in the tree.
+  ///
+  /// This indicates the [Element]'s subtree is in an inconsistent state and must
+  /// not be re-incorporated into the tree again.
+  ///
+  /// When an unrecoverable error is encountered, the framework calls
+  /// [Element.deactivate] on this [Element] and sets its state to `failed`. This
+  /// process is done on a best-effort basis and does not surface any additional
+  /// errors.
+  ///
+  /// This is one of the two final stages of the element lifecycle and is not
+  /// reversible. Reaching this state typically means that a widget implementation
+  /// is throwing unhandled exceptions that need to be properly handled.
+  failed,
+
+  /// The [Element] is disposed and should not be interacted with.
+  ///
+  /// The [Element] must be `inactive` before transitioning into this state,
+  /// and the state transition occurs in [BuildOwner.finalizeTree] which signals
+  /// the end of the build phase.
+  ///
+  /// This is the final stage of the element lifecycle and is not reversible.
+  defunct,
+}
 
 class _InactiveElements {
   bool _locked = false;
   final Set<Element> _elements = HashSet<Element>();
 
-  void _unmount(Element element) {
+  static void _unmount(Element element) {
     assert(element._lifecycleState == _ElementLifecycle.inactive);
     assert(() {
       if (debugPrintGlobalKeyedWidgetLifecycle) {
@@ -2091,8 +2132,12 @@ class _InactiveElements {
 
   static void _deactivateRecursively(Element element) {
     assert(element._lifecycleState == _ElementLifecycle.active);
-    element.deactivate();
-    assert(element._lifecycleState == _ElementLifecycle.inactive);
+    try {
+      element.deactivate();
+    } catch (_) {
+      Element._deactivateFailedSubtreeRecursively(element);
+      rethrow;
+    }
     element.visitChildren(_deactivateRecursively);
     assert(() {
       element.debugDeactivated();
@@ -2104,10 +2149,18 @@ class _InactiveElements {
     assert(!_locked);
     assert(!_elements.contains(element));
     assert(element._parent == null);
-    if (element._lifecycleState == _ElementLifecycle.active) {
-      _deactivateRecursively(element);
+
+    switch (element._lifecycleState) {
+      case _ElementLifecycle.active:
+        _deactivateRecursively(element);
+        // This element is only added to _elements if the whole subtree is
+        // successfully deactivated.
+        _elements.add(element);
+      case _ElementLifecycle.inactive:
+        _elements.add(element);
+      case _ElementLifecycle.initial || _ElementLifecycle.failed || _ElementLifecycle.defunct:
+        assert(false, '$element must not be deactivated when in ${element._lifecycleState} state.');
     }
-    _elements.add(element);
   }
 
   void remove(Element element) {
@@ -2115,7 +2168,7 @@ class _InactiveElements {
     assert(_elements.contains(element));
     assert(element._parent == null);
     _elements.remove(element);
-    assert(element._lifecycleState != _ElementLifecycle.active);
+    assert(element._lifecycleState == _ElementLifecycle.inactive);
   }
 
   bool debugContains(Element element) {
@@ -2934,7 +2987,7 @@ class BuildOwner {
     buildScope._scheduleBuildFor(element);
     assert(() {
       if (debugPrintScheduleBuildForStacks) {
-        debugPrint("...the build scope's dirty list is now: $buildScope._dirtyElements");
+        debugPrint("...the build scope's dirty list is now: ${buildScope._dirtyElements}");
       }
       return true;
     }());
@@ -4516,39 +4569,32 @@ abstract class Element extends DiagnosticableTree implements BuildContext {
 
     try {
       final Key? key = newWidget.key;
-      if (key is GlobalKey) {
-        final Element? newChild = _retakeInactiveElement(key, newWidget);
-        if (newChild != null) {
-          assert(newChild._parent == null);
-          assert(() {
-            _debugCheckForCycles(newChild);
-            return true;
-          }());
-          try {
-            newChild._activateWithParent(this, newSlot);
-          } catch (_) {
-            // Attempt to do some clean-up if activation fails to leave tree in a reasonable state.
-            try {
-              deactivateChild(newChild);
-            } catch (_) {
-              // Clean-up failed. Only surface original exception.
-            }
-            rethrow;
-          }
-          final Element? updatedChild = updateChild(newChild, newWidget, newSlot);
-          assert(newChild == updatedChild);
-          return updatedChild!;
-        }
-      }
-      final Element newChild = newWidget.createElement();
+      final Element? inactiveChild = key is GlobalKey
+          ? _retakeInactiveElement(key, newWidget)
+          : null;
+      final Element newChild = inactiveChild ?? newWidget.createElement();
       assert(() {
         _debugCheckForCycles(newChild);
         return true;
       }());
-      newChild.mount(this, newSlot);
-      assert(newChild._lifecycleState == _ElementLifecycle.active);
-
-      return newChild;
+      try {
+        if (inactiveChild != null) {
+          assert(inactiveChild._parent == null);
+          inactiveChild._activateWithParent(this, newSlot);
+          final Element? updatedChild = updateChild(inactiveChild, newWidget, newSlot);
+          assert(inactiveChild == updatedChild);
+          return updatedChild!;
+        } else {
+          newChild.mount(this, newSlot);
+          assert(newChild._lifecycleState == _ElementLifecycle.active);
+          return newChild;
+        }
+      } catch (_) {
+        // Attempt to do some clean-up if activation or mount fails
+        // to leave tree in a reasonable state.
+        _deactivateFailedChildSilently(newChild);
+        rethrow;
+      }
     } finally {
       if (isTimelineTracked) {
         FlutterTimeline.finishSync();
@@ -4583,6 +4629,7 @@ abstract class Element extends DiagnosticableTree implements BuildContext {
   /// parent proactively calls the old parent's [deactivateChild], first using
   /// [forgetChild] to cause the old parent to update its child model.
   @protected
+  @mustCallSuper
   void deactivateChild(Element child) {
     assert(child._parent == this);
     child._parent = null;
@@ -4598,6 +4645,38 @@ abstract class Element extends DiagnosticableTree implements BuildContext {
     }());
   }
 
+  void _deactivateFailedChildSilently(Element child) {
+    try {
+      child._parent = null;
+      child.detachRenderObject();
+      _deactivateFailedSubtreeRecursively(child);
+    } catch (_) {
+      // Do not rethrow:
+      // The subtree has already thrown a different error and the framework is
+      // cleaning up on a best-effort basis.
+    }
+  }
+
+  // This method calls _ensureDeactivated for the subtree rooted at `element`,
+  // supressing all exceptions thrown.
+  //
+  // This method will attempt to keep doing treewalk even one of the nodes
+  // failed to deactivate.
+  //
+  // The subtree has already thrown a different error and the framework is
+  // cleaning up on a best-effort basis.
+  static void _deactivateFailedSubtreeRecursively(Element element) {
+    try {
+      element.deactivate();
+    } catch (_) {
+      element._ensureDeactivated();
+    }
+    element._lifecycleState = _ElementLifecycle.failed;
+    try {
+      element.visitChildren(_deactivateFailedSubtreeRecursively);
+    } catch (_) {}
+  }
+
   // The children that have been forgotten by forgetChild. This will be used in
   // [update] to remove the global key reservations of forgotten children.
   //
@@ -4672,6 +4751,7 @@ abstract class Element extends DiagnosticableTree implements BuildContext {
   /// Implementations of this method should start with a call to the inherited
   /// method, as in `super.activate()`.
   @mustCallSuper
+  @visibleForOverriding
   void activate() {
     assert(_lifecycleState == _ElementLifecycle.inactive);
     assert(owner != null);
@@ -4701,18 +4781,34 @@ abstract class Element extends DiagnosticableTree implements BuildContext {
   /// animation frame, if the element has not be reactivated, the framework will
   /// unmount the element.
   ///
-  /// This is (indirectly) called by [deactivateChild].
+  /// In case of an uncaught exception when rebuild a widget subtree, the
+  /// framework also calls this method on the failing subtree to make sure the
+  /// widget tree is in a relatively consistent state. The deactivation of such
+  /// subtrees are performed only on a best-effort basis, and the errors thrown
+  /// during deactivation will not be rethrown.
+  ///
+  /// This is indirectly called by [deactivateChild].
   ///
   /// See the lifecycle documentation for [Element] for additional information.
   ///
   /// Implementations of this method should end with a call to the inherited
   /// method, as in `super.deactivate()`.
   @mustCallSuper
+  @visibleForOverriding
   void deactivate() {
     assert(_lifecycleState == _ElementLifecycle.active);
     assert(_widget != null); // Use the private property to avoid a CastError during hot reload.
-    if (_dependencies?.isNotEmpty ?? false) {
-      for (final InheritedElement dependency in _dependencies!) {
+    _ensureDeactivated();
+  }
+
+  /// Removes dependencies and sets the lifecycle state of this [Element] to
+  /// inactive.
+  ///
+  /// This method is immediately called after [Element.deactivate], even if that
+  /// call throws an exception.
+  void _ensureDeactivated() {
+    if (_dependencies case final Set<InheritedElement> dependencies? when dependencies.isNotEmpty) {
+      for (final InheritedElement dependency in dependencies) {
         dependency.removeDependent(this);
       }
       // For expediency, we don't actually clear the list here, even though it's
@@ -4977,8 +5073,7 @@ abstract class Element extends DiagnosticableTree implements BuildContext {
 
   @override
   InheritedWidget dependOnInheritedElement(InheritedElement ancestor, {Object? aspect}) {
-    _dependencies ??= HashSet<InheritedElement>();
-    _dependencies!.add(ancestor);
+    (_dependencies ??= HashSet<InheritedElement>()).add(ancestor);
     ancestor.updateDependencies(this, aspect);
     return ancestor.widget as InheritedWidget;
   }
@@ -5714,7 +5809,7 @@ abstract class ComponentElement extends Element {
   @override
   @pragma('vm:notify-debugger-on-exception')
   void performRebuild() {
-    Widget? built;
+    Widget built;
     try {
       assert(() {
         _debugDoingBuild = true;
@@ -5757,6 +5852,10 @@ abstract class ComponentElement extends Element {
           ],
         ),
       );
+      // _Make sure the old child subtree are deactivated and disposed.
+      try {
+        _child?.deactivate();
+      } catch (_) {}
       _child = updateChild(null, built, slot);
     }
   }
@@ -6167,10 +6266,7 @@ class InheritedElement extends ProxyElement {
 
   @override
   void debugDeactivated() {
-    assert(() {
-      assert(_dependents.isEmpty);
-      return true;
-    }());
+    assert(_dependents.isEmpty);
     super.debugDeactivated();
   }
 
@@ -6748,6 +6844,7 @@ abstract class RenderObjectElement extends Element {
   }
 
   @override
+  @visibleForOverriding
   void deactivate() {
     super.deactivate();
     assert(
@@ -6758,6 +6855,7 @@ abstract class RenderObjectElement extends Element {
   }
 
   @override
+  @visibleForOverriding
   void unmount() {
     assert(
       !renderObject.debugDisposed!,

packages/flutter/test/cupertino/tab_test.dart

@@ -95,10 +95,6 @@ void main() {
 
       expect(tester.takeException(), isFlutterError);
       expect(unknownForRouteCalled, '/');
-
-      // Work-around for https://github.com/flutter/flutter/issues/65655.
-      await tester.pumpWidget(Container());
-      expect(tester.takeException(), isAssertionError);
     },
   );
 

packages/flutter/test/material/app_test.dart

@@ -347,10 +347,6 @@ void main() {
       );
       expect(tester.takeException(), isFlutterError);
       expect(log, <String>['onGenerateRoute /', 'onUnknownRoute /']);
-
-      // Work-around for https://github.com/flutter/flutter/issues/65655.
-      await tester.pumpWidget(Container());
-      expect(tester.takeException(), isAssertionError);
     },
   );
 

packages/flutter/test/widgets/framework_test.dart

@@ -2075,6 +2075,71 @@ The findRenderObject() method was called for the following element:
       ),
     );
   });
+
+  testWidgets(
+    'widget is not active if throw in deactivated',
+    experimentalLeakTesting: LeakTesting.settings
+        .withIgnoredAll(), // leaking by design because of exception
+    (WidgetTester tester) async {
+      final FlutterExceptionHandler? onError = FlutterError.onError;
+      FlutterError.onError = (_) {};
+      const Widget child = Placeholder();
+      await tester.pumpWidget(
+        StatefulWidgetSpy(onDeactivate: (_) => throw StateError('kaboom'), child: child),
+      );
+      final Element element = tester.element(find.byWidget(child));
+      assert(element.debugIsActive);
+
+      await tester.pumpWidget(const SizedBox());
+      FlutterError.onError = onError;
+      expect(element.debugIsActive, false);
+      expect(element.debugIsDefunct, false);
+    },
+  );
+
+  testWidgets(
+    'widget is not active if throw in activated',
+    experimentalLeakTesting: LeakTesting.settings
+        .withIgnoredAll(), // leaking by design because of exception
+    (WidgetTester tester) async {
+      final FlutterExceptionHandler? onError = FlutterError.onError;
+      FlutterError.onError = (_) {};
+      const Widget child = Placeholder();
+      final Widget widget = StatefulWidgetSpy(
+        key: GlobalKey(),
+        onActivate: (_) => throw StateError('kaboom'),
+        child: child,
+      );
+      await tester.pumpWidget(widget);
+      final Element element = tester.element(find.byWidget(child));
+
+      await tester.pumpWidget(MetaData(child: widget));
+      FlutterError.onError = onError;
+      expect(element.debugIsActive, false);
+      expect(element.debugIsDefunct, false);
+    },
+  );
+
+  testWidgets(
+    'widget is unmounted if throw in dispose',
+    experimentalLeakTesting: LeakTesting.settings
+        .withIgnoredAll(), // leaking by design because of exception
+    (WidgetTester tester) async {
+      final FlutterExceptionHandler? onError = FlutterError.onError;
+      FlutterError.onError = (_) {};
+      const Widget child = Placeholder();
+      final Widget widget = StatefulWidgetSpy(
+        onDispose: (_) => throw StateError('kaboom'),
+        child: child,
+      );
+      await tester.pumpWidget(widget);
+      final Element element = tester.element(find.byWidget(child));
+      await tester.pumpWidget(child);
+
+      FlutterError.onError = onError;
+      expect(element.debugIsDefunct, true);
+    },
+  );
 }
 
 class _TestInheritedElement extends InheritedElement {
@@ -2323,6 +2388,7 @@ class StatefulWidgetSpy extends StatefulWidget {
     this.onDeactivate,
     this.onActivate,
     this.onDidUpdateWidget,
+    this.child = const SizedBox(),
   });
 
   final void Function(BuildContext)? onBuild;
@@ -2332,6 +2398,7 @@ class StatefulWidgetSpy extends StatefulWidget {
   final void Function(BuildContext)? onDeactivate;
   final void Function(BuildContext)? onActivate;
   final void Function(BuildContext)? onDidUpdateWidget;
+  final Widget child;
 
   @override
   State<StatefulWidgetSpy> createState() => _StatefulWidgetSpyState();
@@ -2377,7 +2444,7 @@ class _StatefulWidgetSpyState extends State<StatefulWidgetSpy> {
   @override
   Widget build(BuildContext context) {
     widget.onBuild?.call(context);
-    return Container();
+    return widget.child;
   }
 }
 

packages/flutter/test/widgets/memory_allocations_test.dart

@@ -80,6 +80,7 @@ class _TestElement extends RenderObjectElement with RootElementMixin {
     final FocusManager newFocusManager = FocusManager();
     assignOwner(BuildOwner(focusManager: newFocusManager));
     mount(null, null);
+    // ignore: invalid_use_of_visible_for_overriding_member
     deactivate();
   }
 
@@ -145,6 +146,7 @@ Future<_EventStats> _activateFlutterObjectsAndReturnCountOfEvents() async {
   element.makeInactive();
   result.creations +=
       4; // 1 for the new BuildOwner, 1 for the new FocusManager, 1 for the new FocusScopeNode, 1 for the new _HighlightModeManager
+  // ignore: invalid_use_of_visible_for_overriding_member
   element.unmount();
   result.disposals += 2; // 1 for the old BuildOwner, 1 for the element
   renderObject.dispose();

packages/flutter/test/widgets/navigator_restoration_test.dart

@@ -2,7 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-import 'package:flutter/foundation.dart';
 import 'package:flutter/material.dart';
 import 'package:flutter_test/flutter_test.dart';
 import 'package:leak_tracker_flutter_testing/leak_tracker_flutter_testing.dart';
@@ -1095,18 +1094,6 @@ void main() {
         (exception as AssertionError).message,
         contains('All routes returned by onGenerateInitialRoutes are not restorable.'),
       );
-
-      // The previous assert leaves the widget tree in a broken state, so the
-      // following code catches any remaining exceptions from attempting to build
-      // new widget tree.
-      final FlutterExceptionHandler? oldHandler = FlutterError.onError;
-      dynamic remainingException;
-      FlutterError.onError = (FlutterErrorDetails details) {
-        remainingException ??= details.exception;
-      };
-      await tester.pumpWidget(Container(key: UniqueKey()));
-      FlutterError.onError = oldHandler;
-      expect(remainingException, isAssertionError);
     },
   );
 }

packages/flutter/test/widgets/tree_shape_test.dart

@@ -94,29 +94,32 @@ void main() {
     },
   );
 
-  testWidgets('A View can not be moved via GlobalKey to be a child of a RenderObject', (
-    WidgetTester tester,
-  ) async {
-    final Widget globalKeyedView = View(
-      key: GlobalKey(),
-      view: FakeView(tester.view),
-      child: const ColoredBox(color: Colors.red),
-    );
+  testWidgets(
+    'A View can not be moved via GlobalKey to be a child of a RenderObject',
+    experimentalLeakTesting: LeakTesting.settings
+        .withIgnoredAll(), // leaking by design because of exception
+    (WidgetTester tester) async {
+      final Widget globalKeyedView = View(
+        key: GlobalKey(),
+        view: FakeView(tester.view),
+        child: const ColoredBox(color: Colors.red),
+      );
 
-    await tester.pumpWidget(wrapWithView: false, globalKeyedView);
-    expect(tester.takeException(), isNull);
+      await tester.pumpWidget(wrapWithView: false, globalKeyedView);
+      expect(tester.takeException(), isNull);
 
-    await tester.pumpWidget(wrapWithView: false, View(view: tester.view, child: globalKeyedView));
+      await tester.pumpWidget(wrapWithView: false, View(view: tester.view, child: globalKeyedView));
 
-    expect(
-      tester.takeException(),
-      isFlutterError.having(
-        (FlutterError error) => error.message,
-        'message',
-        contains('cannot maintain an independent render tree at its current location.'),
-      ),
-    );
-  });
+      expect(
+        tester.takeException(),
+        isFlutterError.having(
+          (FlutterError error) => error.message,
+          'message',
+          contains('cannot maintain an independent render tree at its current location.'),
+        ),
+      );
+    },
+  );
 
   testWidgets('The view property of a ViewAnchor cannot be a render object widget', (
     WidgetTester tester,

packages/flutter/test_release/widgets/memory_allocations_test.dart

@@ -60,6 +60,7 @@ class _TestElement extends RenderTreeRootElement with RootElementMixin {
   void makeInactive() {
     assignOwner(BuildOwner(focusManager: FocusManager()));
     mount(null, null);
+    // ignore: invalid_use_of_visible_for_overriding_member
     deactivate();
   }
 
@@ -95,6 +96,7 @@ class _MyStatefulWidgetState extends State<_MyStatefulWidget> {
 Future<void> _activateFlutterObjects(WidgetTester tester) async {
   final _TestElement element = _TestElement();
   element.makeInactive();
+  // ignore: invalid_use_of_visible_for_overriding_member
   element.unmount();
 
   // Create and dispose State: