Show error message in release mode when box is not laid out without losing performance #126302

fzyzcjy · 2023-05-09T00:01:26Z

When I encounter bugs in production environment saying null pointer error on return _size!, I wish I could know more information! However, currently the information is revealed by assert(hasSize, 'RenderBox was not laid out: $this');, thus only in debug mode can I know what is this (and/or more information that can be added), while in release mode I can only see a stack trace saying it is RenderBox.size that throws - nothing more :/

Therefore, it is intuitive to add this extra information in release mode. However, will it affect performance? We all know this is in critical path and should be quite careful. Thus I did some experiments: https://godbolt.org/z/zPPPf5969 From my naive understanding of assembly, the two versions of code (size! vs size ?? throw) has the same assembly, except that they throw different types of errors. In other words, when there is no error, both code should be equivalent; when there is an error, surely the new code will be slower since it calls this.toString(), but the error handling process is rare and already heavy, so this is not a problem.

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.
All existing and new tests are passing.

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

Hixie · 2023-05-09T00:17:20Z

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

@@ -1985,7 +1985,7 @@ abstract class RenderBox extends RenderObject {
      }
      return true;
    }());
-    return _size!;
+    return _size ?? (throw AssertionError('RenderBox was not laid out: $this'));


we should use StateError in release builds, not AssertionError, and we should definitely not call RenderBox.toString, because merely referencing that has significant performance implications.

StateError fixed

we should definitely not call RenderBox.toString: May I know a bit more details - do you mean code size or runtime overhead? For code size, I guess toString is not tree-stripped since there can be dynamic.toString but I will make an experiment; for runtime overhead, IMHO the whole error processing logic is quite complex and slow, thus no need to worry

Hmm I realize in release mode, toString() calls toStringShort and then it outputs almost nothing (just describeIdentity)... Then let me just change to that, thus also avoiding toString

Hixie · 2023-05-09T00:17:46Z

test-exempt: changes the type of a release mode exception

fzyzcjy · 2023-05-09T00:42:13Z

(Possibly related: dart-lang/sdk#52317)

fzyzcjy · 2023-05-09T10:04:06Z

(GitHub says there is update 1 hour ago but I cannot see anything new... GitHub looks a bit weird today... So if I am missing something just tell me!)

flutter-dashboard · 2023-05-09T10:16:29Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

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.

goderbauer · 2023-05-30T22:50:50Z

@fzyzcjy I seem to remember that this was opened in the context of Crashlytics garbling up some stack traces. Without that issue, is this PR still relevant and useful?

goderbauer · 2023-05-30T22:53:00Z

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

@@ -1985,7 +1985,7 @@ abstract class RenderBox extends RenderObject {
      }
      return true;
    }());
-    return _size!;
+    return _size ?? (throw StateError('RenderBox was not laid out: $runtimeType#${shortHash(this)}'));


We avoid calling toString on runtimeType (as is done here implicitly) in release mode, see objectRuntimeType

I did it deliberately ;) This is because objectRuntimeType will yield <optimized out> at runtime, while the main idea of the PR is to know which exact class causes the bug.

Given the godbolt decompile results above, it seems that, as long as we do not touch this branch (i.e. throw error), we have zero overhead. When we throw error, we already have a ton of overhead and trouble so this runtimeType.toString does not seem to be a big problem.

fzyzcjy · 2023-05-30T23:12:23Z

@fzyzcjy I seem to remember that this was opened in the context of Crashlytics garbling up some stack traces. Without that issue, is this PR still relevant and useful?

Yes and no. Yes I opened a series of issues because of Sentry yields wrong stacks. However, for this single one, I guess this can still be useful even not under that scenario, because the RenderBox.size may be accessed by functions in class RenderBox, thus it is hard to debug if we just see a stack trace full of RenderBox.layout and RenderBox.size - we do not know which RenderBox causes the problem (say, RenderStack? RenderColoredBox? etc).

Since this PR does not cause performance problems (see Godbolt above), I guess this will help devs.

goderbauer

LGTM

Piinks

LGTM

