Last Comment Bug 644613 - NPE in setComposingText when using VKB
: NPE in setComposingText when using VKB
Status: VERIFIED FIXED
[fixed-in-aurora][fixed-in-2.1]
: crash
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla5
Assigned To: Makoto Kato [:m_kato]
:
Mentors:
: 640454 646140 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-24 08:54 PDT by Josh Matthews [:jdm]
Modified: 2011-05-09 14:21 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
4.0.1+


Attachments
fix v1 (1.15 KB, patch)
2011-03-29 04:45 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
fix v2 (585 bytes, patch)
2011-04-06 15:09 PDT, Makoto Kato [:m_kato]
mwu.code: review+
Details | Diff | Splinter Review
for checkin (1.25 KB, patch)
2011-04-11 05:05 PDT, Makoto Kato [:m_kato]
mbrubeck: review+
jst: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] 2011-03-24 08:54:33 PDT
E/AndroidRuntime( 7673): Uncaught handler: thread main exiting due to uncaught exception
E/AndroidRuntime( 7673): java.lang.NullPointerException
E/AndroidRuntime( 7673): 	at org.mozilla.gecko.GeckoInputConnection.setComposingText(GeckoInputConnection.java:347)
E/AndroidRuntime( 7673): 	at org.mozilla.gecko.GeckoInputConnection.commitText(GeckoInputConnection.java:83)
E/AndroidRuntime( 7673): 	at com.android.internal.view.IInputConnectionWrapper.executeMessage(IInputConnectionWrapper.java:231)
E/AndroidRuntime( 7673): 	at com.android.internal.view.IInputConnectionWrapper$MyHandler.handleMessage(IInputConnectionWrapper.java:57)
E/AndroidRuntime( 7673): 	at android.os.Handler.dispatchMessage(Handler.java:99)
E/AndroidRuntime( 7673): 	at android.os.Looper.loop(Looper.java:123)
E/AndroidRuntime( 7673): 	at android.app.ActivityThread.main(ActivityThread.java:4363)
E/AndroidRuntime( 7673): 	at java.lang.reflect.Method.invokeNative(Native Method)
E/AndroidRuntime( 7673): 	at java.lang.reflect.Method.invoke(Method.java:521)
E/AndroidRuntime( 7673): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:862)
E/AndroidRuntime( 7673): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:620)
E/AndroidRuntime( 7673): 	at dalvik.system.NativeStart.main(Native Method)

STR:
1. go to about:credits
2. open page find
3. type letters at a very reasonable pace

At some point (I got it just by typing "josh" once) Fennec stops responding and logcat shows the above.  What's bizarre is that the failing line is apparently

347         mCompositionSelStart = newCursorPosition > 0 ? mComposingText.length() : 0;

which is confusing, since we explicitly set mComposingText to either a toString() value or "".  Maybe CharSequence.toString() can return null?
Comment 1 Josh Matthews [:jdm] 2011-03-24 09:37:51 PDT
It might be worth adding a |text.length != 0| check when setting mComposingText.
Comment 2 Doug Turner (:dougt) 2011-03-24 10:51:32 PDT
according to stuart, this is a top java crash as reported from the android market.
Comment 3 Matt Brubeck (:mbrubeck) 2011-03-28 16:51:15 PDT
*** Bug 640454 has been marked as a duplicate of this bug. ***
Comment 4 Matt Brubeck (:mbrubeck) 2011-03-28 17:00:41 PDT
According to Makoto in bug 640454 comment 1, this might be caused by reset() setting mComposingText to null in another thread (if I understand him correctly).
Comment 5 Makoto Kato [:m_kato] 2011-03-29 04:45:02 PDT
Created attachment 522643 [details] [diff] [review]
fix v1
Comment 6 Makoto Kato [:m_kato] 2011-03-29 04:51:14 PDT
I cannot reproduce this...

