Last Comment Bug 795226 - java.lang.NullPointerException: at org.mozilla.gecko.GeckoInputConnection$2.run(GeckoInputConnection.java)
: java.lang.NullPointerException: at org.mozilla.gecko.GeckoInputConnection$2.r...
Status: RESOLVED FIXED
[native-crash]
: crash, regression, topcrash
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 17 Branch
: ARM Android
: P1 critical (vote)
: Firefox 19
Assigned To: Jim Chen [:jchen] [:darchons]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-28 00:06 PDT by Scoobidiver (away)
Modified: 2012-10-22 14:16 PDT (History)
7 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
17+


Attachments
Patch to avoid crash scenario (1.27 KB, patch)
2012-10-17 10:48 PDT, Jim Chen [:jchen] [:darchons]
blassey.bugs: review+
Details | Diff | Review
Patch to avoid crash scenario (v2) (1.29 KB, patch)
2012-10-18 06:09 PDT, Jim Chen [:jchen] [:darchons]
nchen: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review

Description Scoobidiver (away) 2012-09-28 00:06:03 PDT
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 2 Scoobidiver (away) 2012-10-13 07:36:02 PDT
It's #4 top crasher in 17.0b1.
Comment 3 Chris Peterson [:cpeterson] 2012-10-15 10:51:16 PDT
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) 2012-10-15 11:11:39 PDT
(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 2012-10-15 16:11:21 PDT
Assigning to Chris to investigate, this is a topcrasher so we'll definitely track for 17.
Comment 6 Chris Peterson [:cpeterson] 2012-10-15 17:10:09 PDT
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 Jim Chen [:jchen] [:darchons] 2012-10-16 10:15:31 PDT
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 Jim Chen [:jchen] [:darchons] 2012-10-17 10:48:39 PDT
Created attachment 672391 [details] [diff] [review]
Patch to avoid crash scenario

It is better to check for null composing span in this case
Comment 9 Brad Lassey [:blassey] (use needinfo?) 2012-10-17 14:20:50 PDT
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 Jim Chen [:jchen] [:darchons] 2012-10-17 14:29:24 PDT
(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] 2012-10-17 15:32:11 PDT
(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 Jim Chen [:jchen] [:darchons] 2012-10-18 06:09:24 PDT
Created attachment 672759 [details] [diff] [review]
Patch to avoid crash scenario (v2)

Okay added the hasCompositionString check. Carried over r+. Thanks, Brad and Chris!
Comment 13 Chris Peterson [:cpeterson] 2012-10-18 09:23:48 PDT
jchen, this is a topcrash for Beta 17, so we will want your fix uplifted to Aurora and Beta.
Comment 14 Jim Chen [:jchen] [:darchons] 2012-10-18 10:04:13 PDT
Please check into m-c for now.
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-10-18 19:23:14 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/977d17351049
Comment 16 Ed Morley [:emorley] 2012-10-19 07:26:13 PDT
https://hg.mozilla.org/mozilla-central/rev/977d17351049
Comment 17 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-21 08:54:02 PDT
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 Jim Chen [:jchen] [:darchons] 2012-10-22 07:29:01 PDT
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 Jim Chen [:jchen] [:darchons] 2012-10-22 12:44:21 PDT
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

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