Closed Bug 1058127 Opened 5 years ago Closed 5 years ago

Fix GeckoEditable.removeSpan race condition

Categories

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

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 34

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(1 file)

Right now removeSpan is handled on the InputConnection thread whereas other calls are handled on the Gecko thread. This can result in race conditions.
This patch changes removeSpan processing from the InputConnection thread to the Gecko thread. Chris, I'm not sure who else can review this, but let me know if you want someone else to look at it.
Attachment #8478465 - Flags: review?(cpeterson)
Comment on attachment 8478465 [details] [diff] [review]
Properly implement removeSpan synchronization (v1)

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

LGTM!

::: mobile/android/base/GeckoEditable.java
@@ -1045,5 @@
>          if (what == Selection.SELECTION_START ||
>                  what == Selection.SELECTION_END) {
>              Log.w(LOGTAG, "selection removed with removeSpan()");
>          }
> -        if (mText.getSpanStart(what) >= 0) { // only remove if it's there

Was this check a (racy) optimization? Do you want this check when processing TYPE_REMOVE_SPAN on the Gecko thread?
Attachment #8478465 - Flags: review?(cpeterson) → review+
(In reply to Chris Peterson (:cpeterson) from comment #2)
> Comment on attachment 8478465 [details] [diff] [review]
> Properly implement removeSpan synchronization (v1)
> 
> Review of attachment 8478465 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM!
> 
> ::: mobile/android/base/GeckoEditable.java
> @@ -1045,5 @@
> >          if (what == Selection.SELECTION_START ||
> >                  what == Selection.SELECTION_END) {
> >              Log.w(LOGTAG, "selection removed with removeSpan()");
> >          }
> > -        if (mText.getSpanStart(what) >= 0) { // only remove if it's there
> 
> Was this check a (racy) optimization? Do you want this check when processing
> TYPE_REMOVE_SPAN on the Gecko thread?

Yep it was a racy optimization. The same check is contained in the removeSpan call on the Gecko thread.
https://hg.mozilla.org/mozilla-central/rev/5efbfff98372
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
You need to log in before you can comment on or make changes to this bug.