Closed Bug 765831 Opened 9 years ago Closed 9 years ago

Clamp more IME selection and composing region indexes that may overrun our editable content

Categories

(Firefox for Android Graveyard :: Keyboards and IME, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox14 affected, firefox15 wontfix, firefox16 fixed)

RESOLVED FIXED
Firefox 16
Tracking Status
firefox14 --- affected
firefox15 --- wontfix
firefox16 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

Attachments

(5 files, 1 obsolete file)

After reviewing IME crash stats, I found some more GeckoInputConnection methods that may be called with out-of-bounds indexes from IMEs.
Part 1: Extract selection index juggling into a Span helper class.
Attachment #634107 - Flags: review?(blassey.bugs)
Part 2: Clamp bogus selection indexes passed to setSelection().
Attachment #634108 - Flags: review?(blassey.bugs)
Part 3: Clamp bogus composing region indexes passed to setComposingRegion().
Attachment #634109 - Flags: review?(blassey.bugs)
Part 4: Clamp string lengths passed to getTextBeforeCursor/getTextAfterCursor.
Attachment #634110 - Flags: review?(blassey.bugs)
Part 5: Clamp composing span indexes.
Attachment #634111 - Flags: review?(blassey.bugs)
Comment on attachment 634107 [details] [diff] [review]
part-1-extract-Span-class.patch

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

::: mobile/android/base/GeckoInputConnection.java
@@ +199,5 @@
>                  // If there is no selection set, we must be doing "Copy All",
>                  // otherwise get the selection
> +                if (selection.length > 0)
> +                    text = text.substring(selection.start, selection.end);
> +                GeckoAppShell.setClipboardText(text);

I don't think this is right. Before if a were equal to b we would send an empty string. Now we'll send whatever is in text.
Attachment #634107 - Flags: review?(blassey.bugs) → review+
Attachment #634108 - Flags: review?(blassey.bugs) → review+
Comment on attachment 634109 [details] [diff] [review]
part-3-clamp-setComposingRegion.patch

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

::: mobile/android/base/GeckoInputConnection.java
@@ +392,5 @@
>              endComposition();
>          }
>  
> +        Span newComposingRegion = new Span(getEditable(), start, end);
> +        return super.setComposingRegion(newComposingRegion.start, newComposingRegion.end);

what does this accomplish? Needs a comment at least.
Attachment #634109 - Flags: review?(blassey.bugs) → review-
Attachment #634110 - Flags: review?(blassey.bugs) → review+
Attachment #634111 - Flags: review?(blassey.bugs) → review+
(In reply to Brad Lassey [:blassey] from comment #6)
> ::: mobile/android/base/GeckoInputConnection.java
> @@ +199,5 @@
> >                  // If there is no selection set, we must be doing "Copy All",
> >                  // otherwise get the selection
> > +                if (selection.length > 0)
> > +                    text = text.substring(selection.start, selection.end);
> > +                GeckoAppShell.setClipboardText(text);
> 
> I don't think this is right. Before if a were equal to b we would send an
> empty string. Now we'll send whatever is in text.

The comment says no selection implies "Copy All", but that differs from the existing code. So I will correct the comment.
(In reply to Brad Lassey [:blassey] from comment #7)
> > +        Span newComposingRegion = new Span(getEditable(), start, end);
> > +        return super.setComposingRegion(newComposingRegion.start, newComposingRegion.end);
> 
> what does this accomplish? Needs a comment at least.

The Span ctor will clamp the start/end parameters, but that behavior is not obvious from the calling code. I will change the Span class to use a "named constructor" called Span.clamp() that will make the caller's intent clear.
Part 3 v2: replace setComposingRegion()'s call to `new Span()` with `Span.clamp()` named constructor.
Attachment #634109 - Attachment is obsolete: true
Attachment #634537 - Flags: review?(blassey.bugs)
Attachment #634537 - Flags: review?(blassey.bugs) → review+
Blocks: 733138
Crashes that depends on this bug are #19 top crasher in 14.0.
tracking-fennec: --- → ?
Is this on Beta?
Aaron, these fixes are Aurora 16, but not Beta 15. At the time, I felt they were too risky to uplift. 

In Firefox 14.0.1's 

In recent crash stats, these IME crashes are ~186th place for Firefox 14.0.1 and ~49 for Beta 15, so I don't think they are a high priority for uplifting.
Ok, sounds like wont-fix for 15.
tracking-fennec: ? → ---
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.