-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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.
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
Fix crashes onKeyPress Android #18114
Fix crashes onKeyPress Android #18114
Conversation
It looks like the only test that failed is because of bad credentials :/ But thanks for your PR @joshyhargreaves! |
The bad credential issue will be fixed with PR #18111. It's OK to ignore the failing step, as it's only trying to run Danger, our PR linter. Everything else appears to check out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@hramos do you think it's worth also landing this PR on 0.54? |
@joshyhargreaves I opened #18128 where we can discuss further. |
Summary: There appear to be two different types of crashes related to the recent addition of `onKeyPress` on Android introduce in `0.53`. This PR addresses the cause of both of them. Firstly, it seems possible to get an `indexOutOfBoundsException` with some 3rd-party keyboards as observed in #17974 & #17922. I have simplified the backspace determining logic slightly, and also put in an explicit check for zero case so it is not possible to get an indexOutOfBoundsException & it should make sense in the context of the onKeyPress logic. Secondly, it appears that `EditText#onCreateInputConnection` can return null. In this case, if we set `null` to be the target of our subclass of `ReactEditTextInputConnectionWrapper`, we will see the crashes as seen [here](#17974 (comment)), whereby any of methods executed in the `InputConnection` interface can result in a crash. It's hard to reason about the state when `null` is returned from `onCreateInputConnection`, however I would might reason that any soft keyboard input cannot update the `EditText` with a `null` `input connection`, as there is no way of interfacing with the `EditText`. I'm am not sure, if there is a later point where we might return/set this input connection at a later point? As without the `InputConnection` onKeyPress will not work. But for now, this will fix this crash at least. I have not managed to reproduce these crashes myself yet, but users have confirmed that the `indexOutOfBounds` exception is fixed with the 'zero' case and has been confirmed on the respective issues #17974 (comment). For the `null` inputConnection target case, I have verified that explicitly setting the target as null in the constructor of `onCreateInputConnection` results in the same stack trace as the one linked. Here is also a [reference](https://github.com/stripe/stripe-android/pull/392/files#diff-6cc1685c98457d07fd4e2dd83f54d5bb) to the same issue closed with the same fix for another project on github. It is also important to verify that the behavior of `onKeyPress` still functions the same after this change, which can be verified by running the RNTesterProject and the `KeyboardEvents` section in `InputText`. The cases to check that I think are important to check are: - Cursor at beginning of input & backspace - Return key & return key at beginning of input - Select text then press delete - Selection then press a key - Space key - Different keyboard types This should not be a breaking change. [ANDROID] [BUGFIX] [TextInput] - Fixes crashes with TextInput introduced in 0.53. Closes #18114 Differential Revision: D7099570 Pulled By: hramos fbshipit-source-id: 75b2dc468c1ed398a33eb00487c6aa14ae04e5c2
Since it appears to me that some of the crashes were affecting users on |
TBH I think it should depend on how 'breaking' is upgrading to 0.54. If it's a big jump then it may be worth it, if it's ex. only upgrading Flow then no need. |
Yeah, let's actually wait for someone to request it (esp. who is blocked) than being proactive in this space. |
Summary: There appear to be two different types of crashes related to the recent addition of `onKeyPress` on Android introduce in `0.53`. This PR addresses the cause of both of them. Firstly, it seems possible to get an `indexOutOfBoundsException` with some 3rd-party keyboards as observed in facebook#17974 & facebook#17922. I have simplified the backspace determining logic slightly, and also put in an explicit check for zero case so it is not possible to get an indexOutOfBoundsException & it should make sense in the context of the onKeyPress logic. Secondly, it appears that `EditText#onCreateInputConnection` can return null. In this case, if we set `null` to be the target of our subclass of `ReactEditTextInputConnectionWrapper`, we will see the crashes as seen [here](facebook#17974 (comment)), whereby any of methods executed in the `InputConnection` interface can result in a crash. It's hard to reason about the state when `null` is returned from `onCreateInputConnection`, however I would might reason that any soft keyboard input cannot update the `EditText` with a `null` `input connection`, as there is no way of interfacing with the `EditText`. I'm am not sure, if there is a later point where we might return/set this input connection at a later point? As without the `InputConnection` onKeyPress will not work. But for now, this will fix this crash at least. I have not managed to reproduce these crashes myself yet, but users have confirmed that the `indexOutOfBounds` exception is fixed with the 'zero' case and has been confirmed on the respective issues facebook#17974 (comment). For the `null` inputConnection target case, I have verified that explicitly setting the target as null in the constructor of `onCreateInputConnection` results in the same stack trace as the one linked. Here is also a [reference](https://github.com/stripe/stripe-android/pull/392/files#diff-6cc1685c98457d07fd4e2dd83f54d5bb) to the same issue closed with the same fix for another project on github. It is also important to verify that the behavior of `onKeyPress` still functions the same after this change, which can be verified by running the RNTesterProject and the `KeyboardEvents` section in `InputText`. The cases to check that I think are important to check are: - Cursor at beginning of input & backspace - Return key & return key at beginning of input - Select text then press delete - Selection then press a key - Space key - Different keyboard types This should not be a breaking change. [ANDROID] [BUGFIX] [TextInput] - Fixes crashes with TextInput introduced in 0.53. Closes facebook#18114 Differential Revision: D7099570 Pulled By: hramos fbshipit-source-id: 75b2dc468c1ed398a33eb00487c6aa14ae04e5c2
I am experiencing this bug using react-native 0.53.0. I get the following stack trace after app crashes when deleting text in a element. Running on a USB connected Moto X2 via "react-native run-android". Is there a way for me to get this patch without upgrading to react-native 0.5.4? Stack Trace java.lang.IndexOutOfBoundsException: charAt: -1 < 0 |
Summary: There appear to be two different types of crashes related to the recent addition of `onKeyPress` on Android introduce in `0.53`. This PR addresses the cause of both of them. Firstly, it seems possible to get an `indexOutOfBoundsException` with some 3rd-party keyboards as observed in facebook#17974 & facebook#17922. I have simplified the backspace determining logic slightly, and also put in an explicit check for zero case so it is not possible to get an indexOutOfBoundsException & it should make sense in the context of the onKeyPress logic. Secondly, it appears that `EditText#onCreateInputConnection` can return null. In this case, if we set `null` to be the target of our subclass of `ReactEditTextInputConnectionWrapper`, we will see the crashes as seen [here](facebook#17974 (comment)), whereby any of methods executed in the `InputConnection` interface can result in a crash. It's hard to reason about the state when `null` is returned from `onCreateInputConnection`, however I would might reason that any soft keyboard input cannot update the `EditText` with a `null` `input connection`, as there is no way of interfacing with the `EditText`. I'm am not sure, if there is a later point where we might return/set this input connection at a later point? As without the `InputConnection` onKeyPress will not work. But for now, this will fix this crash at least. I have not managed to reproduce these crashes myself yet, but users have confirmed that the `indexOutOfBounds` exception is fixed with the 'zero' case and has been confirmed on the respective issues facebook#17974 (comment). For the `null` inputConnection target case, I have verified that explicitly setting the target as null in the constructor of `onCreateInputConnection` results in the same stack trace as the one linked. Here is also a [reference](https://github.com/stripe/stripe-android/pull/392/files#diff-6cc1685c98457d07fd4e2dd83f54d5bb) to the same issue closed with the same fix for another project on github. It is also important to verify that the behavior of `onKeyPress` still functions the same after this change, which can be verified by running the RNTesterProject and the `KeyboardEvents` section in `InputText`. The cases to check that I think are important to check are: - Cursor at beginning of input & backspace - Return key & return key at beginning of input - Select text then press delete - Selection then press a key - Space key - Different keyboard types This should not be a breaking change. [ANDROID] [BUGFIX] [TextInput] - Fixes crashes with TextInput introduced in 0.53. Closes facebook#18114 Differential Revision: D7099570 Pulled By: hramos fbshipit-source-id: 75b2dc468c1ed398a33eb00487c6aa14ae04e5c2
Summary: There appear to be two different types of crashes related to the recent addition of `onKeyPress` on Android introduce in `0.53`. This PR addresses the cause of both of them. Firstly, it seems possible to get an `indexOutOfBoundsException` with some 3rd-party keyboards as observed in facebook#17974 & facebook#17922. I have simplified the backspace determining logic slightly, and also put in an explicit check for zero case so it is not possible to get an indexOutOfBoundsException & it should make sense in the context of the onKeyPress logic. Secondly, it appears that `EditText#onCreateInputConnection` can return null. In this case, if we set `null` to be the target of our subclass of `ReactEditTextInputConnectionWrapper`, we will see the crashes as seen [here](facebook#17974 (comment)), whereby any of methods executed in the `InputConnection` interface can result in a crash. It's hard to reason about the state when `null` is returned from `onCreateInputConnection`, however I would might reason that any soft keyboard input cannot update the `EditText` with a `null` `input connection`, as there is no way of interfacing with the `EditText`. I'm am not sure, if there is a later point where we might return/set this input connection at a later point? As without the `InputConnection` onKeyPress will not work. But for now, this will fix this crash at least. I have not managed to reproduce these crashes myself yet, but users have confirmed that the `indexOutOfBounds` exception is fixed with the 'zero' case and has been confirmed on the respective issues facebook#17974 (comment). For the `null` inputConnection target case, I have verified that explicitly setting the target as null in the constructor of `onCreateInputConnection` results in the same stack trace as the one linked. Here is also a [reference](https://github.com/stripe/stripe-android/pull/392/files#diff-6cc1685c98457d07fd4e2dd83f54d5bb) to the same issue closed with the same fix for another project on github. It is also important to verify that the behavior of `onKeyPress` still functions the same after this change, which can be verified by running the RNTesterProject and the `KeyboardEvents` section in `InputText`. The cases to check that I think are important to check are: - Cursor at beginning of input & backspace - Return key & return key at beginning of input - Select text then press delete - Selection then press a key - Space key - Different keyboard types This should not be a breaking change. [ANDROID] [BUGFIX] [TextInput] - Fixes crashes with TextInput introduced in 0.53. Closes facebook#18114 Differential Revision: D7099570 Pulled By: hramos fbshipit-source-id: 75b2dc468c1ed398a33eb00487c6aa14ae04e5c2
Summary: There appear to be two different types of crashes related to the recent addition of `onKeyPress` on Android introduce in `0.53`. This PR addresses the cause of both of them. Firstly, it seems possible to get an `indexOutOfBoundsException` with some 3rd-party keyboards as observed in facebook#17974 & facebook#17922. I have simplified the backspace determining logic slightly, and also put in an explicit check for zero case so it is not possible to get an indexOutOfBoundsException & it should make sense in the context of the onKeyPress logic. Secondly, it appears that `EditText#onCreateInputConnection` can return null. In this case, if we set `null` to be the target of our subclass of `ReactEditTextInputConnectionWrapper`, we will see the crashes as seen [here](facebook#17974 (comment)), whereby any of methods executed in the `InputConnection` interface can result in a crash. It's hard to reason about the state when `null` is returned from `onCreateInputConnection`, however I would might reason that any soft keyboard input cannot update the `EditText` with a `null` `input connection`, as there is no way of interfacing with the `EditText`. I'm am not sure, if there is a later point where we might return/set this input connection at a later point? As without the `InputConnection` onKeyPress will not work. But for now, this will fix this crash at least. I have not managed to reproduce these crashes myself yet, but users have confirmed that the `indexOutOfBounds` exception is fixed with the 'zero' case and has been confirmed on the respective issues facebook#17974 (comment). For the `null` inputConnection target case, I have verified that explicitly setting the target as null in the constructor of `onCreateInputConnection` results in the same stack trace as the one linked. Here is also a [reference](https://github.com/stripe/stripe-android/pull/392/files#diff-6cc1685c98457d07fd4e2dd83f54d5bb) to the same issue closed with the same fix for another project on github. It is also important to verify that the behavior of `onKeyPress` still functions the same after this change, which can be verified by running the RNTesterProject and the `KeyboardEvents` section in `InputText`. The cases to check that I think are important to check are: - Cursor at beginning of input & backspace - Return key & return key at beginning of input - Select text then press delete - Selection then press a key - Space key - Different keyboard types This should not be a breaking change. [ANDROID] [BUGFIX] [TextInput] - Fixes crashes with TextInput introduced in 0.53. Closes facebook#18114 Differential Revision: D7099570 Pulled By: hramos fbshipit-source-id: 75b2dc468c1ed398a33eb00487c6aa14ae04e5c2
Summary: There appear to be two different types of crashes related to the recent addition of `onKeyPress` on Android introduce in `0.53`. This PR addresses the cause of both of them. Firstly, it seems possible to get an `indexOutOfBoundsException` with some 3rd-party keyboards as observed in facebook#17974 & facebook#17922. I have simplified the backspace determining logic slightly, and also put in an explicit check for zero case so it is not possible to get an indexOutOfBoundsException & it should make sense in the context of the onKeyPress logic. Secondly, it appears that `EditText#onCreateInputConnection` can return null. In this case, if we set `null` to be the target of our subclass of `ReactEditTextInputConnectionWrapper`, we will see the crashes as seen [here](facebook#17974 (comment)), whereby any of methods executed in the `InputConnection` interface can result in a crash. It's hard to reason about the state when `null` is returned from `onCreateInputConnection`, however I would might reason that any soft keyboard input cannot update the `EditText` with a `null` `input connection`, as there is no way of interfacing with the `EditText`. I'm am not sure, if there is a later point where we might return/set this input connection at a later point? As without the `InputConnection` onKeyPress will not work. But for now, this will fix this crash at least. I have not managed to reproduce these crashes myself yet, but users have confirmed that the `indexOutOfBounds` exception is fixed with the 'zero' case and has been confirmed on the respective issues facebook#17974 (comment). For the `null` inputConnection target case, I have verified that explicitly setting the target as null in the constructor of `onCreateInputConnection` results in the same stack trace as the one linked. Here is also a [reference](https://github.com/stripe/stripe-android/pull/392/files#diff-6cc1685c98457d07fd4e2dd83f54d5bb) to the same issue closed with the same fix for another project on github. It is also important to verify that the behavior of `onKeyPress` still functions the same after this change, which can be verified by running the RNTesterProject and the `KeyboardEvents` section in `InputText`. The cases to check that I think are important to check are: - Cursor at beginning of input & backspace - Return key & return key at beginning of input - Select text then press delete - Selection then press a key - Space key - Different keyboard types This should not be a breaking change. [ANDROID] [BUGFIX] [TextInput] - Fixes crashes with TextInput introduced in 0.53. Closes facebook#18114 Differential Revision: D7099570 Pulled By: hramos fbshipit-source-id: 75b2dc468c1ed398a33eb00487c6aa14ae04e5c2
Do you know when this will be merged over, this crash is affecting us on android. |
This fix has been on master for close to a year. |
Motivation
There appear to be two different types of crashes related to the recent addition of
onKeyPress
on Android introduce in0.53
. This PR addresses the cause of both of them.Firstly, it seems possible to get an
indexOutOfBoundsException
with some 3rd-party keyboards as observed in #17974 & #17922. I have simplified the backspace determining logic slightly, and also put in an explicit check for zero case so it is not possible to get an indexOutOfBoundsException & it should make sense in the context of the onKeyPress logic.Secondly, it appears that
EditText#onCreateInputConnection
can return null. In this case, if we setnull
to be the target of our subclass ofReactEditTextInputConnectionWrapper
, we will see the crashes as seen here, whereby any of methods executed in theInputConnection
interface can result in a crash. It's hard to reason about the state whennull
is returned fromonCreateInputConnection
, however I would might reason that any soft keyboard input cannot update theEditText
with anull
input connection
, as there is no way of interfacing with theEditText
. I'm am not sure, if there is a later point where we might return/set this input connection at a later point? As without theInputConnection
onKeyPress will not work. But for now, this will fix this crash at least.Test Plan
I have not managed to reproduce these crashes myself yet, but users have confirmed that the
indexOutOfBounds
exception is fixed with the 'zero' case and has been confirmed on the respective issues #17974 (comment).For the
null
inputConnection target case, I have verified that explicitly setting the target as null in the constructor ofonCreateInputConnection
results in the same stack trace as the one linked. Here is also a reference to the same issue closed with the same fix for another project on github.It is also important to verify that the behavior of
onKeyPress
still functions the same after this change, which can be verified by running the RNTesterProject and theKeyboardEvents
section inInputText
.The cases to check that I think are important to check are:
This should not be a breaking change.
Release Notes
[ANDROID] [BUGFIX] [TextInput] - Fixes crashes with TextInput introduced in 0.53.