If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 21

Status

()

Firefox for Android
Keyboards and IME
--
critical
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: Scoobidiver (away), Assigned: jchen)

Tracking

(4 keywords)

21 Branch
Firefox 21
ARM
Android
crash, regression, reproducible, topcrash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox21+ fixed)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 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

Updated

5 years ago
Component: General → Keyboards and IME
(Reporter)

Comment 1

5 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
tracking-firefox21: --- → ?
Keywords: regression, topcrash
Version: Trunk → Firefox 21
(Assignee)

Updated

5 years ago
Assignee: nobody → nchen
Status: NEW → ASSIGNED
(Reporter)

Updated

5 years ago
tracking-fennec: --- → ?

Updated

5 years ago
tracking-firefox21: ? → +
(Reporter)

Updated

5 years ago
Duplicate of this bug: 840403
(Reporter)

Comment 3

5 years ago
Bug 840403 has STR.
Keywords: reproducible
(Assignee)

Comment 4

5 years ago
Created attachment 713048 [details] [diff] [review]
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.
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+
(Assignee)

Comment 6

5 years ago
Created attachment 713486 [details] [diff] [review]
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.
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+
(Assignee)

Comment 8

5 years ago
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?
status-firefox21: affected → fixed
Target Milestone: --- → Firefox 21

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/6f8fc3c17184
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Depends on: 857413
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.