781748 - java.lang.NullPointerException: at org.mozilla.gecko.GeckoInputConnection.onCreateInputConnection(GeckoInputConnection.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[Scoobidiver (away)]

There are two crashes, one in 16.0a2/20120730 and another one in 16.0a2/20120809: bp-1e44bf93-34d8-4843-ac9f-f66942120809.

java.lang.NullPointerException
	at org.mozilla.gecko.GeckoInputConnection.onCreateInputConnection(GeckoInputConnection.java:847)
	at org.mozilla.gecko.gfx.LayerView.onCreateInputConnection(LayerView.java:115)
	at android.view.inputmethod.InputMethodManager.startInputInner(InputMethodManager.java:1048)
	at android.view.inputmethod.InputMethodManager.restartInput(InputMethodManager.java:1002)
	at org.mozilla.gecko.GeckoInputConnection$1.run(GeckoInputConnection.java:1084)
	at android.os.Handler.handleCallback(Handler.java:615)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:137)
	at android.app.ActivityThread.main(ActivityThread.java:4745)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:511)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:786)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553)
	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.onCreateInputConnection%28GeckoInputConnection.java%29

Comment 1

[Chris Peterson [:cpeterson]]

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

Attachment: handle-null-mIMETypeHint.patch

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

Attachment: part-2-handle-null-mIMEActionHint.patch

Part 2: Handle null mIMEActionHint.

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: part-3-test-IME-parameters.patch

Part 3: Add debug checks for IME state parameters.

Comment 5

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

Comment on attachment 651085 [details] [diff] [review]
handle-null-mIMETypeHint.patch

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

::: mobile/android/base/GeckoInputConnection.java
@@ +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?)]

Comment on attachment 651086 [details] [diff] [review]
part-2-handle-null-mIMEActionHint.patch

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

::: mobile/android/base/GeckoInputConnection.java
@@ +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?)]

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

Comment on attachment 651087 [details] [diff] [review]
part-3-test-IME-parameters.patch

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

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

what are these magic numbers?

Comment 9

[Chris Peterson [:cpeterson]]

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

Attachment: part-1-guard-against-null.patch

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

Attachment: part-2-test-IME-parameters.patch

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 12

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2eb3a82921b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ee83f13ff3e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2eb3a82921b8
https://hg.mozilla.org/mozilla-central/rev/5ee83f13ff3e

Comment 14

[Chris Peterson [:cpeterson]]

Comment on attachment 651824 [details] [diff] [review]
part-1-guard-against-null.patch

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

Comment on attachment 651824 [details] [diff] [review]
part-1-guard-against-null.patch

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/175f65b65826