814886 - java.lang.IllegalArgumentException: invalid selection offsets at org.mozilla.gecko.GeckoEditable$Action.newSetSelection(GeckoEditable.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Scoobidiver (away)]

There's one crash in 20.0a1/20121124: bp-44ca043e-e26a-4766-866b-75b952121124.

java.lang.IllegalArgumentException: invalid selection offsets
	at org.mozilla.gecko.GeckoEditable$Action.newSetSelection(GeckoEditable.java:131)
	at org.mozilla.gecko.GeckoEditable.setSpan(GeckoEditable.java:682)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:511)
	at org.mozilla.gecko.GeckoEditable.invoke(GeckoEditable.java:630)
	at $Proxy0.setSpan(Native Method)
	at android.text.Selection.setSelection(Selection.java:78)
	at android.view.inputmethod.BaseInputConnection.setSelection(BaseInputConnection.java:487)
	at com.android.internal.view.IInputConnectionWrapper.executeMessage(IInputConnectionWrapper.java:288)
	at com.android.internal.view.IInputConnectionWrapper$MyHandler.handleMessage(IInputConnectionWrapper.java:77)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loop(Looper.java:137)
	at android.app.ActivityThread.main(ActivityThread.java:4447)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:511)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:784)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:551)
	at dalvik.system.NativeStart.main(Native Method)

More reports at:
https://crash-stats.mozilla.com/report/list?signature=java.lang.IllegalArgumentException%3A+invalid+selection+offsets+at+org.mozilla.gecko.GeckoEditable%24Action.newSetSelection%28GeckoEditable.java%29

Comment 1

[Robert Kaiser]

Still only this one instance of this signature.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

I see that it is a bug. I will have a patch soon.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Correctly check selection bounds (v1)

Selection start and end should be independent of each other (i.e. we should not have asserted that start <= end). Also, when setting selection, some parts of our code use start == -1 or end == -1 to mean that the respective offset should not be changed. However, this was never implemented in the code that does the selection setting; so this patch implements it.

Comment 4

[Chris Peterson [:cpeterson]]

Comment on attachment 685195 [details] [diff] [review]
Correctly check selection bounds (v1)

Review of attachment 685195 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM with nits.

Does this bug affect Aurora 19, too?

::: mobile/android/base/GeckoEditable.java
@@ +124,5 @@
>              action.mEnd = end;
>              return action;
>          }
>  
>          static Action newSetSelection(int start, int end) {

Can newSetSelection be made private?

@@ +127,5 @@
>  
>          static Action newSetSelection(int start, int end) {
> +            // start == -1 when the start offset should remain the same
> +            // end == -1 when the end offset should remain the same
> +            if (start < -1 || end < -1) {

I think we should still check (start > end). If start >= -1 and end >= -1, then start should be <= end.

@@ +519,5 @@
>          if (DEBUG) {
>              // GeckoEditableListener methods should all be called from the Gecko thread
>              GeckoApp.assertOnGeckoThread();
>          }
> +        if (start < 0 || start > mText.length() || end < 0 || end > mText.length()) {

I think we should still check (start > end). You can replace (start > mText.length) with (start > end) because we know end <= mText.length().

Comment 5

[Scoobidiver (away)]

(In reply to Chris Peterson (:cpeterson) from comment #4)
> Does this bug affect Aurora 19, too?
Not yet but the patch of bug 805162 landed in 19.0 so it might be in the future.

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Scoobidiver from comment #5)
> (In reply to Chris Peterson (:cpeterson) from comment #4)
> > Does this bug affect Aurora 19, too?
> Not yet but the patch of bug 805162 landed in 19.0 so it might be in the
> future.

Right, this bug is in 19 too and I'll nom for uplifting.

> (In reply to Chris Peterson (:cpeterson) from comment #4)
> 
> ::: mobile/android/base/GeckoEditable.java
> @@ +124,5 @@
> >              action.mEnd = end;
> >              return action;
> >          }
> >  
> >          static Action newSetSelection(int start, int end) {
> 
> Can newSetSelection be made private?

GeckoEditable methods call GeckoEditable.Action.newSetSelection()

> @@ +127,5 @@
> >  
> >          static Action newSetSelection(int start, int end) {
> > +            // start == -1 when the start offset should remain the same
> > +            // end == -1 when the end offset should remain the same
> > +            if (start < -1 || end < -1) {
> 
> I think we should still check (start > end). If start >= -1 and end >= -1,
> then start should be <= end.

So selection start and end are supposed to be independent, and it is possible that start > end, e.g. when you click at the end of the text and drag backwards. This is actually the cause of this bug, the -1 part is another aspect that this patch also fixes.

> @@ +519,5 @@
> >          if (DEBUG) {
> >              // GeckoEditableListener methods should all be called from the Gecko thread
> >              GeckoApp.assertOnGeckoThread();
> >          }
> > +        if (start < 0 || start > mText.length() || end < 0 || end > mText.length()) {
> 
> I think we should still check (start > end). You can replace (start >
> mText.length) with (start > end) because we know end <= mText.length().

See above.

Comment 7

[Chris Peterson [:cpeterson]]

Sounds good. LGTM!

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12ab14c79a08

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 685195 [details] [diff] [review]
Correctly check selection bounds (v1)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 805162
User impact if declined: Possible crash when using keyboards
Testing completed (on m-c, etc.): Locally
Risk to taking this patch (and alternatives if risky): None; the patch only applies to conditions where we used to crash
String or UUID changes made by this patch: None

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/12ab14c79a08

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/af9c9bfa3370