bug(android/engine): StringIndexOutOfBoundsException when end > length

[darcywong00]

Relates to #11128 which fixed StringIndexOutOfBoundsException when the selection was reversed

@mcdurdin triggered this similar crash while repeatedly holding down backspace. It's the same area of code, but where the substring end is > length

StringIndexOutOfBoundsException
begin 0, end 12, length 11

java.lang.String in checkBoundsBeginEnd at line 4466
java.lang.String in substring at line 2517
com.keyman.engine.KMKeyboard in updateSelectionRange at line 190
com.keyman.engine.KMManager in updateSelectionRange at line 2150
com.keyman.android.SystemKeyboard in onUpdateSelection at line 132
android.inputmethodservice.InputMethodService$InputMethodSessionImpl in updateSelection at line 1414
android.inputmethodservice.IInputMethodSessionWrapper in executeMessage at line 111
com.android.internal.os.HandlerCaller$MyHandler in handleMessage at line 44
android.os.Handler in dispatchMessage at line 106
android.os.Looper in loopOnce at line 230
android.os.Looper in loop at line 319
android.app.ActivityThread in main at line 8893
java.lang.reflect.Method in invoke
com.android.internal.os.RuntimeInit$MethodAndArgsCaller in run at line 608
com.android.internal.os.ZygoteInit in main at line 1103

I think we should catch this, but also add a limit
end = (end > length) ? length : end;

And if possible add some unit test

[jahorton]

Investigation of the issue yields something potentially enlightening: this first started showing up with 17.0.280-beta. The thing most likely to have affected this: #10885, which landed then.

Someone (could be me, could be @darcywong00) would need to dig into where the underlying assumptions are invalid in order to get a solid fix, I think.

[darcywong00]

Happy for you to spelunk on this one. afict

The consistent crash is the substring() in l.190 where selStart > rawText.length()

keyman/android/KMEA/app/src/main/java/com/keyman/engine/KMKeyboard.java

Lines 166 to 192 in 93dc41e

	protected boolean updateSelectionRange(int selStart, int selEnd) {
	boolean result = false;
	InputConnection ic = KMManager.getInputConnection(this.keyboardType);
	if (ic != null) {
	ExtractedText icText = ic.getExtractedText(new ExtractedTextRequest(), 0);
	if (icText == null) {
	return false;
	}
	String rawText = icText.text.toString();
	updateText(rawText.toString());
	/*
	The values of selStart & selEnd provided by the system are in code units,
	not code-points. We need to account for surrogate pairs here.
	Fortunately, it uses UCS-2 encoding... just like JS.
	References:
	- https://stackoverflow.com/a/23980211
	- https://android.googlesource.com/platform/frameworks/base/+/152944f/core/java/android/view/inputmethod/InputConnection.java#326
	*/
	// Count the number of characters which are surrogate pairs.
	int pairsAtStart = CharSequenceUtil.countSurrogatePairs(rawText.substring(0, selStart), rawText.length());
	String selectedText = rawText.substring(selStart, selEnd);
	int pairsSelected = CharSequenceUtil.countSurrogatePairs(selectedText, selectedText.length());

Is selStart coming from KeymanWeb? The behavior is that the Android app (rawText) is deleting faster than KeymanWeb is updated.

[darcywong00]

Handing off to @jahorton for spelunking

[jahorton]

Well, I found the invalid assumption. We have ourselves a concurrency issue. It is possible for the object returned by .getExtractedText to have a more recent update to selection range than the call that triggered our check against .getExtractedText. Fortunately, the returned ExtractedText object itself provides .selectionStart and .selectionEnd fields, and these will be current. (These are also the proof of asynchronicity as seen in the following screenshots.)

KMKeyboard.updateSelectionRange and its parameters during a rapid backspacing sequence, synchronous with our handler for onUpdateSelection:

Same call, but checking the values available via the ExtractedText object:

As the InputMethodService interface with the editor and our extension is asynchronously managed, updates can happen during handling of raised events. The call still serves well as a signal that "something has updated", but we need an atomic operation to retrieve all relevant values simultaneously in order to avoid an issue here - and getExtractedText looks to be sufficient in this regard.

[darcywong00]

Fixed by #11188