search

keymanapp / keyman

Keyman cross platform input methods system running on Android, iOS, Linux, macOS, Windows and mobile and desktop web
https://keyman.com/
Other
372 stars 102 forks source link

bug(android): selected-text handling crashes, continued #11360

Closed jahorton closed 1 week ago

jahorton commented 1 week ago

Sentry event: https://keyman.sentry.io/share/issue/a08bf2d9be184cc9b6de5d0ae31b5924/

StringIndexOutOfBoundsException

begin 0, end -1, length 0

java.lang.String in substring at line 2517 com.keyman.engine.KMKeyboard in updateSelectionRange at line 207

After merging in #11345, we're still getting crashing events - the link above is based on 17.0.319-beta.

java.lang.StringIndexOutOfBoundsException: begin 0, end -1, length 0
    at java.lang.String.checkBoundsBeginEnd(String.java:4466)
    at java.lang.String.substring(String.java:2517)
    at com.keyman.engine.KMKeyboard.updateSelectionRange(KMKeyboard.java:207)
    at com.keyman.engine.KMManager.updateSelectionRange(KMManager.java:2173)
    at com.keyman.android.SystemKeyboard.onStartInput(SystemKeyboard.java:173)
    at android.inputmethodservice.InputMethodService.doStartInput(InputMethodService.java:3206)
    at android.inputmethodservice.InputMethodService$InputMethodImpl.startInput(InputMethodService.java:832)
    at android.inputmethodservice.InputMethodService$InputMethodImpl.dispatchStartInput(InputMethodService.java:861)
    at android.inputmethodservice.IInputMethodWrapper.executeMessage(IInputMethodWrapper.java:197)
    at com.android.internal.os.HandlerCaller$MyHandler.handleMessage(HandlerCaller.java:44)
    at android.os.Handler.dispatchMessage(Handler.java:106)
    at android.os.Looper.loopOnce(Looper.java:257)
    at android.os.Looper.loop(Looper.java:368)
    at android.app.ActivityThread.main(ActivityThread.java:8826)
    at java.lang.reflect.Method.invoke(Method.java)
    at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:572)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1049)

Following the stack trace, we reach the following sections:

https://github.com/keymanapp/keyman/blob/0a28678c5324fbe5ea8aec5e129df48f9c8b080c/android/KMEA/app/src/main/java/com/keyman/engine/KMKeyboard.java#L188-L193

https://github.com/keymanapp/keyman/blob/0a28678c5324fbe5ea8aec5e129df48f9c8b080c/android/KMEA/app/src/main/java/com/keyman/engine/KMKeyboard.java#L206-L209

... so, selMin - either selectionStart or selectionEnd - can be -1? That doesn't sound right... and unfortunately, there's nothing about the possibility in official documentation. It doesn't sound like something that should be possible... but that's the only way I can see getting the negative value here. We may need to prod around a bit and see if we can get confirmation about this.

darcywong00 commented 1 week ago

Do you have details on how this selection crashed? Was it "reverse"? Was it really length 0?

jahorton commented 1 week ago

All I have is the data observable from the linked Sentry event and raw observations from the code as it is. I did not generate this error or know of who did.

jahorton commented 1 week ago

So far as I can tell, we're always getting this error from SystemKeyboard, never the in-app one.

jahorton commented 1 week ago

I wonder... what would happen if we selected a lot of text? ExtractedText does model a sliding context window, right? I wonder what would happen if we selected, then deleted... say, maybe a full document of text. (A moderate amount doesn't suffice, unfortunately.)

jahorton commented 1 week ago

Looking into Android documentation, I did notice this:

https://developer.android.com/reference/android/view/inputmethod/InputConnection#getExtractedText(android.view.inputmethod.ExtractedTextRequest,%20int)

Editor authors: as a general rule [...] you should be calling InputMethodManager#updateExtractedText(View, int, ExtractedText) whenever you call InputMethodManager#updateSelection(View, int, int, int, int).

So... these values can be directly provided by developers of editors... and I see no guarantee of validation being applied between editor and input method. I certainly hope I'm reading too much into that, though.

While it's not one of the parts that we reference... there is at least one semi-related field for ExtractedText which can legally have the value of -1 in some cases according to official documentation.

darcywong00 commented 1 week ago

I wonder... what would happen if we selected a lot of text?

On 17.0.139-beta, I've tried selecting the entire text (both normal and "reversed" directions), and the keyboard replaced the text fine

sentry-io[bot] commented 1 week ago

Sentry Issue: KEYMAN-ANDROID-50X

mcdurdin commented 1 week ago

either selectionStart or selectionEnd - can be -1?

https://developer.android.com/reference/android/text/Selection#getSelectionEnd(java.lang.CharSequence)

public static final int getSelectionEnd (CharSequence text) Return the offset of the selection edge or cursor, or -1 if there is no selection or cursor.

This suggests that focus has been lost or some similar scenario? In any case, if we get a -1 in either selectionStart or selectionEnd then we probably need to treat this as a no-op just like if we don't get an InputConnection.

jahorton commented 1 week ago

either selectionStart or selectionEnd - can be -1?

https://developer.android.com/reference/android/text/Selection#getSelectionEnd(java.lang.CharSequence)

public static final int getSelectionEnd (CharSequence text) Return the offset of the selection edge or cursor, or -1 if there is no selection or cursor.

This suggests that focus has been lost or some similar scenario? In any case, if we get a -1 in either selectionStart or selectionEnd then we probably need to treat this as a no-op just like if we don't get an InputConnection.

Thinking about it, there is some IPC between the Android engine and its embedded Web engine, right? So if it was active when Web got the keystroke, but the keyboard's focused element loses focus by the time Android gets the result from Web, perhaps that could do it. It could be a race condition of sorts.

We did see other issues due to timing with all the rapid-typing fixes. So, if the app is still playing catchup, I could see this scenario happen if focus changes during that time.

With this as a hypothesis, the following might work as a repro:

I can't guarantee it without personally testing, and this would be performance and timing sensitive.

Edit: okay, that was a bit creepy. What I got, using the Android "Memo" app with the system keyboard (on a 6.0.1 device), was that the lingering rapid-type output... resumed when I next activated a text field, with no prompting from myself. Similar behavior from the in-app, interrupt-via-Settings path. It's as if the WebView simply paused.

Of course... again, this could be timing sensitive - perhaps the WebView was simply "paused" at opportune moments.

Also, that "auto-resume" behavior feels really weird - it's probably worthy of consideration of a bug. Nothing 17.0-blocking, though.

darcywong00 commented 1 week ago

Fixed by #11384 and #11389