795226 - java.lang.NullPointerException: at org.mozilla.gecko.GeckoInputConnection$2.run(GeckoInputConnection.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Scoobidiver (away)]

There's one crash in 18.0a1/20120927114117: bp-f1634d1c-0f3b-4b17-b754-c3a5d2120928.

java.lang.NullPointerException
	at org.mozilla.gecko.GeckoInputConnection$2.run(GeckoInputConnection.java:514)
	at android.os.Handler.handleCallback(Handler.java:587)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:130)
	at android.app.ActivityThread.main(ActivityThread.java:3683)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:507)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:839)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:597)
	at dalvik.system.NativeStart.main(Native Method)

More reports at:
https://crash-stats.mozilla.com/report/list?signature=java.lang.NullPointerException%3A+at+org.mozilla.gecko.GeckoInputConnection%242.run%28GeckoInputConnection.java%29

Comment 1

[Scoobidiver (away)]

It was uplifted to Aurora. The regression ranges might be:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ca4af4af5334&tochange=b038e9e2023f
http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=846561f033db&tochange=a4880b03634f
It might be a regression from bug 793943, bug 792677, bug 793935, bug 793354, bug 788589

Comment 2

[Scoobidiver (away)]

It's #4 top crasher in 17.0b1.

Comment 3

[Chris Peterson [:cpeterson]]

This is our #4 topcrash on Beta 17.

This looks like fallout from bug 788600 (which backed out bug 769520). GeckoInputConnection thinks it hasCompositionString(), but then getComposingSpan() returns null and we crash.

Comment 4

[Scoobidiver (away)]

(In reply to Chris Peterson (:cpeterson) from comment #3)
> This looks like fallout from bug 788600.
That bug landed in 15.0.1 while this bug appeared in 17.0.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Assigning to Chris to investigate, this is a topcrasher so we'll definitely track for 17.

Comment 6

[Chris Peterson [:cpeterson]]

jchen, do you want to take a look at this topcrash bug? I think I have a band-aid fix for this crash, but I don't have time to test it and uplift it for Beta 17 because I'm supposed to be working on B2G/Gaia bugs.

Comment 7

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

I can take it. Are there STR? I see that the keyboard has to be fullscreen but otherwise I'm not sure how to trigger it.

Comment 8

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

Attachment: Patch to avoid crash scenario

It is better to check for null composing span in this case

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 672391 [details] [diff] [review]
Patch to avoid crash scenario

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

::: mobile/android/base/GeckoInputConnection.java
@@ +509,5 @@
>                  if (imm != null && imm.isFullscreenMode()) {
>                      int newStart;
>                      int newEnd;
> +                    Span span = getComposingSpan();
> +                    if (span != null) {

should this be:
if (span != null && hasCompositionString()) {

Comment 10

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

(In reply to Brad Lassey [:blassey] from comment #9)
> Comment on attachment 672391 [details] [diff] [review]
> Patch to avoid crash scenario
> 
> Review of attachment 672391 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/GeckoInputConnection.java
> @@ +509,5 @@
> >                  if (imm != null && imm.isFullscreenMode()) {
> >                      int newStart;
> >                      int newEnd;
> > +                    Span span = getComposingSpan();
> > +                    if (span != null) {
> 
> should this be:
> if (span != null && hasCompositionString()) {

I checked with cpeterson and he said it's redundant to check both getComposingSpan and hasCompositionString.

Comment 11

[Chris Peterson [:cpeterson]]

(In reply to Jim Chen [:jchen :nchen] from comment #10)
> I checked with cpeterson and he said it's redundant to check both
> getComposingSpan and hasCompositionString.

They *ought* to be redundant, but I am not confident they are. getComposingSpan queries the IME and hasCompositionString just tracks composition state from events.

I think we should check both span != null && hasCompositionString() until we can test that they are redundant (and then remove hasCompositionString).

Comment 12

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

Attachment: Patch to avoid crash scenario (v2)

Okay added the hasCompositionString check. Carried over r+. Thanks, Brad and Chris!

Comment 13

[Chris Peterson [:cpeterson]]

jchen, this is a topcrash for Beta 17, so we will want your fix uplifted to Aurora and Beta.

Comment 14

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

Please check into m-c for now.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/977d17351049

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/977d17351049

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Can we get an uplift nomination here?  Having this in the next Beta (ie: land by tomorrow) would let us gather more data and see the crash volume for this signature hopefully decrease.

Comment 18

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

Comment on attachment 672759 [details] [diff] [review]
Patch to avoid crash scenario (v2)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): race condition; missing null check
User impact if declined: topcrash for current Beta
Testing completed (on m-c, etc.): none, because there is no definite STR for the crash. can only be tested by looking at crashstats
Risk to taking this patch (and alternatives if risky): very small risk. changes in patch only applies to cases where we used to crash
String or UUID changes made by this patch: none

Comment 19

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

Comment on attachment 672759 [details] [diff] [review]
Patch to avoid crash scenario (v2)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): race condition; missing null check
User impact if declined: topcrash for current Beta
Testing completed (on m-c, etc.): none, because there is no definite STR for the crash. can only be tested by looking at crashstats
Risk to taking this patch (and alternatives if risky): very small risk. changes in patch only apply to cases where we used to crash
String or UUID changes made by this patch: none

Comment 20

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/f67f7ed3d53a

https://hg.mozilla.org/releases/mozilla-beta/rev/9576d3860ac3