644613 - NPE in setComposingText when using VKB

Status: VERIFIED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Josh Matthews [:jdm]]

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]]

It might be worth adding a |text.length != 0| check when setting mComposingText.

Comment 2

[Doug Turner (:dougt)]

according to stuart, this is a top java crash as reported from the android market.

Comment 4

[Matt Brubeck (:mbrubeck)]

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]]

Attachment: fix v1

Comment 6

[Makoto Kato [:m_kato]]

I cannot reproduce this...

Josh, could you reproduce this using http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-69cb35189f5f/try-mb-br-andrd-r7-bld/fennec-4.1a1pre.en-US.eabi-arm.apk?

Comment 8

[Doug Turner (:dougt)]

makoto, does this actually work?  I like this better than testing for both empty strings and null.

Comment 9

[Makoto Kato [:m_kato]]

(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]]

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]]

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]]

Comment on attachment 522643 [details] [diff] [review]
fix v1

Does mComposingText set "" instead of?

Comment 13

[Michael Wu [:mwu]]

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

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

(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]]

(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]]

Attachment: fix v2

Comment 17

[Michael Wu [:mwu]]

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]]

(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]

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]]

(In reply to comment #19)

It seems to be unnecessary to set that mUpdateRequest is null.

Comment 21

[Makoto Kato [:m_kato]]

Attachment: for checkin

Comment 22

[Doug Turner (:dougt)]

m-c:  http://hg.mozilla.org/mozilla-central/rev/8f0996418826

Comment 23

[Doug Turner (:dougt)]

2.1: http://hg.mozilla.org/releases/mozilla-2.1/rev/8d48b8766662

Comment 24

[Stuart Parmenter]

This also needs to land on aurora

Comment 25

[Benjamin Smedberg]

a=bsmedberg for aurora

Comment 26

[Matt Brubeck (:mbrubeck)]

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)]

Pushed to Aurora: http://hg.mozilla.org/mozilla-aurora/rev/5eb315204093

Comment 28

[Kevin Brosnan [Ex-Mozilla]]

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)]

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]]

To clarify, I realized that I was triggering this bug on the credits link from about:firefox rather than about:credits.