Fix TextSelectionOverlay crash when layout is degenerate#188672
Fix TextSelectionOverlay crash when layout is degenerate#188672auto-submit[bot] merged 15 commits into
Conversation
navaronbracke
left a comment
There was a problem hiding this comment.
Aware that this is still a draft, but just some minor nits on the tests
ed0bfce to
4faf47c
Compare
There was a problem hiding this comment.
Code Review
This pull request prevents crashes during text selection handle dragging when the layout is degenerate (e.g., when coordinates are non-finite or the preferred line height is zero or less) by returning null from _getHandleDy and skipping the drag update. It also adds a regression test for this scenario. The review feedback suggests explicitly checking if preferredLineHeight is finite to handle potential NaN values and correcting the issue number referenced in the test comments.
|
Thank you for the review @navaronbracke! Should be ready for another one. |
| final List<TextBox> boxes = _getOrCreateLayoutTemplate().getBoxesForRange( | ||
| 0, | ||
| 1, | ||
| boxHeightStyle: ui.BoxHeightStyle.strut, |
There was a problem hiding this comment.
dot shorthand isn't enabled in the framework yet?
| 1, | ||
| boxHeightStyle: ui.BoxHeightStyle.strut, | ||
| ); | ||
| if (boxes.isEmpty) { |
There was a problem hiding this comment.
IIRC this should never be empty for the template unless the font doesn't have a glyph for U+0020 which is highly unlikely.
There was a problem hiding this comment.
The test, dragging selection handle does not crash when layout is degenerate (preferredLineHeight == 0), is able to force this scenario. It crashes without the change in text_painter.dart.
There was a problem hiding this comment.
Ah setting the font size to 0. But I'd assume that would be very rare. Is the crash common?
There was a problem hiding this comment.
Also the retuned box list will be empty?
There was a problem hiding this comment.
I was not able to reproduce the crash on my Pixel fold, outside of the example in this PR description which uses a slider + Transform to force the crash. Seems to be a race condition that does not happen too often but has been reported internally and externally.
There was a problem hiding this comment.
I'm not sure what the expected behavior is, but does the getRectForBoxes method return a list of a single box with 0 height, or does it return an empty list?
There was a problem hiding this comment.
It returns an empty list.
There was a problem hiding this comment.
Ok I was able to repo with height set to 1 and font size to 0
| double? _getHandleDy(double dragDy, double handleDy) { | ||
| final double preferredLineHeight = renderObject.preferredLineHeight; | ||
| if (preferredLineHeight <= 0.0 || | ||
| !preferredLineHeight.isFinite || |
There was a problem hiding this comment.
Again, I think callers should be able to assume preferredLineHeight is a positive finite value.
There was a problem hiding this comment.
I'm not sure I follow, does that mean preferredLineHeight can never be 0.0 or less?
There was a problem hiding this comment.
Preferred height should be finite I think (i don't see how skparagraph would give us an infinite paragraph height, barring bugs), so probably make it an assert?
There was a problem hiding this comment.
0.0 is possible if you give it a font size of 0, but probably not infinite or negative.
| /// Returns null if the layout is degenerate (e.g. [RenderEditable.preferredLineHeight] | ||
| /// is zero or coordinates are non-finite), indicating that the drag update should | ||
| /// be skipped. | ||
| double? _getHandleDy(double dragDy, double handleDy) { |
There was a problem hiding this comment.
Why does this method have to use preferredLineHeight instead of actual text layout? Performance? The line diff calculation can go very wrong since it assumes every line has the same height and there are infinite lines (e.g., if there isn't enough lines then this can move the handle to a line that doesn't exist).
There was a problem hiding this comment.
I'm guessing it was used out of convenience here. I'm not opposed to using LineMetrics instead if that's what the alternative would be.
There was a problem hiding this comment.
Ah I was thinking about using hit testing to figure out which line it should snap to. What's the expected behavior if there isn't enough lines?
There was a problem hiding this comment.
I created an issue for this #188871 with some gemini help. It might be a little more involved than fixing the crash as it seems other behaviors are affected by our use of preferredLineHeight like handle positioning. What do you think about resolving this crash in this PR and doing another one to fix issue I just created.
| .getBoxesForRange(0, 1, boxHeightStyle: ui.BoxHeightStyle.strut) | ||
| .single; | ||
| return textBox.toRect().height; | ||
| final List<TextBox> boxes = _getOrCreateLayoutTemplate().getBoxesForRange( |
There was a problem hiding this comment.
Can you add a test for this API? Just want to make sure when the new issue you filed is fixed we can still catch it if getFullHeightForCaret somehow regresses. Also could you add a comment near the isEmpty check that says the list can be empty when font size is 0 and textStyle.height is non-zero?
a46547b to
267a2bd
Compare
267a2bd to
5115698
Compare
5115698 to
4165593
Compare
…12169) Manual roll Flutter from 91939cc4db78 to dc2a8703e12b (50 revisions) Manual roll requested by [email protected] flutter/flutter@91939cc...dc2a870 2026-07-09 [email protected] [ios,macos] Update swiftc.py flags to match swiftc (flutter/flutter#189174) 2026-07-09 [email protected] [AGP 9] Update Warn Version to AGP 9+ (flutter/flutter#189109) 2026-07-09 [email protected] Sync CHANGELOG.md from stable (flutter/flutter#189203) 2026-07-09 [email protected] [web] Roll Chrome to 145 (framework) (flutter/flutter#182861) 2026-07-09 [email protected] Roll Packages from 52d84d6 to 20928d5 (6 revisions) (flutter/flutter#189194) 2026-07-09 [email protected] [web] Avoid absolute positioning for base CanvasKit canvas (flutter/flutter#188337) 2026-07-09 [email protected] Roll Dart SDK from cdb7217e65aa to a11fb7ed40a5 (6 revisions) (flutter/flutter#189195) 2026-07-09 [email protected] Fix dereference of nullptr in the moved-to-rect signal in the Linux embedder (flutter/flutter#189152) 2026-07-09 [email protected] Fix data for design packages (flutter/flutter#189140) 2026-07-09 [email protected] Roll Skia from 7b42d1251d54 to ab3a7b98c94d (2 revisions) (flutter/flutter#189181) 2026-07-09 [email protected] Roll Skia from 05d9d214e0b7 to 7b42d1251d54 (2 revisions) (flutter/flutter#189175) 2026-07-09 [email protected] Roll Skia from 542c8bdd7f4f to 05d9d214e0b7 (4 revisions) (flutter/flutter#189169) 2026-07-09 [email protected] UberSDF rect handling for thin (line-like) rectangles (flutter/flutter#188821) 2026-07-09 [email protected] Roll Skia from dd572c07f63c to 542c8bdd7f4f (4 revisions) (flutter/flutter#189160) 2026-07-09 [email protected] [flutter_tools] Fix hot restart for WASM web builds (flutter/flutter#187898) 2026-07-08 [email protected] Split FlViewRenderer into OpenGL and software backends (flutter/flutter#188824) 2026-07-08 [email protected] Promote android_hardware_smoke_tests out of bringup in CI (flutter/flutter#189081) 2026-07-08 [email protected] Roll Skia from 8df24be66531 to dd572c07f63c (4 revisions) (flutter/flutter#189150) 2026-07-08 [email protected] Expose LinuxWindowRegistrar on _window_linux.dart in order to better support out of tree LinuxWindowingOwners (flutter/flutter#188917) 2026-07-08 [email protected] Roll pub packages (flutter/flutter#189149) 2026-07-08 [email protected] fix(ci): harden some workflows (flutter/flutter#189087) 2026-07-08 [email protected] Roll Skia from 51a62da33da0 to 8df24be66531 (1 revision) (flutter/flutter#189139) 2026-07-08 [email protected] Roll Dart SDK to Dart 3.13 beta3 (flutter/flutter#189122) 2026-07-08 [email protected] [flutter_tools] Don't crash on non-UTF-8 plugin pubspec.yaml (flutter/flutter#188976) 2026-07-08 [email protected] [flutter_tools] Watch transitive #include headers for FragmentProgram hot reload (flutter/flutter#187945) 2026-07-08 [email protected] Roll Skia from 040d9f55de00 to 51a62da33da0 (1 revision) (flutter/flutter#189135) 2026-07-08 [email protected] Roll Packages from 92525f5 to 52d84d6 (7 revisions) (flutter/flutter#189134) 2026-07-08 [email protected] [flutter_tools] Forcefully kill hung subprocesses 5 seconds after timeout (flutter/flutter#187178) 2026-07-08 [email protected] Expose the app's build name and number as compile-time constants (flutter/flutter#187935) 2026-07-08 [email protected] Roll Skia from 1ff92f879815 to 040d9f55de00 (1 revision) (flutter/flutter#189131) 2026-07-08 [email protected] Roll Skia from 6137414bef5c to 1ff92f879815 (6 revisions) (flutter/flutter#189126) 2026-07-08 [email protected] [test cross imports] More test/rendering + flutter_test/test fixes (flutter/flutter#188954) 2026-07-08 [email protected] engine: explain why each candidate build was skipped in Flutter web loader (flutter/flutter#186254) 2026-07-08 [email protected] vscode: add missing unicode.h (flutter/flutter#189102) 2026-07-08 [email protected] Roll Fuchsia Linux SDK from 7RjQJBW3m-3Jl-7jr... to QcRFUtvCw2EobfJ8s... (flutter/flutter#189104) 2026-07-08 [email protected] Roll Skia from 075fbe4778d9 to 6137414bef5c (10 revisions) (flutter/flutter#189106) 2026-07-08 [email protected] engine: warn on WASM load failure when not crossOriginIsolated (flutter/flutter#186252) 2026-07-08 [email protected] Roll Dart SDK from c9bccc09e733 to db2155f56bf3 (2 revisions) (flutter/flutter#189105) 2026-07-08 [email protected] [flutter_tools] Prevent interactive device selection in machine mode (flutter/flutter#188267) 2026-07-08 [email protected] [flutter_tools] Fix wireless ADB device discovery when serial contains spaces (flutter/flutter#187943) 2026-07-07 [email protected] [web] Fix grouped autofill on iOS Chrome (flutter/flutter#187459) 2026-07-07 [email protected] Fix TextSelectionOverlay crash when layout is degenerate (flutter/flutter#188672) 2026-07-07 [email protected] [flutter_tools] Provision Android NDK in the main Gradle invocation (flutter/flutter#186337) 2026-07-07 [email protected] Android_hardware_smoke_test: Migrate to AGP 9 (flutter/flutter#189082) ...
Fixes #187644
Foldable Selection Handle Drag Crash Reproductions
The example below reproduce the
Unsupported operation: Infinity or NaN toIntcrash during selection handle drags under transient degenerate layouts (like folding transitions).Example: Multi-Touch Simulator (For any phone, foldable or non-foldable)
This example uses a Slider to manually scale the height of the
TextFielddown to1e-310mid-drag.Code (
main.dart)Steps to Reproduce:
scaleYto1e-310).Pre-launch Checklist
///).