Last Comment Bug 781748 - java.lang.NullPointerException: at org.mozilla.gecko.GeckoInputConnection.onCreateInputConnection(
: java.lang.NullPointerException: at org.mozilla.gecko.GeckoInputConnection.onC...
: crash
Product: Firefox for Android
Classification: Client Software
Component: Keyboards and IME (show other bugs)
: 16 Branch
: ARM Android
: -- critical (vote)
: Firefox 17
Assigned To: Chris Peterson [:cpeterson]
: Jim Chen [:jchen] [:darchons]
Depends on:
  Show dependency treegraph
Reported: 2012-08-09 23:53 PDT by Scoobidiver (away)
Modified: 2012-08-20 16:03 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

handle-null-mIMETypeHint.patch (5.02 KB, patch)
2012-08-10 21:36 PDT, Chris Peterson [:cpeterson]
blassey.bugs: review-
Details | Diff | Splinter Review
part-2-handle-null-mIMEActionHint.patch (4.27 KB, patch)
2012-08-10 21:36 PDT, Chris Peterson [:cpeterson]
blassey.bugs: review-
Details | Diff | Splinter Review
part-3-test-IME-parameters.patch (2.53 KB, patch)
2012-08-10 21:37 PDT, Chris Peterson [:cpeterson]
no flags Details | Diff | Splinter Review
part-1-guard-against-null.patch (4.08 KB, patch)
2012-08-14 10:51 PDT, Chris Peterson [:cpeterson]
blassey.bugs: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
part-2-test-IME-parameters.patch (2.45 KB, patch)
2012-08-14 10:55 PDT, Chris Peterson [:cpeterson]
blassey.bugs: review+
Details | Diff | Splinter Review

Description Scoobidiver (away) 2012-08-09 23:53:05 PDT
There are two crashes, one in 16.0a2/20120730 and another one in 16.0a2/20120809: bp-1e44bf93-34d8-4843-ac9f-f66942120809.

	at org.mozilla.gecko.GeckoInputConnection.onCreateInputConnection(
	at org.mozilla.gecko.gfx.LayerView.onCreateInputConnection(
	at android.view.inputmethod.InputMethodManager.startInputInner(
	at android.view.inputmethod.InputMethodManager.restartInput(
	at org.mozilla.gecko.GeckoInputConnection$
	at android.os.Handler.handleCallback(
	at android.os.Handler.dispatchMessage(
	at android.os.Looper.loop(
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(
	at dalvik.system.NativeStart.main(Native Method)

More reports at:
Comment 1 Chris Peterson [:cpeterson] 2012-08-10 21:19:51 PDT
GeckoInputConnection's mIMETypeHint looks like it is null. mIMETypeHint and mIMEActionHint can be set to null if Gecko passes in a null. The affected code is in Firefox 14, 15, 16, and 17.
Comment 2 Chris Peterson [:cpeterson] 2012-08-10 21:36:06 PDT
Created attachment 651085 [details] [diff] [review]

Part 1: Handle null mIMETypeHint.

If mIMETypeHint is null we can skip a bunch of if/equals tests.

Also, reverse the checks to use the "null-proof" "string".equalsIgnoreCase(mIMETypeHint) idiom. We know mIMETypeHint is not null, but this is a recommended style (and I use it in some upcoming patches).
Comment 3 Chris Peterson [:cpeterson] 2012-08-10 21:36:46 PDT
Created attachment 651086 [details] [diff] [review]

Part 2: Handle null mIMEActionHint.
Comment 4 Chris Peterson [:cpeterson] 2012-08-10 21:37:25 PDT
Created attachment 651087 [details] [diff] [review]

Part 3: Add debug checks for IME state parameters.
Comment 5 Brad Lassey [:blassey] (use needinfo?) 2012-08-13 16:59:02 PDT
Comment on attachment 651085 [details] [diff] [review]

Review of attachment 651085 [details] [diff] [review]:

::: mobile/android/base/
@@ +801,2 @@
>              outAttrs.inputType |= InputType.TYPE_TEXT_VARIATION_PASSWORD;
> +        } else if (mIMETypeHint != null) {

you shouldn't need this check since you're doing "string literal".equalsIgnoreCase"
Comment 6 Brad Lassey [:blassey] (use needinfo?) 2012-08-13 17:01:18 PDT
Comment on attachment 651086 [details] [diff] [review]

Review of attachment 651086 [details] [diff] [review]:

::: mobile/android/base/
@@ +825,5 @@
>              else if (DEBUG)
>                  Log.d(LOGTAG, "Unexpected mIMETypeHint=\"" + mIMETypeHint + "\"");
>          }
> +        if (mIMEActionHint != null) {

shouldn't need this null check
Comment 7 Brad Lassey [:blassey] (use needinfo?) 2012-08-13 17:04:41 PDT
(In reply to Chris Peterson (:cpeterson) from comment #2)
> If mIMETypeHint is null we can skip a bunch of if/equals tests.
Is this common? Seems like overkill
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2012-08-13 17:06:08 PDT
Comment on attachment 651087 [details] [diff] [review]

Review of attachment 651087 [details] [diff] [review]:

::: mobile/android/base/
@@ +1469,2 @@
>          GeckoApp.assertOnGeckoThread();
> +        if (state < 0 || state > 1)

what are these magic numbers?
Comment 9 Chris Peterson [:cpeterson] 2012-08-14 08:20:01 PDT
(In reply to Brad Lassey [:blassey] from comment #7)
> > If mIMETypeHint is null we can skip a bunch of if/equals tests.
> Is this common? Seems like overkill

Gecko resets mIMEActionHint and mIMETypeHint to "" every time an input form loses focus.

We didn't see crashes before because the code previously initialized mIMEActionHint and mIMETypeHint to "". And then one day Gecko passed us a null.

On further reflection, a simpler fix would be to just protect against null values from Gecko (and changed them to ""). I could then throw out all these null checks.
Comment 10 Chris Peterson [:cpeterson] 2012-08-14 10:51:34 PDT
Created attachment 651824 [details] [diff] [review]

Guard against Gecko sending us null values for mIMEActionHint or mIMETypeHint. GeckoInputConnection can (now) safely assume that these instance variables are never null.
Comment 11 Chris Peterson [:cpeterson] 2012-08-14 10:55:04 PDT
Created attachment 651828 [details] [diff] [review]

patch v2 removes debug check for state range. state was a boolean value from Gecko, but we never actually use it so asserting its value is not particularly useful. We still log the value however.

patch v2 also adds double-quotes around some string parameters in log messages to make "" empty strings easier to identify from just a missing value.
Comment 14 Chris Peterson [:cpeterson] 2012-08-19 18:50:02 PDT
Comment on attachment 651824 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Only a few crash reports have hit this bug, so the impact if declined would be small.
Testing completed (on m-c, etc.): Baked on m-c for a few days without any new crash reports.
Risk to taking this patch (and alternatives if risky): Low-risk because this is small, mobile-only change that just adds some null checks.
String or UUID changes made by this patch: N/A

Only patch #1 would need to be uplifted; patch #2 just adds some debug logging.
Comment 15 Chris Peterson [:cpeterson] 2012-08-19 19:13:40 PDT
Comment on attachment 651824 [details] [diff] [review]

Since this is not a major issue, I'd like to request approval for Aurora 16, but not Beta 15.
Comment 16 Chris Peterson [:cpeterson] 2012-08-20 16:03:44 PDT

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