Prevent uncontrolled crash in Release mode when ParentDataWidget is used incorrectly #155091

darajava · 2024-09-12T15:06:51Z

The following (incorrect) code causes a nasty crash in release mode:

void main() =>
  runApp(
    const Flexible( // <- This is incorrect, should be child of `Row` or similar
      child:
        Text('This is only shown in debug mode!',
          key: Key('title'),
          textDirection: TextDirection.ltr,
        ),
      ),
    );

Before:

After:

I have refactored _updateParentData to make it more simple and to give a clearer and more specific error message when Flexible or Expanded is misused. In both release and debug builds we now check parentDataWidget.debugIsValidRenderObject before calling applyParentData. The problem in the previous code was that there was a check happening in an assert. Therefore, in release/profile modes applyParentData was being unconditionally called, leading to an error causing the UI to completely break and become unusable.

With this change, Flexible can be called now in any component without breaking the UI, and still gets an exception reported in debug mode.

I had initially tried to show ErrorWidget when the tree is in a broken state, but that leads to more exceptions as the original comment in the code I worked on suggested because adding more widgets to a broken tree leads to more exceptions which is hard to test and poor for the debugging experience, as is mentioned.

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.

…ectly

flutter-dashboard · 2024-09-12T15:06:54Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

navaronbracke · 2024-09-17T17:39:39Z

packages/flutter/test/widgets/framework_test.dart

+
+    final dynamic exception = tester.takeException();
+
+    expect(exception, isFlutterError);


I would prevent the use of dynamic here. (If you really want a type, use Object? instead)

Suggested change

expect(exception, isFlutterError);

expect(tester.takeException(), isFlutterError);

navaronbracke · 2024-09-17T17:40:39Z

packages/flutter/test/widgets/framework_test.dart

+
+    final dynamic exception = tester.takeException();
+
+    expect(exception, isFlutterError);


You should also check if the error message is correct, using the having() matcher.

navaronbracke · 2024-09-17T17:41:59Z

packages/flutter/test/widgets/framework_test.dart

@@ -1574,6 +1574,23 @@ void main() {
    expect(RawKeyboard.instance.keyEventHandler, same(rawKeyEventHandler));
  });