…ithout losing performance (flutter/flutter#126302)

Roll Flutter from 0b74153 to 8a5c22e (46 revisions) flutter/flutter@0b74153...8a5c22e 2023-06-07 5236035+fzyzcjy@users.noreply.github.com Super tiny MediaQuery doc update (flutter/flutter#127904) 2023-06-07 jacksongardner@google.com Revert "Make inspector weakly referencing the inspected objects." (flutter/flutter#128436) 2023-06-07 leigha.jarett@gmail.com Update menu API docs to help developers migrate to m3 (flutter/flutter#128351) 2023-06-07 andrewrkolos@gmail.com [tools] allow explicitly specifying the JDK to use via a new config setting (flutter/flutter#128264) 2023-06-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6f9df0f988c1 to 59d5444cf06c (3 revisions) (flutter/flutter#128376) 2023-06-07 andrewrkolos@gmail.com Do not try to load main/default asset image if only higher-res variants exist (flutter/flutter#128143) 2023-06-07 92602467+99spark@users.noreply.github.com Addressed Ambiguity in transform.scale constructor docs (flutter/flutter#128182) 2023-06-07 5236035+fzyzcjy@users.noreply.github.com Super tiny fix of dead link (flutter/flutter#128160) 2023-06-07 goderbauer@google.com Refactor tests (flutter/flutter#128371) 2023-06-07 polinach@google.com Make inspector weakly referencing the inspected objects. (flutter/flutter#128095) 2023-06-07 goderbauer@google.com Add viewId to PointerEvents (flutter/flutter#128287) 2023-06-07 5236035+fzyzcjy@users.noreply.github.com Show error message in release mode when box is not laid out without losing performance (flutter/flutter#126302) 2023-06-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from ca499463ec2e to 6f9df0f988c1 (1 revision) (flutter/flutter#128363) 2023-06-06 khanhnwin@gmail.com Update Draggable YouTube video link (flutter/flutter#128078) 2023-06-06 31859944+LongCatIsLooong@users.noreply.github.com Remove more rounding hacks from TextPainter (flutter/flutter#127826) 2023-06-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 4571695f9e76 to ca499463ec2e (1 revision) (flutter/flutter#128356) 2023-06-06 43054281+camsim99@users.noreply.github.com [Android] Update plugin and module templates to use Flutter constant for `compileSdkVersion` (flutter/flutter#128073) 2023-06-06 rmolivares@renzo-olivares.dev handleSelectWord in MultiSelectableSelectionContainerDelegate should handle rects inside of rects (flutter/flutter#127478) 2023-06-06 christopherfujino@gmail.com [flutter_tools] never tree shake 0x20 (space) font codepoints on web (flutter/flutter#128302) 2023-06-06 31859944+LongCatIsLooong@users.noreply.github.com Remove `textScaleFactor` dependent logic from `AppBar` (flutter/flutter#128112) 2023-06-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from b6d37f8f74ad to 4571695f9e76 (6 revisions) (flutter/flutter#128350) 2023-06-06 5236035+fzyzcjy@users.noreply.github.com Fix `Null check operator used on a null value` on TextField with contextMenuBuilder (flutter/flutter#128114) 2023-06-06 engine-flutter-autoroll@skia.org Roll Packages from db4e5c2 to da72219 (10 revisions) (flutter/flutter#128348) 2023-06-06 91688203+yusuf-goog@users.noreply.github.com Updating cirrus docker image to ubuntu focal. (flutter/flutter#128291) 2023-06-06 leigha.jarett@gmail.com Adding example for migrating to navigation drawer (flutter/flutter#128295) 2023-06-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 722aad83e5fe to b6d37f8f74ad (2 revisions) (flutter/flutter#128341) 2023-06-06 goderbauer@google.com Clean-up viewId casts in flutter_test (flutter/flutter#128256) 2023-06-06 kevinjchisholm@google.com Update cherry-pick issue template to more uniform labels. (flutter/flutter#128333) 2023-06-06 hans.muller@gmail.com Use Material3 in the 2D viewport tests (flutter/flutter#128155) 2023-06-06 nbosch@google.com Use a `show` over a `hide` for `test_api` exports (flutter/flutter#128298) 2023-06-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from aaa7574375a6 to 722aad83e5fe (1 revision) (flutter/flutter#128307) 2023-06-06 leigha.jarett@gmail.com Migration guide for moving from BottomNavigationBar to NavigationBar (flutter/flutter#128263) 2023-06-06 mdebbar@google.com [web] Use 'Uri' instead of 'dart:html' to extract pathname (flutter/flutter#127983) 2023-06-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2b353ae90731 to aaa7574375a6 (4 revisions) (flutter/flutter#128301) 2023-06-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 220ece4d9faa to 2b353ae90731 (1 revision) (flutter/flutter#128293) 2023-06-05 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 4.0.4 to 4.1.0 (flutter/flutter#128290) 2023-06-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7f12e3497428 to 220ece4d9faa (6 revisions) (flutter/flutter#128282) 2023-06-05 jonahwilliams@google.com [framework] attempt non-key solution (flutter/flutter#128273) 2023-06-05 chillers@google.com [labeler] Fix adding labels when name is directory (flutter/flutter#128243) 2023-06-05 katelovett@google.com Remove scrollbar deprecations isAlwaysShown and hoverThickness (flutter/flutter#127351) 2023-06-05 katelovett@google.com Fix update drag error that made NestedScrollView un-scrollable (flutter/flutter#127718) 2023-06-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from f9f72388a4da to 7f12e3497428 (4 revisions) (flutter/flutter#128271) 2023-06-05 goderbauer@google.com Migrate SemanticsBinding to onSemanticsActionEvent (flutter/flutter#128254) 2023-06-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from c838a1b05924 to f9f72388a4da (19 revisions) (flutter/flutter#128252) 2023-06-05 jonahwilliams@google.com [framework] force flexible space background to rebuild. (flutter/flutter#128138) 2023-06-05 engine-flutter-autoroll@skia.org Roll Packages from 75085ed to db4e5c2 (4 revisions) (flutter/flutter#128246) ...

…ithout losing performance (flutter/flutter#126302)

Update box.dart

2242634

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label May 9, 2023

fzyzcjy mentioned this pull request May 9, 2023

Show error message in release mode when box is not laid out without losing performance #126303

Closed

fzyzcjy marked this pull request as ready for review May 9, 2023 00:07

Hixie reviewed May 9, 2023

View reviewed changes

fzyzcjy added 3 commits May 9, 2023 08:30

Update box.dart

ee582b9

Update box.dart

a58e49a

Update box.dart

f3e163e

fzyzcjy mentioned this pull request May 9, 2023

Knowing which class it is after obfuscation (willing to PR) dart-lang/sdk#52317

Closed

fzyzcjy changed the title ~~Show error message in release mode when box is not render box is not laid out without losing performance~~ Show error message in release mode when box is not laid out without losing performance May 9, 2023

goderbauer reviewed May 30, 2023

View reviewed changes

goderbauer approved these changes Jun 6, 2023

View reviewed changes

goderbauer requested a review from Piinks June 6, 2023 22:15

Piinks approved these changes Jun 6, 2023

View reviewed changes

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 6, 2023

Merge branch 'master' into patch-34

bb2aa31

auto-submit bot merged commit 31e3ae8 into flutter:master Jun 7, 2023

engine-flutter-autoroll mentioned this pull request Jun 7, 2023

Roll Flutter from 0b7415356e07 to 6848110ff3a9 (44 revisions) flutter/packages#4156

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2023

31e3ae8 Show error message in release mode when box is not laid out w…

d86a5b7

…ithout losing performance (flutter/flutter#126302)

engine-flutter-autoroll mentioned this pull request Jun 7, 2023

Roll Flutter from 0b7415356e07 to 2830f0734d6c (45 revisions) flutter/packages#4158

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2023

31e3ae8 Show error message in release mode when box is not laid out w…

e6091a6

…ithout losing performance (flutter/flutter#126302)

engine-flutter-autoroll mentioned this pull request Jun 7, 2023

Roll Flutter from 0b7415356e07 to 8a5c22e282db (46 revisions) flutter/packages#4160

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2023

31e3ae8 Show error message in release mode when box is not laid out w…

19d0206

…ithout losing performance (flutter/flutter#126302)

HansMuller mentioned this pull request Jun 9, 2023

Flutter Framework Team: Notable Commits #121415

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023

31e3ae8 Show error message in release mode when box is not laid out w…

25158ef

…ithout losing performance (flutter/flutter#126302)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

31e3ae8 Show error message in release mode when box is not laid out w…

46dd81a

…ithout losing performance (flutter/flutter#126302)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

31e3ae8 Show error message in release mode when box is not laid out w…

b4da96f

…ithout losing performance (flutter/flutter#126302)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

31e3ae8 Show error message in release mode when box is not laid out w…

6035855

…ithout losing performance (flutter/flutter#126302)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Show error message in release mode when box is not laid out without losing performance #126302

Show error message in release mode when box is not laid out without losing performance #126302

Uh oh!

fzyzcjy commented May 9, 2023 •

edited

Loading

Uh oh!

Hixie May 9, 2023

Uh oh!

fzyzcjy May 9, 2023

Uh oh!

fzyzcjy May 9, 2023 •

edited

Loading

Uh oh!

Hixie commented May 9, 2023

Uh oh!

fzyzcjy commented May 9, 2023

Uh oh!

fzyzcjy commented May 9, 2023

Uh oh!

flutter-dashboard bot commented May 9, 2023

Uh oh!

goderbauer commented May 30, 2023

Uh oh!

goderbauer May 30, 2023

Uh oh!

fzyzcjy May 30, 2023

Uh oh!

fzyzcjy commented May 30, 2023 •

edited

Loading

Uh oh!

goderbauer left a comment

Uh oh!

Piinks left a comment

Uh oh!

Uh oh!

Show error message in release mode when box is not laid out without losing performance #126302

Show error message in release mode when box is not laid out without losing performance #126302

Uh oh!

Conversation

fzyzcjy commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Hixie May 9, 2023

Choose a reason for hiding this comment

Uh oh!

fzyzcjy May 9, 2023

Choose a reason for hiding this comment

Uh oh!

fzyzcjy May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented May 9, 2023

Uh oh!

fzyzcjy commented May 9, 2023

Uh oh!

fzyzcjy commented May 9, 2023

Uh oh!

flutter-dashboard bot commented May 9, 2023

Uh oh!

goderbauer commented May 30, 2023

Uh oh!

goderbauer May 30, 2023

Choose a reason for hiding this comment

Uh oh!

fzyzcjy May 30, 2023

Choose a reason for hiding this comment

Uh oh!

fzyzcjy commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fzyzcjy commented May 9, 2023 •

edited

Loading

fzyzcjy May 9, 2023 •

edited

Loading

fzyzcjy commented May 30, 2023 •

edited

Loading