857413 - ANR: deadlock in IME code when using UI-thread-safe Editable

Status: VERIFIED FIXED

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

Description

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

The ANR reporter caught a deadlock when switching the IME thread from UI thread to background thread:

>"main" prio=5 tid=1 WAIT
>  | group="main" sCount=1 dsCount=0 obj=0x41254508 self=0x41244138
>  | sysTid=11678 nice=0 sched=0/0 cgrp=apps handle=1074925360
>  | schedstat=( 7018432624 7428619357 15925 ) utm=550 stm=151 core=0
>  at java.lang.Object.wait(Native Method)
>  - waiting on <0x412545d8> (a java.lang.VMThread) held by tid=1 (main)
>  at java.lang.Thread.parkFor(Thread.java:1231)
>  at sun.misc.Unsafe.park(Unsafe.java:323)
>  at java.util.concurrent.locks.LockSupport.park(LockSupport.java:157)
>  at java.util.concurrent.SynchronousQueue$TransferStack.awaitFulfill(SynchronousQueue.java:419)
>  at java.util.concurrent.SynchronousQueue$TransferStack.transfer(SynchronousQueue.java:320)
>  at java.util.concurrent.SynchronousQueue.put(SynchronousQueue.java:811)
>  at org.mozilla.gecko.GeckoInputConnection$InputThreadUtils.runOnIcThread(GeckoInputConnection.java:86)
>  at org.mozilla.gecko.GeckoInputConnection$InputThreadUtils.access$400(GeckoInputConnection.java:49)
>  at org.mozilla.gecko.GeckoInputConnection$InputThreadUtils$3.invoke(GeckoInputConnection.java:149)
>  at $Proxy0.getSpanFlags(Native Method)
>  at android.text.method.MetaKeyKeyListener.getActive(MetaKeyKeyListener.java:197)
>  at android.text.method.MetaKeyKeyListener.getMetaState(MetaKeyKeyListener.java:157)
>  at org.mozilla.gecko.GeckoInputConnection.processKey(GeckoInputConnection.java:736)
>  at org.mozilla.gecko.GeckoInputConnection.onKeyDown(GeckoInputConnection.java:753)
>  at org.mozilla.gecko.gfx.LayerView.onKeyDown(LayerView.java:266)
>  at android.view.KeyEvent.dispatch(KeyEvent.java:2705)
>  at android.view.View.dispatchKeyEvent(View.java:7224)
>  at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1359)
>  at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1363)
>  at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1363)
>  at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1363)
>  at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1363)
>  at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1363)
>  at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1363)
>  at com.android.internal.policy.impl.PhoneWindow$DecorView.superDispatchKeyEvent(PhoneWindow.java:2080)
>  at com.android.internal.policy.impl.PhoneWindow.superDispatchKeyEvent(PhoneWindow.java:1456)
>  at android.app.Activity.dispatchKeyEvent(Activity.java:2407)
>  at com.android.internal.policy.impl.PhoneWindow$DecorView.dispatchKeyEvent(PhoneWindow.java:2007)
>  at android.view.ViewRootImpl.deliverKeyEventPostIme(ViewRootImpl.java:3793)
>  at android.view.ViewRootImpl.deliverKeyEvent(ViewRootImpl.java:3727)
>  at android.view.ViewRootImpl.deliverInputEvent(ViewRootImpl.java:3296)
>  at android.view.ViewRootImpl.doProcessInputEvents(ViewRootImpl.java:4392)
>  at android.view.ViewRootImpl$ViewRootHandler.handleMessage(ViewRootImpl.java:2922)
>  at android.os.Handler.dispatchMessage(Handler.java:99)
>  at android.os.Looper.loop(Looper.java:137)
>  at android.app.ActivityThread.main(ActivityThread.java:4895)
>  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:994)
>  at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:761)
>  at dalvik.system.NativeStart.main(Native Method)

