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.

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

IndexOutOfBoundsException when using Google Indic Keyboard with RN 0.53.0 #17974

Closed
SuhairZain opened this issue Feb 14, 2018 · 39 comments
Closed
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@SuhairZain
Copy link
Contributor

Is this a bug report?

Yes

Have you read the Contributing Guidelines?

Yes

Environment

Environment:
OS: macOS High Sierra 10.13.2
Node: 8.4.0
Yarn: 0.27.5
npm: 5.3.0
Watchman: 4.9.0
Xcode: Xcode 9.2 Build version 9C40b
Android Studio: 3.0 AI-171.4443003

Packages: (wanted => installed)
react: 16.2.0 => 16.2.0
react-native: 0.53.0 => 0.53.0

Target Platform: Android (6.0.1)

Steps to Reproduce

  1. Create and run a new project with RN 0.53.0, using react-native init. (Most likely exists for projects created using other methods as well, didn't check.)
  2. Switch the keyboard to Google Indic Keyboard from phone's input settings
  3. Steps for reproducing the crash with emojis:
    a. Create a TextInput with no additional props (or switch to the master branch of the provided repo).
    a. Type an emoji and press the backspace twice.
  4. Steps for reproducing the crash with any character:
    a. Create a TextInput with autoCorrect={false} (or switch to the auto-correct branch of the provided repo).
    a. Type any character and press the backspace once.

Expected Behavior

The emojis and characters get cleared with no crashes

Actual Behavior

screenshot_2018-02-14-10-04-04-422_com android settings

The app crashes on clearing the emojis, with no additional props set on the TextInput
2018-02-14 10_37_49
The app crashes on clearing the last character in case autoCorrect={false} is set
2018-02-14 10_38_17

Reproducible Demo

https://github.com/SuhairZain/react-native-gindic-keyboard-crash
The master branch crashes when deleting an emoji character
The auto-correct branch crashes when deleting the last character in the input

The reason that I've created this issue here rather than on the Google Indic keyboard repo is that I can confirm that this bug does not exist with 0.47.1, which is the version our app was on before we upgraded. This is a bug that was introduced in one of the versions from then to 0.53.0.

@joshjhargreaves
Copy link
Contributor

joshjhargreaves commented Feb 15, 2018

@SuhairZain can you post the stack trace?

@SuhairZain
Copy link
Contributor Author

@joshyhargreaves Here's all the data that I have about the crash:

{
  "client": {
    "clientUrl": "https://github.com/MindscapeHQ/raygun4android",
    "name": "Raygun4Android",
    "version": "3.0.0"
  },
  "context": {
    "identifier": "xxxxxxxxxxxxxxxxx"
  },
  "environment": {
    "architecture": "armeabi-v7a",
    "availablePhysicalMemory": 620,
    "availableVirtualMemory": 0,
    "board": "msm8937",
    "brand": "Xiaomi",
    "currentOrientation": "Portrait",
    "deviceCode": "land",
    "deviceName": "Redmi 3S",
    "diskSpaceFree": 5861,
    "locale": "en_US",
    "oSVersion": "6.0.1",
    "osSDKVersion": "23",
    "processorCount": 8,
    "totalPhysicalMemory": 2843,
    "totalVirtualMemory": 0,
    "utcOffset": 5.0,
    "windowsBoundHeight": 1280,
    "windowsBoundWidth": 720
  },
  "error": {
    "className": "java.lang.IndexOutOfBoundsException",
    "message": "IndexOutOfBoundsException: charAt: -1 < 0",
    "stackTrace": [
      {
        "className": "android.text.SpannableStringBuilder",
        "fileName": "SpannableStringBuilder.java",
        "lineNumber": 117,
        "methodName": "charAt"
      },
      {
        "className": "com.facebook.react.views.textinput.ReactEditTextInputConnectionWrapper",
        "fileName": "ReactEditTextInputConnectionWrapper.java",
        "lineNumber": 104,
        "methodName": "setComposingText"
      },
      {
        "className": "com.android.internal.view.IInputConnectionWrapper",
        "fileName": "IInputConnectionWrapper.java",
        "lineNumber": 340,
        "methodName": "executeMessage"
      },
      {
        "className": "com.android.internal.view.IInputConnectionWrapper$MyHandler",
        "fileName": "IInputConnectionWrapper.java",
        "lineNumber": 78,
        "methodName": "handleMessage"
      },
      {
        "className": "android.os.Handler",
        "fileName": "Handler.java",
        "lineNumber": 102,
        "methodName": "dispatchMessage"
      },
      {
        "className": "android.os.Looper",
        "fileName": "Looper.java",
        "lineNumber": 157,
        "methodName": "loop"
      },
      {
        "className": "android.app.ActivityThread",
        "fileName": "ActivityThread.java",
        "lineNumber": 5555,
        "methodName": "main"
      },
      {
        "className": "java.lang.reflect.Method",
        "fileName": "Method.java",
        "lineNumber": -2,
        "methodName": "invoke"
      },
      {
        "className": "com.android.internal.os.ZygoteInit$MethodAndArgsCaller",
        "fileName": "ZygoteInit.java",
        "lineNumber": 745,
        "methodName": "run"
      },
      {
        "className": "com.android.internal.os.ZygoteInit",
        "fileName": "ZygoteInit.java",
        "lineNumber": 635,
        "methodName": "main"
      }
    ]
  },
  "machineName": "Redmi 3S",
  "request": {
    "iPAddress": [
      "xxxxxxxxxxxxx"
    ],
    "networkConnectivityState": "Connected - WiFi"
  },
  "tags": [
    "UnhandledException"
  ],
  "user": {
    "identifier": "xxxxxxxxxxx"
  },
  "version": "xxxxxxxxxxx",
  "OccurredOn": "2018-02-14T04:27:24Z"
}

@joshjhargreaves
Copy link
Contributor

@SuhairZain are you able to reproduce on a simulator? Or is this a device?

@SuhairZain
Copy link
Contributor Author

@joshyhargreaves This is a device. I tried to reproduce it on an emulator but it didn't have the Indic keyboard. If it'd help you, I'll try to install the keyboard on an emulator and see.

@joshjhargreaves
Copy link
Contributor

@SuhairZain I installed the Indic keyboard on an emulator by downloading the APK and installing with adb however no crash for me on the emulator itself; I will try on a device.

Could you try the fix that I just pushed to a branch, you can either update your react-native dependency in your package.json or initialise a new project with react-native init fixedOnKeyPress --version joshyhargreaves/react-native#89eaafe6aa9f98a32ae48a400ebfea8295918bb6

@SuhairZain
Copy link
Contributor Author

SuhairZain commented Feb 15, 2018

@joshyhargreaves I was unable to reproduce it on an emulator as well. I think you wouldn't be able to reproduce this on a device either. Seems like I'm running an old version of Google Indic keyboard on my device, even though I have auto-updates on. The version I have on the emulator is 3.2.5.164561151-x86, whereas on my device it's 3.2.1.127799310-armeabi-v7a.

I think it'd be wise for me to not upgrade to the latest version of the keyboard on the device since it may be difficult to revert to the old version. What do you suggest?

@joshjhargreaves
Copy link
Contributor

@SuhairZain could you build your repo-project for your device with the changes I just pushed and let me know if it still crashes on your device?

@joshjhargreaves
Copy link
Contributor

joshjhargreaves commented Feb 15, 2018

I greatly suspect it will have fixed the issue. For the onKeyPress API perspective, it should still make sense.

@SuhairZain
Copy link
Contributor Author

SuhairZain commented Feb 15, 2018 via email

@joshjhargreaves
Copy link
Contributor

joshjhargreaves commented Feb 15, 2018

@SuhairZain, I know what the issue was as I who introduced it while implementing onKeyPress for Android. If you look at my earlier comment you can pull the fix from my branch and test it.

@SuhairZain
Copy link
Contributor Author

@joshyhargreaves I'm sorry. I was replying via email and didn't see your 1st comment. I'll check right away.

@SuhairZain
Copy link
Contributor Author

SuhairZain commented Feb 15, 2018

@joshyhargreaves I'm unable to build a new project created using the code you posted above. Getting the following error:

Scanning folders for symlinks in /Users/suhairzain/Documents/fixedOnKeyPress/node_modules (16ms)
JS server already running.
Building and installing the app on the device (cd android && ./gradlew installDebug)...
Incremental java compilation is an incubating feature.
:app:preBuild UP-TO-DATE
:app:preDebugBuild UP-TO-DATE
:app:checkDebugManifest
:app:preReleaseBuild UP-TO-DATE
:app:prepareComAndroidSupportAppcompatV72301Library
:app:prepareComAndroidSupportRecyclerviewV72301Library
:app:prepareComAndroidSupportSupportV42301Library
:app:prepareComFacebookFrescoDrawee081Library
:app:prepareComFacebookFrescoFbcore081Library
:app:prepareComFacebookFrescoFresco081Library
:app:prepareComFacebookFrescoImagepipeline081Library
:app:prepareComFacebookFrescoImagepipelineOkhttp081Library
:app:prepareComFacebookReactReactNative0201Library
:app:prepareOrgWebkitAndroidJscR174650Library
:app:prepareDebugDependencies
:app:compileDebugAidl
:app:compileDebugRenderscript
:app:generateDebugBuildConfig
:app:mergeDebugShaders
:app:compileDebugShaders
:app:generateDebugAssets
:app:mergeDebugAssets
:app:generateDebugResValues
:app:generateDebugResources
> Building 57% > :app:mergeDebugResources^C
➜  fixedOnKeyPress react-native run-android
Scanning folders for symlinks in /Users/suhairzain/Documents/fixedOnKeyPress/node_modules (12ms)
Starting JS server...
Building and installing the app on the device (cd android && ./gradlew installDebug)...
Incremental java compilation is an incubating feature.
:app:preBuild UP-TO-DATE
:app:preDebugBuild UP-TO-DATE
:app:checkDebugManifest
:app:preReleaseBuild UP-TO-DATE
:app:prepareComAndroidSupportAppcompatV72301Library UP-TO-DATE
:app:prepareComAndroidSupportRecyclerviewV72301Library UP-TO-DATE
:app:prepareComAndroidSupportSupportV42301Library UP-TO-DATE
:app:prepareComFacebookFrescoDrawee081Library UP-TO-DATE
:app:prepareComFacebookFrescoFbcore081Library UP-TO-DATE
:app:prepareComFacebookFrescoFresco081Library UP-TO-DATE
:app:prepareComFacebookFrescoImagepipeline081Library UP-TO-DATE
:app:prepareComFacebookFrescoImagepipelineOkhttp081Library UP-TO-DATE
:app:prepareComFacebookReactReactNative0201Library UP-TO-DATE
:app:prepareOrgWebkitAndroidJscR174650Library UP-TO-DATE
:app:prepareDebugDependencies
:app:compileDebugAidl UP-TO-DATE
:app:compileDebugRenderscript UP-TO-DATE
:app:generateDebugBuildConfig UP-TO-DATE
:app:mergeDebugShaders UP-TO-DATE
:app:compileDebugShaders UP-TO-DATE
:app:generateDebugAssets UP-TO-DATE
:app:mergeDebugAssets UP-TO-DATE
:app:generateDebugResValues UP-TO-DATE
:app:generateDebugResources UP-TO-DATE
:app:mergeDebugResources
:app:bundleDebugJsAndAssets SKIPPED
:app:processDebugManifest
:app:processDebugResources
warning: string 'catalyst_debugjs' has no default translation.
warning: string 'catalyst_element_inspector' has no default translation.
warning: string 'catalyst_jsload_error' has no default translation.
warning: string 'catalyst_jsload_message' has no default translation.
warning: string 'catalyst_jsload_title' has no default translation.
warning: string 'catalyst_reloadjs' has no default translation.
warning: string 'catalyst_settings' has no default translation.
warning: string 'catalyst_settings_title' has no default translation.

:app:generateDebugSources
:app:incrementalDebugJavaCompilationSafeguard
:app:compileDebugJavaWithJavac
:app:compileDebugJavaWithJavac - is not incremental (e.g. outputs have changed, no previous execution, etc.).
/Users/suhairzain/Documents/fixedOnKeyPress/android/app/src/main/java/com/fixedonkeypress/MainApplication.java:5: error: cannot find symbol
import com.facebook.react.ReactApplication;
                         ^
  symbol:   class ReactApplication
  location: package com.facebook.react
/Users/suhairzain/Documents/fixedOnKeyPress/android/app/src/main/java/com/fixedonkeypress/MainApplication.java:6: error: cannot find symbol
import com.facebook.react.ReactNativeHost;
                         ^
  symbol:   class ReactNativeHost
  location: package com.facebook.react
/Users/suhairzain/Documents/fixedOnKeyPress/android/app/src/main/java/com/fixedonkeypress/MainApplication.java:14: error: cannot find symbol
public class MainApplication extends Application implements ReactApplication {
                                                            ^
  symbol: class ReactApplication
/Users/suhairzain/Documents/fixedOnKeyPress/android/app/src/main/java/com/fixedonkeypress/MainApplication.java:16: error: cannot find symbol
  private final ReactNativeHost mReactNativeHost = new ReactNativeHost(this) {
                ^
  symbol:   class ReactNativeHost
  location: class MainApplication
/Users/suhairzain/Documents/fixedOnKeyPress/android/app/src/main/java/com/fixedonkeypress/MainApplication.java:36: error: cannot find symbol
  public ReactNativeHost getReactNativeHost() {
         ^
  symbol:   class ReactNativeHost
  location: class MainApplication
/Users/suhairzain/Documents/fixedOnKeyPress/android/app/src/main/java/com/fixedonkeypress/MainApplication.java:16: error: cannot find symbol
  private final ReactNativeHost mReactNativeHost = new ReactNativeHost(this) {
                                                       ^
  symbol:   class ReactNativeHost
  location: class MainApplication
/Users/suhairzain/Documents/fixedOnKeyPress/android/app/src/main/java/com/fixedonkeypress/MainApplication.java:35: error: method does not override or implement a method from a supertype
  @Override
  ^
/Users/suhairzain/Documents/fixedOnKeyPress/android/app/src/main/java/com/fixedonkeypress/MainActivity.java:5: error: MainActivity is not abstract and does not override abstract method getPackages() in ReactActivity
public class MainActivity extends ReactActivity {
       ^
8 errors
:app:compileDebugJavaWithJavac FAILED

@joshjhargreaves
Copy link
Contributor

@SuhairZain sorry, that's my bad, I forgot that android needs to be built from source.

@SuhairZain
Copy link
Contributor Author

@joshyhargreaves In that case, I'll build it from source and try tomorrow. I see that it'll take some time.

@joshjhargreaves
Copy link
Contributor

joshjhargreaves commented Feb 15, 2018

You can build react-native in-place in node_modules by running the command: ./gradlew :ReactAndroid:installArchives. Thanks let me know how you get on!

@ccdwyer
Copy link

ccdwyer commented Feb 15, 2018

I'll try building the APK this way and sending it over to my team that is onsite. I won't hear back from them until tomorrow afternoon/Monday most likely, but I'll let you know.

@joshjhargreaves
Copy link
Contributor

@ccdwyer, sounds good, thank you, and we can go from there. Sorry about this crash, it was a definite oversight on my part.

@ccdwyer
Copy link

ccdwyer commented Feb 15, 2018

@joshyhargreaves Pardon my ignorance, where am I supposed to run the ./gradlew :ReactAndroid:installArchives command? I tried running that inside my android folder and got:

  • What went wrong:
    Project 'ReactAndroid' not found in root project 'MyProjectName'.

@SuhairZain
Copy link
Contributor Author

@joshyhargreaves @ccdwyer I'm getting the following error when trying to build from source as described in Building React Native from source.

Scanning folders for symlinks in /Users/suhairzain/Documents/TextInputBug/node_modules (20ms)
Starting JS server...
Building and installing the app on the device (cd android && ./gradlew installDebug)...
Incremental java compilation is an incubating feature.

FAILURE: Build failed with an exception.

* What went wrong:
A problem occurred configuring project ':app'.
> java.lang.NullPointerException (no error message)

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.

BUILD FAILED

Total time: 11.746 secs
Could not install the app on the device, read the error above for details.
Make sure you have an Android emulator running or a device connected and have
set up your Android development environment:
https://facebook.github.io/react-native/docs/getting-started.html

If you're able to build from source, would it be possible for you to use your fork with the example repo I had provided and send me a universal APK?

@joshjhargreaves
Copy link
Contributor

@ccdwyer, you have to run this from within node_modules/React-Native.

When React Native is published to npm, it runs this command and pushes the built assets as part of the package. As I’ve just pushed this to a branch, and including this repository as a git dependency; this step hasn’t happened.

@joshjhargreaves
Copy link
Contributor

What I didn’t have time to do, was figure out the location of these built assets, and force add them to git and push them. I don’t have my laptop with me and I’m away this weekend, otherwise I’d do it!

@SuhairZain
Copy link
Contributor Author

@joshyhargreaves I get it. However, it seems that Play Store finally decided to update Google Indic keyboard on my device yesterday. I'm no longer having crashes. Will have to find an older version and install it on an emulator using adb. Where did you download the latest APK from?

@ccdwyer
Copy link

ccdwyer commented Feb 19, 2018

@joshyhargreaves Do you think it might be possible for you to add those assets? I encountered some issues trying to build react native myself, so I haven't been able to test the change.

@joshjhargreaves
Copy link
Contributor

@ccdwyer no problem, I will do this evening as currently AFK!

@ccdwyer
Copy link

ccdwyer commented Feb 19, 2018

@joshyhargreaves Thanks!

@joshjhargreaves
Copy link
Contributor

@ccdwyer I've pushed the built assets: update the npm dependency to joshyhargreaves/react-native#616457a6a8ccc0c0b717646d2a66af68df358152

@ccdwyer
Copy link

ccdwyer commented Feb 20, 2018

@joshyhargreaves Thanks! I built an APK with it and sent it over to my team.

@ccdwyer
Copy link

ccdwyer commented Feb 23, 2018

@joshyhargreaves Just letting you know I didn't forget about this. My team is supposed to test the APK with the fix tomorrow. Things move a little slower in the midwest. :)

@joshjhargreaves
Copy link
Contributor

@ccdwyer any news?

@ccdwyer
Copy link

ccdwyer commented Feb 23, 2018

@joshyhargreaves Nothing yet, pinged them again for a response.

@ccdwyer
Copy link

ccdwyer commented Feb 23, 2018

@joshyhargreaves It appears that the crash is gone for us with your change. Thanks!

@kelset
Copy link
Contributor

kelset commented Feb 26, 2018

👋 @joshyhargreaves

I think that the issue OP posted is actually just one of the ways the bug can manifest itself (and #17922 is another) - this weekend we released the latest update to our app with RN 0.53 and I have 3 different crash reports that seems to be all related to the same root issue:

Fatal Exception: java.lang.NullPointerException: Attempt to invoke interface method 'android.os.Handler android.view.inputmethod.InputConnection.getHandler()' on a null object reference
       at android.view.inputmethod.InputConnectionWrapper.getHandler(InputConnectionWrapper.java:262)
       at android.view.inputmethod.InputMethodManager.startInputInner(InputMethodManager.java:1703)
       at android.view.inputmethod.InputMethodManager$H.handleMessage(InputMethodManager.java:558)
       at android.os.Handler.dispatchMessage(Handler.java:102)
       at android.os.Looper.loop(Looper.java:154)
       at android.app.ActivityThread.main(ActivityThread.java:6692)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1468)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1358)
Fatal Exception: java.lang.NullPointerException: Attempt to invoke interface method 'android.view.inputmethod.ExtractedText android.view.inputmethod.InputConnection.getExtractedText(android.view.inputmethod.ExtractedTextRequest, int)' on a null object reference
       at android.view.inputmethod.InputConnectionWrapper.getExtractedText(InputConnectionWrapper.java:62)
       at com.android.internal.view.IInputConnectionWrapper.executeMessage(IInputConnectionWrapper.java:273)
       at com.android.internal.view.IInputConnectionWrapper$MyHandler.handleMessage(IInputConnectionWrapper.java:78)
       at android.os.Handler.dispatchMessage(Handler.java:102)
       at android.os.Looper.loop(Looper.java:154)
       at android.app.ActivityThread.main(ActivityThread.java:5523)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:739)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:629)
Fatal Exception: java.lang.NullPointerException: Attempt to invoke interface method 'boolean android.view.inputmethod.InputConnection.finishComposingText()' on a null object reference
       at android.view.inputmethod.InputConnectionWrapper.finishComposingText(InputConnectionWrapper.java:78)
       at android.view.inputmethod.InputMethodManager.checkFocusNoStartInput(InputMethodManager.java:1543)
       at android.view.inputmethod.InputMethodManager.onWindowFocus(InputMethodManager.java:1587)
       at android.view.ViewRootImpl$ViewRootHandler.handleMessage(ViewRootImpl.java:3958)
       at android.os.Handler.dispatchMessage(Handler.java:111)
       at android.os.Looper.loop(Looper.java:214)
       at android.app.ActivityThread.main(ActivityThread.java:6102)
       at java.lang.reflect.Method.invoke(Method.java)
       at java.lang.reflect.Method.invoke(Method.java:372)
       at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1028)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:823)

Sadly I can't post much more of the stacktrace since it comes from the minified prod version; but I strongly believe the fix you linked earlier will solve these crashes too (I can't dive deeper into creating a repro or doing a new release pointing at your branch because of higher prior tasks this week).

