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

RESOLVED FIXED in Firefox 21

Status

()

defect
--
critical
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: scoobidiver, Assigned: jchen)

Tracking

(4 keywords)

21 Branch
Firefox 21
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox21+ fixed)

Details

(Whiteboard: [native-crash], crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

6 years ago
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
Component: General → Keyboards and IME
Reporter

Comment 1

6 years ago
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.
Blocks: 835906
Keywords: regression, topcrash
Version: Trunk → Firefox 21
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Reporter

Updated

6 years ago
tracking-fennec: --- → ?
Reporter

Updated

6 years ago
Duplicate of this bug: 840403
Reporter

Comment 3

6 years ago
Bug 840403 has STR.
Keywords: reproducible
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.
Attachment #713048 - Flags: review?(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.
Attachment #713048 - Flags: review?(cpeterson) → feedback+
(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.
Attachment #713048 - Attachment is obsolete: true
Attachment #713486 - Flags: review?(cpeterson)
Comment on attachment 713486 [details] [diff] [review]
Provide UI-thread-safe Editable for KeyListener (v2)

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

LGTM
Attachment #713486 - Flags: review?(cpeterson) → review+
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?
Target Milestone: --- → Firefox 21
https://hg.mozilla.org/mozilla-central/rev/6f8fc3c17184
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 857413
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.