NPE in setComposingText when using VKB

VERIFIED FIXED in Firefox 5

Status

()

Core
Widget: Android
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: jdm, Assigned: m_kato)

Tracking

({crash})

Trunk
mozilla5
ARM
Android
crash
Points:
---

Firefox Tracking Flags

(firefox5+ fixed, fennec4.0.1+)

Details

(Whiteboard: [fixed-in-aurora][fixed-in-2.1])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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?
(Reporter)

Updated

6 years ago
Keywords: crash
(Reporter)

Comment 1

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

Comment 2

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

Updated

6 years ago
tracking-fennec: --- → 4.0.1+
Duplicate of this bug: 640454
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).
(Assignee)

Comment 5

6 years ago
Created attachment 522643 [details] [diff] [review]
fix v1
Assignee: nobody → m_kato
Status: NEW → ASSIGNED
(Assignee)

Comment 6

6 years ago
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?
Duplicate of this bug: 646140
(Assignee)

Updated

6 years ago
OS: Linux → Android
Hardware: x86 → ARM

Comment 8

6 years ago
makoto, does this actually work?  I like this better than testing for both empty strings and null.
(Assignee)

Comment 9

6 years ago
(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?
(Reporter)

Comment 10

6 years ago
I'll give it a try tonight.  I just haven't had access to my device for the past couple days.
(Reporter)

Comment 11

6 years ago
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.
(Assignee)

Comment 12

6 years ago
Comment on attachment 522643 [details] [diff] [review]
fix v1

Does mComposingText set "" instead of?
Attachment #522643 - Flags: review?(mwu)

Comment 13

6 years ago
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..
Attachment #522643 - Flags: feedback?(jimnchen+bmo)
(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.
(Assignee)

Comment 15

6 years ago
(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.
(Assignee)

Comment 16

6 years ago
Created attachment 524281 [details] [diff] [review]
fix v2
Attachment #522643 - Attachment is obsolete: true
Attachment #522643 - Flags: review?(mwu)
Attachment #522643 - Flags: feedback?(jimnchen+bmo)
(Assignee)

Updated

6 years ago
Attachment #524281 - Flags: review?(mwu)

Comment 17

6 years ago
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..
Attachment #524281 - Flags: review?(mwu) → review+
(Assignee)

Comment 18

6 years ago
(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

6 years ago
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
(Assignee)

Comment 20

6 years ago
(In reply to comment #19)

It seems to be unnecessary to set that mUpdateRequest is null.
(Assignee)

Comment 21

6 years ago
Created attachment 525058 [details] [diff] [review]
for checkin

Updated

6 years ago
Keywords: checkin-needed
m-c:  http://hg.mozilla.org/mozilla-central/rev/8f0996418826
2.1: http://hg.mozilla.org/releases/mozilla-2.1/rev/8d48b8766662
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Keywords: checkin-needed

Comment 24

6 years ago
This also needs to land on aurora
tracking-firefox5: --- → ?
a=bsmedberg for aurora
tracking-firefox5: ? → +
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.
Attachment #525058 - Flags: review+
Attachment #525058 - Flags: approval-mozilla-aurora?
Whiteboard: [needs to land on aurora]
Pushed to Aurora: http://hg.mozilla.org/mozilla-aurora/rev/5eb315204093
Whiteboard: [needs to land on aurora] → [fixed-in-aurora][fixed-in-2.1]
Target Milestone: --- → mozilla5

Updated

6 years ago
Attachment #525058 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Status: RESOLVED → VERIFIED
status-firefox5: --- → fixed
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
(Reporter)

Comment 30

6 years ago
To clarify, I realized that I was triggering this bug on the credits link from about:firefox rather than about:credits.
You need to log in before you can comment on or make changes to this bug.