TextInput: Verify TextRange and make method call fail loudly #104711
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
flutter-dashboard
bot
added
a: text input
dkwingsmt
force-pushed
the
verify-text-range-and-fail-loudly
branch
from
c948e2b to
a0a6629
Compare
dkwingsmt
changed the title
dkwingsmt
added
a: error message
tgucio
reviewed
May 30, 2022
dkwingsmt
force-pushed
the
verify-text-range-and-fail-loudly
branch
from
e2a0ffa to
b1c806b
Compare
Contributor
|
This pull request is not suitable for automatic merging in its current state.
|
tgucio
reviewed
Jun 9, 2022
| this.selection = const TextSelection.collapsed(offset: -1), | ||
| this.composing = TextRange.empty, | ||
| }) : assert(text != null), | ||
| // The constructor does not verified that `selection` and `composing` are |
Contributor
There was a problem hiding this comment.
Grammar nit: "does not verify".
dnfield
reviewed
Jun 14, 2022
| _channel.invokeMethod<void>( | ||
| 'TextInput.setClient', | ||
| <dynamic>[ connection._id, configuration.toJson() ], | ||
| <dynamic>[ |
Contributor
There was a problem hiding this comment.
consider <Object> (or nullable if needed).
dnfield
approved these changes
Jun 14, 2022
Contributor
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
LGTM with nit(s)
Please either leave the original bug open or file a new one to fix the embedding. We should not allow engine crashes on invalid input from applications, and applications are not required to use the framework for this.
Contributor
|
This may have broken web? |
Contributor
|
This pull request is not suitable for automatic merging in its current state.
|
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jun 15, 2022
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/plugins
that referenced
this pull request
Jun 15, 2022
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/plugins
that referenced
this pull request
Jun 15, 2022
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/plugins
that referenced
this pull request
Jun 15, 2022
8 tasks
camsim99
pushed a commit
to camsim99/flutter
that referenced
this pull request
Aug 10, 2022
…#104711) * Fix * Tests * Format * Empty line * Fix range test * Fix * Comments * trailing spaces * Fix tests * Comments
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 30, 2022
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/plugins
that referenced
this pull request
Aug 30, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes the confusion raised in #104578 by checking
TextRangeagainst the string content and report errors. This PR also fixes a bug found by this change on the conversion betweenTextInputActionand string.Users might construct invalid
TextRanges and send them to the engine. These errors will not be caught during construction, becauseTextEditingValueis a const class and can't verifyTextRange's fields. This might result in silent crashes on the engine side.To catch this on the framework side, I tried to add verification to
toJSONandfromJSON. However, this is not sufficient. The error can occur during a method call handler, where errors are swallowed and converted to an error reply to the method call. Unsurprisingly, the engine side doesn't handle errors. Even if it does, the error would be very hard to track due to lack of stacktrace.Therefore, this PR also wraps method call handlers with a
tryblock and reports errors toFlutterErrors.report. This is possible because we expect no errors fromtextinput's handlers.I would suggest making all errors during method call handlers reported in the same way. However that would be out of the scope of this PR.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.