It's still quite a small 'surface', since we are talking about a precise set of actions on a custom keyboard on Android (overall, we got less than 20 crashes reported by up to 6 users - which is a .00x% of our userbase) so no rush, but are you planning on doing a PR with it?

I don't think, since it's already late Feb, that it would be merged & cherry picked into a new 0.53.x release, but I think it would be possible to have it in 0.54 💪

(ps: ofc if there's anything that I can do to help let me know!)

@joshjhargreaves
Copy link
Contributor

Hey @kelset, thank you for the crash reports, I had actually suspected that their might be a few different ways that this bug could manifest itself.
One of the main issues I’ve had actually, is I haven’t been able to repro it myself actually, but I will put in some more effort. I’m entirely sure, that the fix I posted wouldn’t result in any more indexOutOfBoundsExceptions.

Looking at your stack traces, I’m not actually that confident that that the fix I posted previously would fix these, as it seems like the reference to the wrapped InputConnection can turn null, but I will do a bit of thinking/researching over the next few hours.

I will absolutely put in a pull request for this, I think it may be a good idea also to only enable this codepath if using ‘onKeyPress’, and so I will have a look at that also!

Let’s just get this fixed, thanks again for the crash reports.

@joshjhargreaves
Copy link
Contributor

joshjhargreaves commented Feb 26, 2018

@kelset it looks like you need to guard against null being returned by super.onCreateInputConnection(outAttrs) and that would almost definitely explain the null pointer exceptions you're seeing in your crashes, when then InputConnection target is null!
Reference & corresponding line in react-native source.

It would be helpful to know exactly the particular cases when oncreateInputConnection can return null as returning null from onCreateInputConnection in these cases would stop onKeyPress from working, and when/if there'd be an appropriate time to recreate the input connection. There is also the option to implement every method in InputConnection in our subclass and guard against the null target case, but that doesn't feel right to me.

edit: but I'd say, in my judgement, adding the indexOutOfBounds fix and this one would be good to get landed!

@kelset
Copy link
Contributor

kelset commented Feb 26, 2018

Hey, thanks for the clarification.
Yes I think replicating the fix as it has been done in the PR linked could be a good 'first approach' to have something that will prevent the crash. I believe that on the PR eventually there can be a discussion if this is the best approach or another road should be taken.

@joshjhargreaves
Copy link
Contributor

@kelset I'll put in a PR to master in a few hours. Hopefully @kmagiera won't mind giving it a review!

facebook-github-bot pushed a commit that referenced this issue Feb 27, 2018
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
@joshjhargreaves
Copy link
Contributor

This should be addressed in, so I will close: b60a727. Please re-open if the problem somehow persists.

@joshjhargreaves
Copy link
Contributor

joshjhargreaves commented Feb 27, 2018

I'lll re-open until we know if this commit make it into the 0.54 release.

@grabbou
Copy link
Contributor

grabbou commented Mar 1, 2018

I'll close it as I am cherry picking it right now.

@grabbou grabbou closed this as completed Mar 1, 2018
grabbou pushed a commit that referenced this issue Mar 1, 2018
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
cpirich pushed a commit to tracemeinc/react-native that referenced this issue Mar 22, 2018
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
fcangialosi pushed a commit to inizio-inc/react-native that referenced this issue Jul 17, 2018
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
fcangialosi pushed a commit to inizio-inc/react-native that referenced this issue Jul 18, 2018
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
artdent pushed a commit to triggr/react-native that referenced this issue Jul 19, 2018
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
artdent pushed a commit to triggr/react-native that referenced this issue Jul 19, 2018
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
@facebook facebook locked as resolved and limited conversation to collaborators Mar 1, 2019
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Mar 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

6 participants