Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 16

Status

()

Firefox for Android
Keyboards and IME
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 16
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14 affected, firefox15 wontfix, firefox16 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
After reviewing IME crash stats, I found some more GeckoInputConnection methods that may be called with out-of-bounds indexes from IMEs.
(Assignee)

Comment 1

5 years ago
Created attachment 634107 [details] [diff] [review]
part-1-extract-Span-class.patch

Part 1: Extract selection index juggling into a Span helper class.
Attachment #634107 - Flags: review?(blassey.bugs)
(Assignee)

Comment 2

5 years ago
Created attachment 634108 [details] [diff] [review]
part-2-clamp-setSelection-parameters.patch

Part 2: Clamp bogus selection indexes passed to setSelection().
Attachment #634108 - Flags: review?(blassey.bugs)
(Assignee)

Comment 3

5 years ago
Created attachment 634109 [details] [diff] [review]
part-3-clamp-setComposingRegion.patch

Part 3: Clamp bogus composing region indexes passed to setComposingRegion().
Attachment #634109 - Flags: review?(blassey.bugs)
(Assignee)

Comment 4

5 years ago
Created attachment 634110 [details] [diff] [review]
part-4-clamp-getTextBeforeAfterCursor.patch

Part 4: Clamp string lengths passed to getTextBeforeCursor/getTextAfterCursor.
Attachment #634110 - Flags: review?(blassey.bugs)
(Assignee)

Comment 5

5 years ago
Created attachment 634111 [details] [diff] [review]
part-5-clamp-composing-span.patch

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+
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
Created attachment 634537 [details] [diff] [review]
part-3-clamp-setComposingRegion-v2.patch

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+
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/97bc7b4de4ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba96999567ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7def2566200
https://hg.mozilla.org/integration/mozilla-inbound/rev/80528404a24e
https://hg.mozilla.org/integration/mozilla-inbound/rev/62d5b893232b
status-firefox16: affected → fixed
Target Milestone: --- → Firefox 16
https://hg.mozilla.org/mozilla-central/rev/97bc7b4de4ff
https://hg.mozilla.org/mozilla-central/rev/ba96999567ad
https://hg.mozilla.org/mozilla-central/rev/c7def2566200
https://hg.mozilla.org/mozilla-central/rev/80528404a24e
https://hg.mozilla.org/mozilla-central/rev/62d5b893232b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 733138

Comment 13

5 years ago
Crashes that depends on this bug are #19 top crasher in 14.0.
tracking-fennec: --- → ?
Is this on Beta?
(Assignee)

Comment 15

5 years ago
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.
status-firefox15: affected → wontfix
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.