From 4e51877f5cbdb4a92568dce50c2bdc381cfbe861 Mon Sep 17 00:00:00 2001 From: Clara Bayarri Date: Tue, 27 Mar 2018 14:25:33 +0100 Subject: [PATCH] Fix crash when modifying Selection The root of this bug was in the fact that Selection.removeSelection removes two spans, the start index and end index of the selection. Each span removal triggers Editor#onSpanRemoved, which in turn tries to set a selection. This meant that if we started with selection (100, 120), then removeSpan(start) was called, so we had (-1, 120), then the onSpanRemoved code tried to set a selection so set it to (120, 120), then removeSpan(end) was called, ending up in (120, -1). There are two stages to this fix 1. A lot of our code assumes that when either start or end selection are larger than -1, both are valid. Therefore when we have one of them out of sync, we crash. Fixed this assumption in all the places I found 2. We didn't have a mechanism to use FLAG_INTERMEDIATE when removing spans, only when adding them, so this CL adds a remove with flags. This allows us to not trigger onSpanRemoved when only one of the selection indexes is removed. Because this is an added method to an interface, the default just calls the existing method. The new method is implemented in SpannableStringInternal and SpannableStringBuilder to read FLAG_INTERMEDIATE and avoid sending a spans changed event. Selection.removeSelection then uses FLAG_INTERMEDIATE when removing the first of the two selection spans. Note that 2. would be enough to fix the current bug, but we want to avoid other implementations of Spannable from crashing in the wild. In general, it seems like a good idea to verify both selection indexes are valid whenever they are used. Bug: 72101848 Test: atest FrameworksCoreTests:SpannableStringBuilderTest Test: atest FrameworksCoreTests:SpannableStringTest Test: atest CtsWidgetTestCases:TextViewTest Test: atest CtsWidgetTestCases:EditTextTest Test: atest android.text.cts.SelectionTest (note new test as well) Test: atest android.view.inputmethod.cts.BaseInputConnectionTest Test: atest android.text.DynamicLayoutTest Change-Id: I0d647fad152d0bef0f2115a46c3d17ebd8642281 --- core/java/android/text/Selection.java | 2 +- core/java/android/text/Spannable.java | 13 +++++++ .../android/text/SpannableStringBuilder.java | 19 ++++++++--- .../android/text/SpannableStringInternal.java | 13 +++++-- .../view/inputmethod/BaseInputConnection.java | 2 +- core/java/android/widget/Editor.java | 4 ++- core/java/android/widget/TextView.java | 2 +- .../src/android/text/SpannableTest.java | 34 +++++++++++++++++++ 8 files changed, 79 insertions(+), 10 deletions(-) diff --git a/core/java/android/text/Selection.java b/core/java/android/text/Selection.java index 34456580ee619..5256e471e974d 100644 --- a/core/java/android/text/Selection.java +++ b/core/java/android/text/Selection.java @@ -180,7 +180,7 @@ public class Selection { * Remove the selection or cursor, if any, from the text. */ public static final void removeSelection(Spannable text) { - text.removeSpan(SELECTION_START); + text.removeSpan(SELECTION_START, Spanned.SPAN_INTERMEDIATE); text.removeSpan(SELECTION_END); removeMemory(text); } diff --git a/core/java/android/text/Spannable.java b/core/java/android/text/Spannable.java index 39b78eb056d39..8315b2aa52c65 100644 --- a/core/java/android/text/Spannable.java +++ b/core/java/android/text/Spannable.java @@ -45,6 +45,19 @@ extends Spanned */ public void removeSpan(Object what); + /** + * Remove the specified object from the range of text to which it + * was attached, if any. It is OK to remove an object that was never + * attached in the first place. + * + * See {@link Spanned} for an explanation of what the flags mean. + * + * @hide + */ + default void removeSpan(Object what, int flags) { + removeSpan(what); + } + /** * Factory used by TextView to create new {@link Spannable Spannables}. You can subclass * it to provide something other than {@link SpannableString}. diff --git a/core/java/android/text/SpannableStringBuilder.java b/core/java/android/text/SpannableStringBuilder.java index d41dfdcd29fc9..41a9c45aed57d 100644 --- a/core/java/android/text/SpannableStringBuilder.java +++ b/core/java/android/text/SpannableStringBuilder.java @@ -312,7 +312,7 @@ public class SpannableStringBuilder implements CharSequence, GetChars, Spannable // The following condition indicates that the span would become empty (textIsRemoved || mSpanStarts[i] > start || mSpanEnds[i] < mGapStart)) { mIndexOfSpan.remove(mSpans[i]); - removeSpan(i); + removeSpan(i, 0 /* flags */); return true; } return resolveGap(mSpanStarts[i]) <= end && (i & 1) != 0 && @@ -472,7 +472,7 @@ public class SpannableStringBuilder implements CharSequence, GetChars, Spannable } // Note: caller is responsible for removing the mIndexOfSpan entry. - private void removeSpan(int i) { + private void removeSpan(int i, int flags) { Object object = mSpans[i]; int start = mSpanStarts[i]; @@ -496,7 +496,9 @@ public class SpannableStringBuilder implements CharSequence, GetChars, Spannable // Invariants must be restored before sending span removed notifications. restoreInvariants(); - sendSpanRemoved(object, start, end); + if ((flags & Spanned.SPAN_INTERMEDIATE) == 0) { + sendSpanRemoved(object, start, end); + } } // Documentation from interface @@ -782,10 +784,19 @@ public class SpannableStringBuilder implements CharSequence, GetChars, Spannable * Remove the specified markup object from the buffer. */ public void removeSpan(Object what) { + removeSpan(what, 0 /* flags */); + } + + /** + * Remove the specified markup object from the buffer. + * + * @hide + */ + public void removeSpan(Object what, int flags) { if (mIndexOfSpan == null) return; Integer i = mIndexOfSpan.remove(what); if (i != null) { - removeSpan(i.intValue()); + removeSpan(i.intValue(), flags); } } diff --git a/core/java/android/text/SpannableStringInternal.java b/core/java/android/text/SpannableStringInternal.java index 5dd1a52b4a7a0..bcc2fda86074b 100644 --- a/core/java/android/text/SpannableStringInternal.java +++ b/core/java/android/text/SpannableStringInternal.java @@ -249,6 +249,13 @@ import java.lang.reflect.Array; } /* package */ void removeSpan(Object what) { + removeSpan(what, 0 /* flags */); + } + + /** + * @hide + */ + public void removeSpan(Object what, int flags) { int count = mSpanCount; Object[] spans = mSpans; int[] data = mSpanData; @@ -262,11 +269,13 @@ import java.lang.reflect.Array; System.arraycopy(spans, i + 1, spans, i, c); System.arraycopy(data, (i + 1) * COLUMNS, - data, i * COLUMNS, c * COLUMNS); + data, i * COLUMNS, c * COLUMNS); mSpanCount--; - sendSpanRemoved(what, ostart, oend); + if ((flags & Spanned.SPAN_INTERMEDIATE) == 0) { + sendSpanRemoved(what, ostart, oend); + } return; } } diff --git a/core/java/android/view/inputmethod/BaseInputConnection.java b/core/java/android/view/inputmethod/BaseInputConnection.java index 5f7a0f789fa48..090e19f91e38c 100644 --- a/core/java/android/view/inputmethod/BaseInputConnection.java +++ b/core/java/android/view/inputmethod/BaseInputConnection.java @@ -522,7 +522,7 @@ public class BaseInputConnection implements InputConnection { b = tmp; } - if (a == b) return null; + if (a == b || a < 0) return null; if ((flags&GET_TEXT_WITH_STYLES) != 0) { return content.subSequence(a, b); diff --git a/core/java/android/widget/Editor.java b/core/java/android/widget/Editor.java index 99467265b5c58..6af678b1053d1 100644 --- a/core/java/android/widget/Editor.java +++ b/core/java/android/widget/Editor.java @@ -6031,7 +6031,9 @@ public class Editor { mSwitchedLines = false; final int selectionStart = mTextView.getSelectionStart(); final int selectionEnd = mTextView.getSelectionEnd(); - if (selectionStart > selectionEnd) { + if (selectionStart < 0 || selectionEnd < 0) { + Selection.removeSelection((Spannable) mTextView.getText()); + } else if (selectionStart > selectionEnd) { Selection.setSelection((Spannable) mTextView.getText(), selectionEnd, selectionStart); } diff --git a/core/java/android/widget/TextView.java b/core/java/android/widget/TextView.java index 11db6b650583b..fae6db5d694fa 100644 --- a/core/java/android/widget/TextView.java +++ b/core/java/android/widget/TextView.java @@ -9380,7 +9380,7 @@ public class TextView extends View implements ViewTreeObserver.OnPreDrawListener final int selectionStart = getSelectionStart(); final int selectionEnd = getSelectionEnd(); - return selectionStart >= 0 && selectionStart != selectionEnd; + return selectionStart >= 0 && selectionEnd > 0 && selectionStart != selectionEnd; } String getSelectedText() { diff --git a/core/tests/coretests/src/android/text/SpannableTest.java b/core/tests/coretests/src/android/text/SpannableTest.java index 307ac62f9d07b..f3684286dbbb7 100644 --- a/core/tests/coretests/src/android/text/SpannableTest.java +++ b/core/tests/coretests/src/android/text/SpannableTest.java @@ -16,6 +16,8 @@ package android.text; +import static org.junit.Assert.assertEquals; + import android.platform.test.annotations.Presubmit; import android.support.test.filters.SmallTest; import android.support.test.runner.AndroidJUnit4; @@ -24,6 +26,8 @@ import android.test.MoreAsserts; import org.junit.Test; import org.junit.runner.RunWith; +import java.util.concurrent.CountDownLatch; + @Presubmit @SmallTest @RunWith(AndroidJUnit4.class) @@ -53,4 +57,34 @@ public abstract class SpannableTest { spans = spannable.getSpans(2, 2, Object.class); MoreAsserts.assertEquals(new Object[]{unemptySpan}, spans); } + + @Test + public void testRemoveSpanWithIntermediateFlag() { + Spannable spannable = newSpannableWithText("abcdef"); + Object emptySpan = new Object(); + spannable.setSpan(emptySpan, 1, 1, 0); + Object unemptySpan = new Object(); + spannable.setSpan(unemptySpan, 1, 2, 0); + + final CountDownLatch latch = new CountDownLatch(1); + SpanWatcher watcher = new SpanWatcher() { + @Override + public void onSpanAdded(Spannable text, Object what, int start, int end) {} + + @Override + public void onSpanRemoved(Spannable text, Object what, int start, int end) { + latch.countDown(); + } + + @Override + public void onSpanChanged(Spannable text, Object what, int ostart, int oend, int nstart, + int nend) {} + }; + spannable.setSpan(watcher, 0, 2, 0); + + spannable.removeSpan(emptySpan, Spanned.SPAN_INTERMEDIATE); + assertEquals(1, latch.getCount()); + spannable.removeSpan(unemptySpan); + assertEquals(0, latch.getCount()); + } }