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

RESOLVED FIXED in Firefox 19

Status

()

defect
--
critical
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: scoobidiver, Assigned: jchen)

Tracking

({crash, regression})

19 Branch
Firefox 20
ARM
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox17 unaffected, firefox18 unaffected, firefox19 fixed, firefox20 fixed)

Details

(Whiteboard: [native-crash], crash signature)

Attachments

(1 attachment)

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
Still only this one instance of this signature.
I see that it is a bug. I will have a patch soon.
Assignee: nobody → nchen
Status: NEW → ASSIGNED
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.
Attachment #685195 - Flags: review?(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().
Attachment #685195 - Flags: review?(cpeterson) → review+
(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.
Blocks: 805162
Keywords: regression
Version: Trunk → Firefox 19
(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.
Sounds good. LGTM!
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
Attachment #685195 - Flags: approval-mozilla-aurora?
Attachment #685195 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/12ab14c79a08
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.