Josh, could you reproduce this using http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/m_kato@ga2.so-net.ne.jp-69cb35189f5f/try-mb-br-andrd-r7-bld/fennec-4.1a1pre.en-US.eabi-arm.apk?
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2011-03-29 12:31:49 PDT
*** Bug 646140 has been marked as a duplicate of this bug. ***
Comment 8 Doug Turner (:dougt) 2011-03-30 09:49:49 PDT
makoto, does this actually work?  I like this better than testing for both empty strings and null.
Comment 9 Makoto Kato [:m_kato] 2011-03-30 10:12:25 PDT
(In reply to comment #8)
> makoto, does this actually work?  I like this better than testing for both
> empty strings and null.

I cannot reproduce this.  Although I create testing package via try server, Josh is no reply.  Maybe, mComposingText should empty string instead of no modified...

Also, at first, although I add synchronized code, it may cause dead lock.

Josh, could you test comment 6's package?
Comment 10 Josh Matthews [:jdm] 2011-03-30 11:40:15 PDT
I'll give it a try tonight.  I just haven't had access to my device for the past couple days.
Comment 11 Josh Matthews [:jdm] 2011-03-31 22:55:09 PDT
While I've never found guaranteed STR, the steps in comment 0 have usually been fairly reliable for me. With that in mind, I have not been able to reproduce the crash in the build posted.
Comment 12 Makoto Kato [:m_kato] 2011-04-01 22:03:11 PDT
Comment on attachment 522643 [details] [diff] [review]
fix v1

Does mComposingText set "" instead of?
Comment 13 Michael Wu [:mwu] 2011-04-02 01:04:33 PDT
Comment on attachment 522643 [details] [diff] [review]
fix v1

jchen, any thoughts?

I think I would prefer to do mComposingText = "";.

An interesting thing to try in setComposingText would be to 

String composingText = mComposingText = text != null ? text.toString() : "";

and use composingText in the rest of the function. If this fixes it, it is likely a thread issue. However, as far as I know, everything is on the same thread here..
Comment 14 Jim Chen [:jchen] [:darchons] 2011-04-02 21:17:20 PDT
(In reply to comment #13)
> Comment on attachment 522643 [details] [diff] [review]
> fix v1
> 
> I think I would prefer to do mComposingText = "";.

Me too. And we have to change the line in finishComposingText as well.

> An interesting thing to try in setComposingText would be to 
> 
> String composingText = mComposingText = text != null ? text.toString() : "";
> 
> and use composingText in the rest of the function. If this fixes it, it is
> likely a thread issue. However, as far as I know, everything is on the same
> thread here..

Maybe it's between Java and native threads? In particular, Gecko could be calling GeckoAppShell.notifyIME, which calls GeckoInputConnection.reset, while the Java thread is waiting on the IME_GET_SELECTION event that we send in setComposingText.

In that case, your solution would work. We can also move the line
> mComposingText = text != null ? text.toString() : "";
to after the if block, so we send out IME_GET_SELECTION before we mess with mComposingText.

It's only a speculation though. I haven't been able to reproduce the bug on the Droid I have.
Comment 15 Makoto Kato [:m_kato] 2011-04-02 22:05:59 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > Comment on attachment 522643 [details] [diff] [review]
> > fix v1
> > 
> > I think I would prefer to do mComposingText = "";.
> 
> Me too. And we have to change the line in finishComposingText as well.

OK. I will update this patch.

> > An interesting thing to try in setComposingText would be to 
> > 
> > String composingText = mComposingText = text != null ? text.toString() : "";
> > 
> > and use composingText in the rest of the function. If this fixes it, it is
> > likely a thread issue. However, as far as I know, everything is on the same
> > thread here..
> 
> Maybe it's between Java and native threads? In particular, Gecko could be
> calling GeckoAppShell.notifyIME, which calls GeckoInputConnection.reset, while
> the Java thread is waiting on the IME_GET_SELECTION event that we send in
> setComposingText.

setCompositinText is from activity.  But reset is from ResetInputState, so I think that it is on Gecko main thread.
Comment 16 Makoto Kato [:m_kato] 2011-04-06 15:09:37 PDT
Created attachment 524281 [details] [diff] [review]
fix v2
Comment 17 Michael Wu [:mwu] 2011-04-06 15:31:18 PDT
Comment on attachment 524281 [details] [diff] [review]
fix v2

Please also change the mComposingText setting in finishComposingText as jchen suggested.

Do we need to remove mUpdateRequest = null? Is that crashing? I guess it should be mostly safe either way..
Comment 18 Makoto Kato [:m_kato] 2011-04-07 09:12:20 PDT
(In reply to comment #17)
> Comment on attachment 524281 [details] [diff] [review]
> fix v2
> 
> Please also change the mComposingText setting in finishComposingText as jchen
> suggested.
> 
> Do we need to remove mUpdateRequest = null? Is that crashing? I guess it should be mostly safe either way..

I think that it is possible if Java method is called during reset() on another thread.  Since I don't access crash data on Market, I don't know whether there is a crash data that mUpdateRequest is null.  If nothing, it is unnecessary to remove this.
Comment 19 Stuart Parmenter 2011-04-07 13:17:07 PDT
java.lang.NullPointerException
at org.mozilla.gecko.GeckoInputConnection.setComposingText(GeckoInputConnection.java:347)
at org.mozilla.gecko.GeckoInputConnection.commitText(GeckoInputConnection.java:83)
at com.android.internal.view.IInputConnectionWrapper.executeMessage(IInputConnectionWrapper.java:257)
at com.android.internal.view.IInputConnectionWrapper$MyHandler.handleMessage(IInputConnectionWrapper.java:77)
at android.os.Handler.dispatchMessage(Handler.java:99)
at android.os.Looper.loop(Looper.java:123)
at android.app.ActivityThread.main(ActivityThread.java:4627)
at java.lang.reflect.Method.invokeNative(Native Method)
at java.lang.reflect.Method.invoke(Method.java:521)
at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:871)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:629)
at dalvik.system.NativeStart.main(Native Method)

is what we've been seeing
Comment 20 Makoto Kato [:m_kato] 2011-04-11 05:05:36 PDT
(In reply to comment #19)

It seems to be unnecessary to set that mUpdateRequest is null.
Comment 21 Makoto Kato [:m_kato] 2011-04-11 05:05:57 PDT
Created attachment 525058 [details] [diff] [review]
for checkin
Comment 22 Doug Turner (:dougt) 2011-04-12 11:19:18 PDT
m-c:  http://hg.mozilla.org/mozilla-central/rev/8f0996418826
Comment 23 Doug Turner (:dougt) 2011-04-12 11:23:33 PDT
2.1: http://hg.mozilla.org/releases/mozilla-2.1/rev/8d48b8766662
Comment 24 Stuart Parmenter 2011-04-12 13:05:25 PDT
This also needs to land on aurora
Comment 25 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-04-13 12:14:03 PDT
a=bsmedberg for aurora
Comment 26 Matt Brubeck (:mbrubeck) 2011-04-13 14:36:22 PDT
Comment on attachment 525058 [details] [diff] [review]
for checkin

bsmedberg gave approval-mozilla-aurora+ in comment 25.  Setting the correct flag now that it's available, just for record-keeping purposes.
Comment 27 Matt Brubeck (:mbrubeck) 2011-04-13 15:12:27 PDT
Pushed to Aurora: http://hg.mozilla.org/mozilla-aurora/rev/5eb315204093
Comment 28 Kevin Brosnan [:kbrosnan] 2011-04-19 16:03:52 PDT
Verified

Mozilla/5.0 (Android; Linux armv7l; rv:6.0a1) Gecko/20110419 Firefox/6.0a1
Fennec/6.0a1 ID:20110419042214

Mozilla/5.0 (Android; Linux armv7l; rv:2.1.1) Gecko/20110415 Firefox/4.0.2pre
Fennec/4.0.1 ID:20110415172201
Comment 29 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-05-09 14:19:09 PDT
cannot verify due to bug 615223

Aurora Branch:
Mozilla/5.0 (Android; Linux armv71; rv5.0a2) Gecko/20110509 Firefox/5.0a2 Fennec/5.0a2
Device: Thunderbolt
OS: Android 2.2
Comment 30 Josh Matthews [:jdm] 2011-05-09 14:21:53 PDT
To clarify, I realized that I was triggering this bug on the credits link from about:firefox rather than about:credits.

Note You need to log in before you can comment on or make changes to this bug.