Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Check for invalid indexes when performing InputAdpator backspace. #9322

Merged
merged 8 commits into from
Jun 17, 2019
Merged

Check for invalid indexes when performing InputAdpator backspace. #9322

merged 8 commits into from
Jun 17, 2019

Conversation

GaryQian
Copy link
Contributor

@GaryQian GaryQian commented Jun 13, 2019 •
edited
Loading

Selection.getSelectionStart can return -1, which causes an index out of bounds. In the case of a invalid deletion, unspecifiable selections will not be deleted.

b/135036951
Fixes flutter/flutter#33642

@GaryQian GaryQian added the crash label Jun 13, 2019
@GaryQian GaryQian mentioned this pull request Jun 13, 2019
Closed
@GaryQian
Copy link
Contributor Author

I am currently in need of a repro of the bug to ensure this does what I intend it to do.

@GaryQian
Copy link
Contributor Author

@justinmc for repro

@justinmc
Copy link
Contributor

I couldn't reproduce this with a quick try on 2 phones lying around, but tomorrow I'll try again with the Samsung phone that I got it on before.

@justinmc
Copy link
Contributor

Sorry, going to try this again on Monday. I have the Samsung phone but I can't get the engine to compile for it on my Mac :(

@GaryQian
Copy link
Contributor Author

GaryQian commented Jun 14, 2019 •
edited
Loading

I just managed to repro the crash with an arabic keyboard I can continue with this now! Thanks for trying though!

@justinmc
Copy link
Contributor

Ah good, no problem!

@GaryQian
Copy link
Contributor Author

After exploring this more, this will need additional work to handle RTL, as well as other indexing issues.

@GaryQian
Copy link
Contributor Author

GaryQian commented Jun 15, 2019 •
edited
Loading

This resolves the crash portion of backspacing in Chinese and some RTL keyboards, but I have discovered some (existing, non-crashing) incompatibility when backspacing using the chinese keyboard on mixed RTL/LTR text. I will resolve that behavior in a separate PR.

This change should not cause any behavior regressions other than eliminating crashes.

justinmc
justinmc approved these changes Jun 17, 2019
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Do we have a way to log a warning when the index is out of bounds in clampIndexToEditable? I just figure we might want to know if that happens since it could indicate a bug elsewhere, even if this is able to recover.

shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java Outdated Show resolved Hide resolved
@GaryQian GaryQian merged commit 675033f into flutter:master Jun 17, 2019
@GaryQian
Copy link
Contributor Author

Fixes flutter/flutter#33642

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 18, 2019
@engine-flutter-autoroll
675033f Check for invalid indexes when performing InputAdpator backsp…
16de79b 
…ace. (flutter/engine#9322)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 18, 2019
Merged
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Jun 18, 2019
@engine-flutter-autoroll
Roll engine 20d3861..05c034e (10 commits) (#34621)
3703e88 
flutter/engine@20d3861...05c034e

git log 20d3861..05c034e --no-merges --oneline
05c034e Update component manifests for ambient replace-as-executable (flutter/engine#9350)
66022ce Roll src/third_party/skia 55091020435c..4b203ad7ac02 (6 commits) (flutter/engine#9352)
fcff2d6 Use the DartServiceIsolate status callback to publish the observatory URI to the Android embedder (flutter/engine#9337)
ea7ca98 Send the isolate service ID from the engine to the embedder (flutter/engine#9324)
675033f Check for invalid indexes when performing InputAdpator backspace. (flutter/engine#9322)
e5918d1 Roll src/third_party/skia 92b81e14d9c2..55091020435c (6 commits) (flutter/engine#9348)
e00ac47 Reorganize darwin for shared ios/macOS (flutter/engine#9255)
9da409c Fix crash on Huawei device with AndroidView (flutter/engine#9192)
a0f8554 Removed an unused class definition for iOS code. (flutter/engine#9346)
96a1a84 Replace lock_guard with scoped_lock and use class template argument deduction. (flutter/engine#9338)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff ([email protected]), and stop
the roller if necessary.
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 19, 2019
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 19, 2019
@engine-flutter-autoroll
675033f Check for invalid indexes when performing InputAdpator backsp…
4a0ddcc 
…ace. (flutter/engine#9322)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 19, 2019
@engine-flutter-autoroll
675033f Check for invalid indexes when performing InputAdpator backsp…
7f219fd 
…ace. (flutter/engine#9322)
This was referenced Jun 19, 2019
Closed
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 19, 2019
@engine-flutter-autoroll
675033f Check for invalid indexes when performing InputAdpator backsp…
41f7968 
…ace. (flutter/engine#9322)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 19, 2019
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 19, 2019
@engine-flutter-autoroll
675033f Check for invalid indexes when performing InputAdpator backsp…
e793644 
…ace. (flutter/engine#9322)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 19, 2019
@engine-flutter-autoroll
675033f Check for invalid indexes when performing InputAdpator backsp…
0247fad 
…ace. (flutter/engine#9322)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 19, 2019
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 19, 2019
@engine-flutter-autoroll
675033f Check for invalid indexes when performing InputAdpator backsp…
049759c 
…ace. (flutter/engine#9322)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 19, 2019
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 19, 2019
@engine-flutter-autoroll
675033f Check for invalid indexes when performing InputAdpator backsp…
4dce0be 
…ace. (flutter/engine#9322)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 19, 2019
Closed
@justinmc justinmc mentioned this pull request Jun 19, 2019
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 19, 2019
@engine-flutter-autoroll
675033f Check for invalid indexes when performing InputAdpator backsp…
8cc0be0 
…ace. (flutter/engine#9322)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 19, 2019
Closed
johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019
@engine-flutter-autoroll @johnsonmh
Roll engine 20d3861..05c034e (10 commits) (flutter#34621)
a8dad04 
flutter/engine@20d3861...05c034e

git log 20d3861..05c034e --no-merges --oneline
05c034e Update component manifests for ambient replace-as-executable (flutter/engine#9350)
66022ce Roll src/third_party/skia 55091020435c..4b203ad7ac02 (6 commits) (flutter/engine#9352)
fcff2d6 Use the DartServiceIsolate status callback to publish the observatory URI to the Android embedder (flutter/engine#9337)
ea7ca98 Send the isolate service ID from the engine to the embedder (flutter/engine#9324)
675033f Check for invalid indexes when performing InputAdpator backspace. (flutter/engine#9322)
e5918d1 Roll src/third_party/skia 92b81e14d9c2..55091020435c (6 commits) (flutter/engine#9348)
e00ac47 Reorganize darwin for shared ios/macOS (flutter/engine#9255)
9da409c Fix crash on Huawei device with AndroidView (flutter/engine#9192)
a0f8554 Removed an unused class definition for iOS code. (flutter/engine#9346)
96a1a84 Replace lock_guard with scoped_lock and use class template argument deduction. (flutter/engine#9338)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff ([email protected]), and stop
the roller if necessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinmc justinmc justinmc approved these changes

@jason-simmons jason-simmons Awaiting requested review from jason-simmons

@chinmaygarde chinmaygarde Awaiting requested review from chinmaygarde

Assignees
No one assigned
Labels
affects: text input cla: yes crash
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

App crash when deleting the password in TextFormField with obscureText is true(Use the keyboard's del button)
3 participants
@GaryQian @justinmc @googlebot