+  testWidgets('Incorrect use of ParentDataWidget shows a useful error message', (WidgetTester tester) async {
+    await tester.pumpWidget(
+      Container(


Nit: Use SizedBox instead of Container.

navaronbracke · 2024-09-17T17:48:36Z

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

          throw FlutterError.fromParts(<DiagnosticsNode>[
-            ErrorSummary('Incorrect use of ParentDataWidget.'),
+            ErrorSummary('Incorrect use of ParentDataWidget. The widget `${


Could the formatting be improved by using a triple quotes string?

I.e.

''' Incorrect use of ParentDataWidget. The widget `${f.runtimeType} must be a direct child of a `Row`, `Column`, or `Flex` widget.' '''

navaronbracke · 2024-09-17T17:55:21Z

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

          throw FlutterError.fromParts(<DiagnosticsNode>[
-            ErrorSummary('Incorrect use of ParentDataWidget.'),
+            ErrorSummary('Incorrect use of ParentDataWidget. The widget `${
+              parentDataWidget.runtimeType


Calling runtimeType.toString() in release mode is expensive. Consider using the objectRuntimeType() helper, although that one also uses an assert.

See https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/foundation/object.dart

I found a toStringShort helper function within this file which calls objectRuntimeType

- don't call expensive `.runtimeType` - improve formatting - make test more specific

darajava · 2024-09-17T18:26:50Z

@navaronbracke thanks for your review, I have resolved your comments and updated the branch.

navaronbracke · 2024-09-17T19:37:51Z

packages/flutter/test/widgets/framework_test.dart

+      SizedBox(
+        key: const Key('container'),
+        child: Flexible( // Flexible should not be used directly in a Container or other inappropriate parents
+          child: Container(),


This is still a Container. This should probably also be a SizedBox(). See https://dart.dev/tools/linter-rules/avoid_unnecessary_containers

You might also need to add const to the first SizedBox widget.

packages/flutter/test/widgets/framework_test.dart

navaronbracke · 2024-09-17T19:42:44Z

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

+        try {
+          String errorMessage = "Incorrect use of ParentDataWidget.\n";
+
+          if (parentDataWidget.toStringShort() == 'Positioned') {


Why do we special case the Positioned/Stack case here? There might be an unknown amount of possibilities that lead to the "Incorrect use of ParentDataWidget" error.

I remember when I was new to Flutter, the Incorrect use of ParentDataWidget. error was very confusing to me, so I wanted to have a more descriptive error message as well as to fix the CI tests. Also when I was looking around the tests, I couldn't find any other violations leading to this error - but that's not to say there won't be a new way of violating it in future.

I'll add specific cases only for Positioned, Flexible, and Expanded, leaving the generic error if it doesn't match.

Co-authored-by: Navaron Bracke <brackenavaron@gmail.com>

navaronbracke · 2024-09-18T06:06:08Z

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

+          String errorMessage = 'Incorrect use of ParentDataWidget.\n';
+
+          if (parentDataWidgetType == 'Positioned') {
+            errorMessage += 'The widget `Positioned` must be a descendent of a `Stack` widget.\n';


The wording in these two messages is different? One mentions descendant (there is a typo in that word, too)
and the other mentions the word child.

I'll update the spelling there. So with Positioned it can be anywhere in the Stack (i.e. it does not have to be a direct child). For a Flexible or Expanded, it needs to be a direct child, hence the differing wording.

navaronbracke · 2024-09-18T06:07:32Z

packages/flutter/test/widgets/framework_test.dart

@@ -1574,6 +1574,32 @@ void main() {
    expect(RawKeyboard.instance.keyEventHandler, same(rawKeyEventHandler));
  });

+  testWidgets('Incorrect use of ParentDataWidget shows a useful error message', (WidgetTester tester) async {


Do we need an additional test for the Stack/Positioned case?

So there are already tests for this which I've found in parent_test_data.dart, which already covers the case that I've tested in this PR. I've edited the tests in parent_test_data to work with the new error messages and I now think the test I have made is redundant, and actually would not have failed before I made the change in framework.dart, so I think I need a better test, what do you think?

I can't currently think of a way to test release-mode code though - which is what this PR addresses, so I'm going to look into this.

The original code I modified relied on code in an assert to control the flow, and hence running into nasty exceptions only in release mode. I have made a contrived example below demonstrating the issue in the original code:

num returnDivisionCalculation() { num numerator = 1; num denominator = 0; bool attemptCalculation = true; assert(() { if (denominator == 0) { attemptCalculation = false; try { throw "Specific division error"; } catch { // log the error and continue } } }()); if (attemptCalculation) { // Always true in release mode. return numerator / denominator; } return 0; }

In debug mode, the code in assert will be run, causing safe and controlled execution of the code, but in release mode an unexpected exception will be thrown.

I don't think it's possible to write a failing test that will succeed after my change and I will ask for an exemption on Discord. I have edited the tests to update the new error messages for misuse of these widgets (of which I am happy to revert, just thought I might improve them while here)

@navaronbracke do you think that there is a way to test this?

Piinks · 2024-09-26T14:49:13Z

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

+          if (parentDataWidgetType == 'Positioned') {
+            errorMessage += 'The widget `Positioned` must be a descendant of a `Stack` widget.\n';
+          } else if (parentDataWidgetType == 'Flexible' || parentDataWidgetType == 'Expanded') {
+            errorMessage += 'The widget `$parentDataWidgetType` must be a direct child of a `Row`, `Column`, or `Flex` widget.\n';
+          }
+


This checking for types is not something we should do. There are many ParentDataWidgets, this error message handling could not possibly account for all of them. It also means we are baking in knowledge of other classes here.

Sure, the idea here is to only account for 3 common ones -- it leaves the error message as-is for any other ParentDataWidget to reduce confusion. IMO "Incorrect use of ParentDataWidget" isn't a very clear error message.

Having said this I'm very happy to remove attempted improvements to DX and fix only the original issue!

The right solution is likely to expose a way for ParentDataWidgets to be able to provide their own error message. That way any of them could provide more information.

Ah I see, makes sense. I think that is beyond the scope of this fix then, would you agree? I will remove changes to the error messages in this PR tonight.

A problem I'm having with this fix is testing. At its core, it's fixing some bad code that is only run in debug mode. I left a comment here, do you know of a way to test this?

- Remove tests - Remove attempt at improving error message

darajava · 2024-09-26T15:26:23Z

@Piinks I've now simplified this PR, but it's still missing a test.

chunhtai · 2024-10-01T22:10:48Z

(triage) Hi @darajava we need a test to merge this pr. Can you add a test?

darajava · 2024-10-01T22:52:25Z

Hi, no I don’t think this is testable since the bug is due to flow control being determined from with an assert, which only is run on debug mode, hence the bug only happens on release mode. Happy to add a test if there is a way to do it!

…

On Wed, 2 Oct 2024 at 00:11, chunhtai ***@***.***> wrote: (triage) Hi @darajava <https://github.com/darajava> we need a test to merge this pr. Can you add a test? — Reply to this email directly, view it on GitHub <#155091 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABNCOBN73HEEE5YSHHOS64LZZMMX5AVCNFSM6AAAAABODPODHOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOBXGE3DINRWGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

chunhtai · 2024-10-01T23:17:47Z

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

-      try {
-        if (!parentDataWidget.debugIsValidRenderObject(renderObject)) {
-          applyParentData = false;
+    if (parentDataWidget.debugIsValidRenderObject(renderObject)) {


This should probably not named started with debug, and should probably be convert into a private static method

I agree. It affects the control flow, and should not be exclusive to assert mode. But it was written this way ever since the first day, and not only debugIsValidRenderObject, but also the method that it depends on, debugTypicalAncestorWidgetClass, are prefixed by debug, the latter being intended for overriding and is indeed overridden by many classes. We might want to ask Hixie (the first author) whether it was intentional.

Edit: Hixie has added context in #108186 weeks ago.

It would be best to rename debugTypicalAncestorWidgetClass (which assigns the type used here) as well, but it has been used by many widgets that I doubt if we can. At least we should add a comment to explain that it's now used in release mode.

We shouldn't use debugTypicalAncestorWidgetClass in release mode, because it'll force widgets to be included that might otherwise get optimised out. But I think we can still throw in release mode even if the error message isn't as clear. We have lots of precedent for that.

(debugIsValidRenderObject should be renamed though)

I've renamed debugIsValidRenderObject and made it private. I tried making it static but the generic type made some tests fail and I couldn't figure out why.

Are you saying that incorrect usage of ParentDataWidget should still throw and cause a crash to users in release mode? I'd have imagined that release mode should be as fault-tolerant as possible, and attempt to deliver a good experience even if it doesn't follow Flutter's rules. In my experience using e.g. Flexible outside a Column does not cause any visual/usability issues, only a broken tree which is flagged in the logs in debug mode.

A similar case to this that currently does exist is using a Positioned outside a Stack. There is no point in using it outside a Stack, and it is an error, but it does not currently break the release build and gives an error in the console in debug mode.

…d static

Piinks · 2024-10-15T20:30:04Z

Hi, no I don’t think this is testable since the bug is due to flow control
being determined from with an assert, which only is run on debug mode,
hence the bug only happens on release mode. Happy to add a test if there is
a way to do it!

@darajava in the test I would expect to verify the assertion is triggered.

Piinks · 2024-10-15T20:30:44Z

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

@@ -1604,7 +1604,7 @@ abstract class ParentDataWidget<T extends ParentData> extends ProxyWidget {
  ///
  /// This is called just before [applyParentData] is invoked with the same
  /// [RenderObject] provided to that method.
-  bool debugIsValidRenderObject(RenderObject renderObject) {


This is a public method, making it private is a breaking change. Do we need to rename this here?

Piinks · 2024-10-15T20:33:00Z

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

+      // If it's not a valid render object, then don't show the ErrorWidget, or
+      // throw an exception, just report the error and move on.
+      //
+      // This prevents it from crashing in release mode and still reports the


It may not prevent it from crashing, depending on what relies on that parent data being applied. For example, if a widget lays out multiple children, and relies on parent data to inform the layout, this change could skip a crash here, just to crash later when it cannot properly lay out the widgets.

darajava · 2024-10-15T20:47:32Z

I’ve put too many hours into this important but simple fix and can’t afford to put more in unfortunately. If anyone wants to continue please feel free!

Don't ignore error on debug mode when ParentDataWidget is used incorr…

48316d9

…ectly

github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Sep 12, 2024

darajava added 2 commits September 16, 2024 12:45

put back into original state to check if CI tests pass

4776c5a

Merge branch 'master' into improve-debug-release-disparity

5533dd3

darajava marked this pull request as draft September 16, 2024 10:50

darajava added 7 commits September 16, 2024 14:16

re-add fix

d45f339

fix a different way

c681c6d

Refactor _updateParentData and give clearer error.

cb1ddea

Add unit test to verify exception thrown in debug mode.

99e5b50

remove debug

fc7af1c

re-add test

f769593

Merge branch 'master' into improve-debug-release-disparity

f29d4c7

darajava marked this pull request as ready for review September 17, 2024 16:34

darajava mentioned this pull request Sep 17, 2024

Incorrect use of ParentDataWidget exception may not appear visually in debug mode. #108186

Open

navaronbracke reviewed Sep 17, 2024

View reviewed changes

darajava added 2 commits September 17, 2024 19:24

Merge branch 'master' into improve-debug-release-disparity

3107ab1

Resolve review comments

1daea0a

- don't call expensive `.runtimeType` - improve formatting - make test more specific

darajava added 2 commits September 17, 2024 21:30

Fix CI and improve formatting

c793db8

Merge branch 'master' into improve-debug-release-disparity

30e4d51

navaronbracke reviewed Sep 17, 2024

View reviewed changes

darajava and others added 6 commits September 17, 2024 22:32

Move comment to better place

f61b696

Co-authored-by: Navaron Bracke <brackenavaron@gmail.com>

don't catch generic case for error message

6dc40d9

fix CI

1fcede6

Use correct toString method

032dc45

fix wrong variable in interpolated string

627f7c7

Merge branch 'master' into improve-debug-release-disparity

6b26a70

navaronbracke reviewed Sep 18, 2024

View reviewed changes

darajava added 2 commits September 18, 2024 13:04

Fix non-preferred spelling

5c5238b

Merge branch 'master' into improve-debug-release-disparity

5e51ea7

darajava changed the title ~~Show error in debug mode when ParentDataWidget is used incorrectly~~ Prevent uncontrolled crash in Release mode when ParentDataWidget is used incorrectly Sep 18, 2024

darajava requested a review from navaronbracke September 19, 2024 17:15

Piinks requested review from Piinks and removed request for navaronbracke September 24, 2024 22:15

Piinks requested changes Sep 26, 2024

View reviewed changes

Simplify PR scope

221a3e3

- Remove tests - Remove attempt at improving error message

Merge branch 'master' into improve-debug-release-disparity

559eb21

chunhtai reviewed Oct 1, 2024

View reviewed changes

darajava added 3 commits October 2, 2024 10:21

Merge branch 'master' into improve-debug-release-disparity

dbe3339

rename method to check if render object is valid, and make private an…

688563b

…d static

undo static

85fc314

Piinks reviewed Oct 15, 2024

View reviewed changes

darajava closed this Oct 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent uncontrolled crash in Release mode when ParentDataWidget is used incorrectly #155091

Prevent uncontrolled crash in Release mode when ParentDataWidget is used incorrectly #155091

darajava commented Sep 12, 2024 •

edited

Loading

flutter-dashboard bot commented Sep 12, 2024

navaronbracke Sep 17, 2024

navaronbracke Sep 17, 2024

navaronbracke Sep 17, 2024

navaronbracke Sep 17, 2024

navaronbracke Sep 17, 2024

darajava Sep 17, 2024

darajava commented Sep 17, 2024

navaronbracke Sep 17, 2024

navaronbracke Sep 17, 2024

darajava Sep 17, 2024 •

edited

Loading

navaronbracke Sep 18, 2024

darajava Sep 18, 2024

navaronbracke Sep 18, 2024

darajava Sep 18, 2024

darajava Sep 19, 2024 •

edited

Loading

Piinks Sep 26, 2024

darajava Sep 26, 2024

Piinks Sep 26, 2024

darajava Sep 26, 2024

darajava commented Sep 26, 2024

chunhtai commented Oct 1, 2024

darajava commented Oct 1, 2024 via email

chunhtai Oct 1, 2024

dkwingsmt Oct 1, 2024 •

edited

Loading

Hixie Oct 2, 2024

Hixie Oct 2, 2024

darajava Oct 2, 2024 •

edited

Loading

Piinks commented Oct 15, 2024

Piinks Oct 15, 2024

Piinks Oct 15, 2024

darajava commented Oct 15, 2024


		final dynamic exception = tester.takeException();

		expect(exception, isFlutterError);

	expect(exception, isFlutterError);
	expect(tester.takeException(), isFlutterError);

Prevent uncontrolled crash in Release mode when ParentDataWidget is used incorrectly #155091

Prevent uncontrolled crash in Release mode when ParentDataWidget is used incorrectly #155091

Conversation

darajava commented Sep 12, 2024 • edited Loading

Pre-launch Checklist

flutter-dashboard bot commented Sep 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darajava commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darajava Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darajava Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darajava commented Sep 26, 2024

chunhtai commented Oct 1, 2024

darajava commented Oct 1, 2024 via email

Choose a reason for hiding this comment

dkwingsmt Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darajava Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Piinks commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darajava commented Oct 15, 2024

darajava commented Sep 12, 2024 •

edited

Loading

darajava Sep 17, 2024 •

edited

Loading

darajava Sep 19, 2024 •

edited

Loading

dkwingsmt Oct 1, 2024 •

edited

Loading

darajava Oct 2, 2024 •

edited

Loading