839882 - java.lang.NullPointerException: at android.text.Selection.setSelection(Selection.java)

Status: RESOLVED FIXED

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

Description

[Scoobidiver (away)]

There are several crashes from the same user in 21.0a1/20130210, including bp-f308ace0-e12a-4dbd-85e7-5be272130210.

java.lang.NullPointerException
	at android.text.Selection.setSelection(Selection.java:79)
	at android.text.method.QwertyKeyListener.onKeyDown(QwertyKeyListener.java:102)
	at android.text.method.TextKeyListener.onKeyDown(TextKeyListener.java:136)
	at org.mozilla.gecko.GeckoInputConnection.processKeyDown(GeckoInputConnection.java:550)
	at org.mozilla.gecko.GeckoInputConnection.onKeyDown(GeckoInputConnection.java:523)
	at org.mozilla.gecko.gfx.LayerView.onKeyDown(LayerView.java:262)
	at android.view.KeyEvent.dispatch(KeyEvent.java:2705)
	at android.view.View.dispatchKeyEvent(View.java:7234)
	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1409)
	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1413)
	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1413)
	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1413)
	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1413)
	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1413)
	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1413)
	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1413)
	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1413)
	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1413)
	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1413)
	at com.android.internal.policy.impl.PhoneWindow$DecorView.superDispatchKeyEvent(PhoneWindow.java:2118)
	at com.android.internal.policy.impl.PhoneWindow.superDispatchKeyEvent(PhoneWindow.java:1470)
	at android.app.Activity.dispatchKeyEvent(Activity.java:2426)
	at com.android.internal.policy.impl.PhoneWindow$DecorView.dispatchKeyEvent(PhoneWindow.java:2045)
	at android.view.ViewRootImpl.deliverKeyEventPostIme(ViewRootImpl.java:3929)
	at android.view.ViewRootImpl.handleImeFinishedEvent(ViewRootImpl.java:3877)
	at android.view.ViewRootImpl$ViewRootHandler.handleMessage(ViewRootImpl.java:3006)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loop(Looper.java:137)
	at android.app.ActivityThread.main(ActivityThread.java:4921)
	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:1038)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:805)
	at dalvik.system.NativeStart.main(Native Method)

More reports at:
https://crash-stats.mozilla.com/report/list?signature=java.lang.NullPointerException%3A+at+android.text.Selection.setSelection%28Selection.java%29

Comment 1

[Scoobidiver (away)]

It has been hit by three users now so it's a regression. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=876652a0c8ef&tochange=29dd80c95b7d
It's likely a regression from bug 835906.

Comment 3

[Scoobidiver (away)]

Bug 840403 has STR.

Comment 4

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

Attachment: Provide UI-thread-safe Editable for KeyListener (v1)

We switched all of our GeckoEditable operations to the IC thread, but turns out there is one point in our code where we have to use GeckoEditable from the UI thread -- KeyListener during key events. This patch provides a proxy Editable that is safe to run on the UI thread. It forwards all calls to the IC thread, waits for return values, and provides proper synchronization throughout so there is no posibility of deadlocks. With this patch, I can no longer reproduce this bug.

Comment 5

[Chris Peterson [:cpeterson]]

Comment on attachment 713048 [details] [diff] [review]
Provide UI-thread-safe Editable for KeyListener (v1)

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

Some comments. Also, you should investigate whether SynchronousQueue could simplify the inter-thread waiting and notifying. A SynchronousQueue allows two threads to rendezvous and hand off an object from one thread to the other.

https://developer.android.com/reference/java/util/concurrent/SynchronousQueue.html

::: mobile/android/base/GeckoInputConnection.java
@@ +46,5 @@
>  
> +    private class ThreadUtils {
> +        private Editable mUiEditable;
> +        private Object mUiEditableReturn;
> +        private Throwable mUiEditableException;

mUiEditableException should be an Exception, not a Throwable. (See my note below). You even call this Throwable a "mUiEditable*EXCEPTION*". :)

