From b52d659406104a53ecbcdd64d41ec89971a9e967 Mon Sep 17 00:00:00 2001 From: Hans Muller Date: Mon, 17 Apr 2017 15:44:41 -0700 Subject: [PATCH] Disallow unsafe ancestor methods (#9425) --- .../flutter/lib/src/widgets/framework.dart | 56 +++++++- .../widgets/dispose_ancestor_lookup_test.dart | 122 ++++++++++++++++++ 2 files changed, 176 insertions(+), 2 deletions(-) create mode 100644 packages/flutter/test/widgets/dispose_ancestor_lookup_test.dart diff --git a/packages/flutter/lib/src/widgets/framework.dart b/packages/flutter/lib/src/widgets/framework.dart index e56cb97b8d..d0702d9acc 100644 --- a/packages/flutter/lib/src/widgets/framework.dart +++ b/packages/flutter/lib/src/widgets/framework.dart @@ -1585,6 +1585,11 @@ abstract class BuildContext { /// (directly or indirectly) from build methods, layout and paint callbacks, or /// from [State.didChangeDependencies]. /// + /// This method should not be called from [State.deactivate] or [State.dispose] + /// because the element tree is no longer stable at that time. To refer to + /// an ancestor from one of those methods, save a reference to the ancestor + /// in [State.didChangeDependencies]. + /// /// It is also possible to call this from interaction event handlers (e.g. /// gesture callbacks) or timers, to obtain a value once, if that value is not /// going to be cached and reused later. @@ -1609,6 +1614,11 @@ abstract class BuildContext { /// widgets to obtain their corresponding [InheritedElement] object so that they /// can call [InheritedElement.dispatchDidChangeDependencies] to actually /// notify the widgets that _did_ register such a relationship. + /// + /// This method should not be called from [State.deactivate] or [State.dispose] + /// because the element tree is no longer stable at that time. To refer to + /// an ancestor from one of those methods, save a reference to the ancestor + /// by calling [inheritFromWidgetOfExactType] in [State.didChangeDependencies]. InheritedElement ancestorInheritedElementForWidgetOfExactType(Type targetType); /// Returns the nearest ancestor widget of the given type, which must be the @@ -1623,6 +1633,11 @@ abstract class BuildContext { /// Calling this method is relatively expensive (O(N) in the depth of the /// tree). Only call this method if the distance from this widget to the /// desired ancestor is known to be small and bounded. + /// + /// This method should not be called from [State.deactivate] or [State.dispose] + /// because the widget tree is no longer stable at that time. To refer to + /// an ancestor from one of those methods, save a reference to the ancestor + /// by calling [inheritFromWidgetOfExactType] in [State.didChangeDependencies]. Widget ancestorWidgetOfExactType(Type targetType); /// Returns the [State] object of the nearest ancestor [StatefulWidget] widget @@ -1631,7 +1646,7 @@ abstract class BuildContext { /// This should not be used from build methods, because the build context will /// not be rebuilt if the value that would be returned by this method changes. /// In general, [inheritFromWidgetOfExactType] is more appropriate for such - /// cases. This method is useful to change the state of an ancestor widget in + /// cases. This method is useful for changing the state of an ancestor widget in /// a one-off manner, for example, to cause an ancestor scrolling list to /// scroll this build context's widget into view, or to move the focus in /// response to user interaction. @@ -1645,6 +1660,11 @@ abstract class BuildContext { /// tree). Only call this method if the distance from this widget to the /// desired ancestor is known to be small and bounded. /// + /// This method should not be called from [State.deactivate] or [State.dispose] + /// because the widget tree is no longer stable at that time. To refer to + /// an ancestor from one of those methods, save a reference to the ancestor + /// by calling [inheritFromWidgetOfExactType] in [State.didChangeDependencies]. + /// /// Example: /// /// ```dart @@ -1663,6 +1683,11 @@ abstract class BuildContext { /// it is used by [Material] so that [InkWell] widgets can trigger the ink /// splash on the [Material]'s actual render object. /// + /// This method should not be called from [State.deactivate] or [State.dispose] + /// because the widget tree is no longer stable at that time. To refer to + /// an ancestor from one of those methods, save a reference to the ancestor + /// by calling [inheritFromWidgetOfExactType] in [State.didChangeDependencies]. + /// /// Calling this method is relatively expensive (O(N) in the depth of the /// tree). Only call this method if the distance from this widget to the /// desired ancestor is known to be small and bounded. @@ -1677,6 +1702,11 @@ abstract class BuildContext { /// This is useful for inspecting the widget tree. /// /// Calling this method is relatively expensive (O(N) in the depth of the tree). + /// + /// This method should not be called from [State.deactivate] or [State.dispose] + /// because the element tree is no longer stable at that time. To refer to + /// an ancestor from one of those methods, save a reference to the ancestor + /// by calling [inheritFromWidgetOfExactType] in [State.didChangeDependencies]. void visitAncestorElements(bool visitor(Element element)); /// Walks the children of this widget. @@ -2739,7 +2769,7 @@ abstract class Element implements BuildContext { 'The size of this render object has not yet been determined because ' 'this render object has not yet been through layout, which typically ' 'means that the size getter was called too early in the pipeline ' - '(e.g., during the build pahse) before the framework has determined ' + '(e.g., during the build phase) before the framework has determined ' 'the size and position of the render objects during layout.\n' 'The size getter was called for the following element:\n' ' $this\n' @@ -2756,8 +2786,25 @@ abstract class Element implements BuildContext { Set _dependencies; bool _hadUnsatisfiedDependencies = false; + bool _debugCheckStateIsActiveForAncestorLoopkup() { + assert(() { + if (_debugLifecycleState != _ElementLifecycle.active) { + throw new FlutterError( + 'Looking up a deactivated widget\'s ancestor is unsafe.\n' + 'At this point the state of the widget\'s element tree is no longer ' + 'stable. To safely refer to a widget\'s ancestor in its dispose() method, ' + 'save a reference to the ancestor by calling inheritFromWidgetOfExactType() ' + 'in the widget\'s didChangeDependencies() method.\n' + ); + } + return true; + }); + return true; + } + @override InheritedWidget inheritFromWidgetOfExactType(Type targetType) { + assert(_debugCheckStateIsActiveForAncestorLoopkup); final InheritedElement ancestor = _inheritedWidgets == null ? null : _inheritedWidgets[targetType]; if (ancestor != null) { assert(ancestor is InheritedElement); @@ -2772,6 +2819,7 @@ abstract class Element implements BuildContext { @override InheritedElement ancestorInheritedElementForWidgetOfExactType(Type targetType) { + assert(_debugCheckStateIsActiveForAncestorLoopkup); final InheritedElement ancestor = _inheritedWidgets == null ? null : _inheritedWidgets[targetType]; return ancestor; } @@ -2783,6 +2831,7 @@ abstract class Element implements BuildContext { @override Widget ancestorWidgetOfExactType(Type targetType) { + assert(_debugCheckStateIsActiveForAncestorLoopkup); Element ancestor = _parent; while (ancestor != null && ancestor.widget.runtimeType != targetType) ancestor = ancestor._parent; @@ -2791,6 +2840,7 @@ abstract class Element implements BuildContext { @override State ancestorStateOfType(TypeMatcher matcher) { + assert(_debugCheckStateIsActiveForAncestorLoopkup); Element ancestor = _parent; while (ancestor != null) { if (ancestor is StatefulElement && matcher.check(ancestor.state)) @@ -2803,6 +2853,7 @@ abstract class Element implements BuildContext { @override RenderObject ancestorRenderObjectOfType(TypeMatcher matcher) { + assert(_debugCheckStateIsActiveForAncestorLoopkup); Element ancestor = _parent; while (ancestor != null) { if (ancestor is RenderObjectElement && matcher.check(ancestor.renderObject)) @@ -2815,6 +2866,7 @@ abstract class Element implements BuildContext { @override void visitAncestorElements(bool visitor(Element element)) { + assert(_debugCheckStateIsActiveForAncestorLoopkup); Element ancestor = _parent; while (ancestor != null && visitor(ancestor)) ancestor = ancestor._parent; diff --git a/packages/flutter/test/widgets/dispose_ancestor_lookup_test.dart b/packages/flutter/test/widgets/dispose_ancestor_lookup_test.dart new file mode 100644 index 0000000000..f0ece4bff6 --- /dev/null +++ b/packages/flutter/test/widgets/dispose_ancestor_lookup_test.dart @@ -0,0 +1,122 @@ +// Copyright 2017 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:flutter_test/flutter_test.dart' hide TypeMatcher; +import 'package:flutter/widgets.dart'; + +typedef void TestCallback(BuildContext context); + +class TestWidget extends StatefulWidget { + TestWidget(this.callback); + + final TestCallback callback; + + @override + TestWidgetState createState() => new TestWidgetState(); +} + +class TestWidgetState extends State { + @override + void dispose() { + widget.callback(context); + super.dispose(); + } + + @override + Widget build(BuildContext context) => const Text('test'); +} + +void main() { + testWidgets('inheritFromWidgetOfExactType() called from dispose() throws error', (WidgetTester tester) async { + bool disposeCalled = false; + await tester.pumpWidget( + new TestWidget((BuildContext context) { + disposeCalled = true; + context.inheritFromWidgetOfExactType(Container); + }), + ); + await tester.pumpWidget(new Container()); + expect(disposeCalled, isTrue); + expect(tester.takeException(), isFlutterError); + }); + + testWidgets('ancestorInheritedElementForWidgetOfExactType() called from dispose() throws error', (WidgetTester tester) async { + bool disposeCalled = false; + await tester.pumpWidget( + new TestWidget((BuildContext context) { + disposeCalled = true; + context.ancestorInheritedElementForWidgetOfExactType(Container); + }), + ); + await tester.pumpWidget(new Container()); + expect(disposeCalled, isTrue); + expect(tester.takeException(), isFlutterError); + }); + + testWidgets('ancestorWidgetOfExactType() called from dispose() throws error', (WidgetTester tester) async { + bool disposeCalled = false; + await tester.pumpWidget( + new TestWidget((BuildContext context) { + disposeCalled = true; + context.ancestorWidgetOfExactType(Container); + }), + ); + await tester.pumpWidget(new Container()); + expect(disposeCalled, isTrue); + expect(tester.takeException(), isFlutterError); + }); + + testWidgets('ancestorStateOfType() called from dispose() throws error', (WidgetTester tester) async { + bool disposeCalled = false; + await tester.pumpWidget( + new TestWidget((BuildContext context) { + disposeCalled = true; + context.ancestorStateOfType(const TypeMatcher()); + }), + ); + await tester.pumpWidget(new Container()); + expect(disposeCalled, isTrue); + expect(tester.takeException(), isFlutterError); + }); + + testWidgets('ancestorRenderObjectOfType() called from dispose() throws error', (WidgetTester tester) async { + bool disposeCalled = false; + await tester.pumpWidget( + new TestWidget((BuildContext context) { + disposeCalled = true; + context.ancestorRenderObjectOfType(const TypeMatcher()); + }), + ); + await tester.pumpWidget(new Container()); + expect(disposeCalled, isTrue); + expect(tester.takeException(), isFlutterError); + }); + + testWidgets('visitAncestorElements() called from dispose() throws error', (WidgetTester tester) async { + bool disposeCalled = false; + await tester.pumpWidget( + new TestWidget((BuildContext context) { + disposeCalled = true; + context.visitAncestorElements((Element element){ }); + }), + ); + await tester.pumpWidget(new Container()); + expect(disposeCalled, isTrue); + expect(tester.takeException(), isFlutterError); + }); + + testWidgets('dispose() method does not unconditionally throw an error', (WidgetTester tester) async { + bool disposeCalled = false; + await tester.pumpWidget( + new TestWidget((BuildContext context) { + disposeCalled = true; + }), + ); + await tester.pumpWidget(new Container()); + expect(disposeCalled, isTrue); + }); + + + +}