>"GeckoInputConnection" daemon prio=5 tid=16 MONITOR
>  | group="main" sCount=1 dsCount=0 obj=0x41d82558 self=0x5e21d478
>  | sysTid=12595 nice=0 sched=0/0 cgrp=apps handle=1625306488
>  | schedstat=( 518799 31982422 9 ) utm=0 stm=0 core=0
>  at java.lang.Object.wait(Native Method)
>  at java.lang.Object.wait(Object.java:364)
>  at org.mozilla.gecko.GeckoEditable$4.run(GeckoEditable.java:651)
>  at android.os.Handler.handleCallback(Handler.java:615)
>  at android.os.Handler.dispatchMessage(Handler.java:92)
>  at android.os.Looper.loop(Looper.java:137)
>  at org.mozilla.gecko.GeckoInputConnection$1.run(GeckoInputConnection.java:476)
>  at java.lang.Thread.run(Thread.java:856)

Comment 1

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

Attachment: Use only one copy of GeckoInputConnection.InputThreadUtils (v1)

I can't think of why this would occur. My speculation is that there's a race condition when we have multiple copies of InputThreadUtils. This patch makes InputThreadUtils static and hopefully will fix the issue.

Comment 2

[Chris Peterson [:cpeterson]]

Comment on attachment 735235 [details] [diff] [review]
Use only one copy of GeckoInputConnection.InputThreadUtils (v1)

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

LGTM.

Does this deadlock affect Aurora 22 and Beta 21? I think we should consider uplifting it because this is a bad bug and the fix is pretty safe.

::: mobile/android/base/GeckoInputConnection.java
@@ +46,5 @@
>  
>      private static Handler sBackgroundHandler;
>  
> +    private static class InputThreadUtils {
> +        public static final InputThreadUtils INSTANCE = new InputThreadUtils();

1. Let's add a short comment explaining why we need a static instance for the UI and IC threads.

2. Since `INSTANCE` is not a constant, I think we should rename it to something like `sInstance` or a `getInstance()` method.

@@ +180,5 @@
>              return mUiEditable;
>          }
>      }
>  
> +    private final InputThreadUtils mThreadUtils = InputThreadUtils.INSTANCE;

Why keep mThreadUtils? Why not just replace uses of mThreadUtils with direct calls on InputThreadUtils.INSTANCE?

Comment 3

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

Attachment: Use only one copy of GeckoInputConnection.InputThreadUtils (v1.1)

Added comment, changed to sInstance, and got rid of mThreadUtils.

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/78bdaf0577cd

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/78bdaf0577cd

Comment 6

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

The latest ANR data no longer show this deadlock; marking as verified fixed

Comment 7

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

Comment on attachment 736295 [details] [diff] [review]
Use only one copy of GeckoInputConnection.InputThreadUtils (v1.1)

[Approval Request Comment]

Bug caused by (feature/regressing bug #): Bug 839882; confirmed affected on 22 through ANR data, but because the bug was landed on 21, 21 should affected as well (ANR data is not available for Beta to confirm this)

User impact if declined: Deadlock/ANR when using keyboard in Fennec

Testing completed (on m-c, etc.): m-c

Risk to taking this patch (and alternatives if risky): Small; the patch avoids the deadlock but does not impact functionality

String or IDL/UUID changes made by this patch: None

Comment 8

[bhavana bajaj [:bajaj]]

Comment on attachment 736295 [details] [diff] [review]
Use only one copy of GeckoInputConnection.InputThreadUtils (v1.1)

I am approving this on aurora/beta as this is a fallout from Bug 839882 which landed in Fx 21 to resolve a top-crasher.

Spoke to :jchen and based on aurora/nightly this is the most common deadlocks when using fennec .The patch is low risk enough for our beta 4.Since ANR is only on nightly please keep on any new regressions this may have.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/71e7584352b0

Needs some love for beta, though.

Comment 10

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

Pushed to Beta after rebasing and building locally,

https://hg.mozilla.org/releases/mozilla-beta/rev/95bedf59af4a