@@ +55,5 @@
> +            GeckoApp.assertOnThread(
> +                mEditableClient.getInputConnectionHandler().getLooper().getThread());
> +        }
> +
> +        public synchronized void runOnIcThread(Runnable runnable) {

Does runOnIcThread() need to be public? It is only called by ThreadUtils.getEditableForUiThread().

@@ +73,5 @@
> +                mEditableClient.getInputConnectionHandler().post(runnable);
> +            }
> +        }
> +
> +        public void waitIcThread() {

I think we should rename `waitIcThread()` to `waitForUiThread()` and `waitUiThread()` to `waitForIcThread()`. Those names indicate what the methods do, rather than who is calling them.

@@ +85,5 @@
> +                    mIcThreadRunnable = null;
> +                }
> +                try {
> +                    mIcThreadWaiting = true;
> +                    wait(); // wait for UI thread to notify us

We might want to consider calling wait() method that takes a timeout. If we time out, we can log a warning that the UI thread is taking too long, throw an exception, or just return prematurely so we don't block the IC thread forever.

@@ +103,5 @@
> +                throw new IllegalThreadStateException("IC thread is not waiting");
> +            }
> +            try {
> +                notify(); // IC thread is waiting; notify it
> +                wait(); // wait for IC thread to notify us back

We might want to consider calling wait() method that takes a timeout. If we time out, we can log a warning that the UI thread is taking too long, throw an exception, or just return prematurely so we don't block the IC thread forever.

@@ +113,5 @@
> +            if (DEBUG) {
> +                GeckoApp.assertOnUiThread();
> +            }
> +            try {
> +                wait();

We might want to consider calling wait() method that takes a timeout. If we time out, we can log a warning that the IC thread is taking too long, throw an exception, or just return prematurely so we don't block the UI thread forever.

@@ +159,5 @@
> +                                try {
> +                                    mUiEditableReturn = method.invoke(
> +                                        mEditableClient.getEditable(), args);
> +                                } catch (Throwable e) {
> +                                    mUiEditableException = e;

We should only catch Exceptions, not Throwables. Throwables include Errors, which are significant problems.

@@ +720,4 @@
>          if (mIMEState == IME_STATE_DISABLED ||
> +                mIMEState == IME_STATE_PLUGIN ||
> +                keyCode == KeyEvent.KEYCODE_ENTER ||
> +                keyCode == KeyEvent.KEYCODE_DEL ||

Please fix the indentation like before.

Comment 6

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

Attachment: Provide UI-thread-safe Editable for KeyListener (v2)

(In reply to Chris Peterson (:cpeterson) from comment #5)
> Comment on attachment 713048 [details] [diff] [review]
> Provide UI-thread-safe Editable for KeyListener (v1)
> 
> Some comments. Also, you should investigate whether SynchronousQueue could
> simplify the inter-thread waiting and notifying. A SynchronousQueue allows
> two threads to rendezvous and hand off an object from one thread to the
> other.
>
> https://developer.android.com/reference/java/util/concurrent/
> SynchronousQueue.html

Switched to using SynchronousQueue for one of the two cases where we did wait/notify. For the other case, returning Editable return value from IC thread to UI thread, I think it's simpler to just use wait/notify (one of the reasons is SynchronousQueue can't handle null values).

> ::: mobile/android/base/GeckoInputConnection.java
> @@ +46,5 @@
> >  
> > +    private class ThreadUtils {
> > +        private Editable mUiEditable;
> > +        private Object mUiEditableReturn;
> > +        private Throwable mUiEditableException;
> 
> mUiEditableException should be an Exception, not a Throwable. (See my note
> below). You even call this Throwable a "mUiEditable*EXCEPTION*". :)

Changed. Thanks for the explanation!

> @@ +55,5 @@
> > +            GeckoApp.assertOnThread(
> > +                mEditableClient.getInputConnectionHandler().getLooper().getThread());
> > +        }
> > +
> > +        public synchronized void runOnIcThread(Runnable runnable) {
> 
> Does runOnIcThread() need to be public? It is only called by
> ThreadUtils.getEditableForUiThread().

Made private.

> @@ +73,5 @@
> > +                mEditableClient.getInputConnectionHandler().post(runnable);
> > +            }
> > +        }
> > +
> > +        public void waitIcThread() {
> 
> I think we should rename `waitIcThread()` to `waitForUiThread()` and
> `waitUiThread()` to `waitForIcThread()`. Those names indicate what the
> methods do, rather than who is calling them.

Changed waitIcThread to waitForUiThread. For the notifyIcThread, I changed the name to "endWaitForUiThread" (I think that's a better matching name and also we are not using .notify() anymore). Also got rid of waitUiThread/notifyUiThread by inlining them.

> @@ +85,5 @@
> > +                    mIcThreadRunnable = null;
> > +                }
> > +                try {
> > +                    mIcThreadWaiting = true;
> > +                    wait(); // wait for UI thread to notify us
> 
> We might want to consider calling wait() method that takes a timeout. If we
> time out, we can log a warning that the UI thread is taking too long, throw
> an exception, or just return prematurely so we don't block the IC thread
> forever.

Hmm. Having a time out seems a bit arbitrary to me. We will most likely crash if we timeout, and I'd rather have the user decide whether to wait or to get killed (if we do happen to get to the point of ANR)

> @@ +720,4 @@
> >          if (mIMEState == IME_STATE_DISABLED ||
> > +                mIMEState == IME_STATE_PLUGIN ||
> > +                keyCode == KeyEvent.KEYCODE_ENTER ||
> > +                keyCode == KeyEvent.KEYCODE_DEL ||
> 
> Please fix the indentation like before.

Okay. I fixed the extra indentation we had in onKeyDown(), just to be consistent.

Comment 7

[Chris Peterson [:cpeterson]]

Comment on attachment 713486 [details] [diff] [review]
Provide UI-thread-safe Editable for KeyListener (v2)

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

LGTM

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f8fc3c17184

I wish we had a test for this. Maybe this can be one of your target bugs, cpeterson?

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6f8fc3c17184