Gracefully handle negative position in getWordAtOffset #128464
Conversation
github-actions
bot
added
the
framework
justinmc
force-pushed
the
negative-position
branch
from
ec8bbc2
to
a0b2b9d
Compare
|
Looking at the Oops I read it wrong maybe it will, it also uses |
| @@ -2126,8 +2127,8 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin, | |||
| // If the platform is Android and the text is read only, try to select the | |||
| // previous word if there is one; otherwise, select the single whitespace at | |||
| // the position. | |||
| if (TextLayoutMetrics.isWhitespace(plainText.codeUnitAt(effectiveOffset)) | |||
| && effectiveOffset > 0) { | |||
| if (effectiveOffset > 0 | |||
There was a problem hiding this comment.
If there's no clear repro, could you add an assert here so we could debug what happened?
There was a problem hiding this comment.
I worry that there is a legit case where effectiveOffset can be less than 0, and the correct course of action is just to skip this if. Should I leave it with no assertion so that developers aren't confused if they trigger it in a legitimate case?
There was a problem hiding this comment.
So the theory is that _textPainter.getPositionForOffset is producing a TextPosition of (0, upstream) ? That's not supposed to be a valid output of the getPositionForOffset method I guess. If that's the case this uncovers an upstream bug so I think it's still nice to have an assert here. It won't be an issue in release builds unlike the OOB crash.
There was a problem hiding this comment.
Good point, done. Yeah that's my best guess at what's happening.
|
@Renzo-Olivares I thought I could get it by using a grapheme cluster based on that code but I still never saw |
flutter/flutter@c40baf4...042c036 2023-06-22 ahmedelsaayid@gmail.com Remove unnecessary variable `_hasPrimaryFocus` (flutter/flutter#129066) 2023-06-22 tessertaha@gmail.com Fix Material 3 Scrollable `TabBar` (flutter/flutter#125974) 2023-06-22 utisam@gmail.com Fix: Closing bottom sheet and removing FAB cause assertion failure (flutter/flutter#128566) 2023-06-22 tessertaha@gmail.com Add `InputDecorationTheme.merge` (flutter/flutter#129011) 2023-06-22 engine-flutter-autoroll@skia.org Roll Packages from 9af50d4 to 95bc1c6 (6 revisions) (flutter/flutter#129351) 2023-06-22 42216813+eliasyishak@users.noreply.github.com Prevent crashes on range errors when selecting device (flutter/flutter#129290) 2023-06-22 zanderso@users.noreply.github.com Revert "Roll Flutter Engine from 703c9a14ac7f to 8cc6d6d5efdb (1 revision)" (flutter/flutter#129353) 2023-06-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 703c9a14ac7f to 8cc6d6d5efdb (1 revision) (flutter/flutter#129339) 2023-06-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from d9530e2b87de to 703c9a14ac7f (1 revision) (flutter/flutter#129337) 2023-06-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from c6251a69a09a to d9530e2b87de (1 revision) (flutter/flutter#129334) 2023-06-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 08aaa88bf67f to c6251a69a09a (10 revisions) (flutter/flutter#129331) 2023-06-22 jason-simmons@users.noreply.github.com Manual roll of packages to 9af50d4 (flutter/flutter#129328) 2023-06-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 090fae83548a to 08aaa88bf67f (3 revisions) (flutter/flutter#129306) 2023-06-21 yjbanov@google.com [framework,web] add FlutterTimeline and semantics benchmarks that use it (flutter/flutter#128366) 2023-06-21 fluttergithubbot@gmail.com Roll pub packages (flutter/flutter#128966) 2023-06-21 6655696+guidezpl@users.noreply.github.com Remove incorrect non-nullable assumption from `ShapeDecoration.lerp` (flutter/flutter#129298) 2023-06-21 jmccandless@google.com Gracefully handle negative position in getWordAtOffset (flutter/flutter#128464) 2023-06-21 christopherfujino@gmail.com [flutter_tools] add a gradle error handler for could not open cache directory (flutter/flutter#129222) 2023-06-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from f973fb4636d3 to 090fae83548a (5 revisions) (flutter/flutter#129293) 2023-06-21 rmolivares@renzo-olivares.dev Selection area right click behavior should match native (flutter/flutter#128224) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC dit@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
I received reports of crashes due to a negative position in getWordAtOffset, and this prevents them. It looks like the existing logic was meant to handle negative positions anyway, but the terms were reversed?
I was not able to actually reproduce the crash. I can't figure out a way for
getPositionForOffsetto returnoffset: 0, affinity: upstream.In order to test this without a real repro, I made getWordAtOffset
@visibleForTesting. Is there any other way to test it?