1058127 - Fix GeckoEditable.removeSpan race condition

Status: RESOLVED FIXED

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

Description

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

Right now removeSpan is handled on the InputConnection thread whereas other calls are handled on the Gecko thread. This can result in race conditions.

Comment 1

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

Attachment: Properly implement removeSpan synchronization (v1)

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.

Comment 2

[Chris Peterson [: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?

Comment 3

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

(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.

Comment 4

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

https://hg.mozilla.org/integration/fx-team/rev/5efbfff98372

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5efbfff98372