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

RESOLVED FIXED in Firefox 16

Status

()

Firefox for Android
Keyboards and IME
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Scoobidiver (away), Assigned: cpeterson)

Tracking

({crash})

16 Branch
Firefox 17
ARM
Android
crash
Points:
---

Firefox Tracking Flags

(firefox14 wontfix, firefox15 wontfix, firefox16 fixed, firefox17 fixed)

Details

(Whiteboard: [native-crash], crash signature)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

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

Comment 1

5 years ago
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.
Assignee: nobody → cpeterson
Status: NEW → ASSIGNED
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
(Assignee)

Comment 2

5 years ago
Created attachment 651085 [details] [diff] [review]
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).
Attachment #651085 - Flags: review?(blassey.bugs)
(Assignee)

Comment 3

5 years ago
Created attachment 651086 [details] [diff] [review]
part-2-handle-null-mIMEActionHint.patch

Part 2: Handle null mIMEActionHint.
Attachment #651086 - Flags: review?(blassey.bugs)
(Assignee)

Comment 4

5 years ago
Created attachment 651087 [details] [diff] [review]
part-3-test-IME-parameters.patch

Part 3: Add debug checks for IME state parameters.
Attachment #651087 - Flags: review?(blassey.bugs)
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"
Attachment #651085 - Flags: review?(blassey.bugs) → review-
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
Attachment #651086 - Flags: review?(blassey.bugs) → review-
(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 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?
(Assignee)

Comment 9

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

Comment 10

5 years ago
Created attachment 651824 [details] [diff] [review]
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.
Attachment #651085 - Attachment is obsolete: true
Attachment #651086 - Attachment is obsolete: true
Attachment #651824 - Flags: review?(blassey.bugs)
(Assignee)

Comment 11

5 years ago
Created attachment 651828 [details] [diff] [review]
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.
Attachment #651087 - Attachment is obsolete: true
Attachment #651087 - Flags: review?(blassey.bugs)
Attachment #651828 - Flags: review?(blassey.bugs)
Attachment #651824 - Flags: review?(blassey.bugs) → review+
Attachment #651828 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2eb3a82921b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ee83f13ff3e
status-firefox17: affected → fixed
https://hg.mozilla.org/mozilla-central/rev/2eb3a82921b8
https://hg.mozilla.org/mozilla-central/rev/5ee83f13ff3e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
(Assignee)

Comment 14

5 years ago
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.
Attachment #651824 - Flags: approval-mozilla-beta?
Attachment #651824 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 15

5 years ago
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.
Attachment #651824 - Flags: approval-mozilla-beta?
Attachment #651824 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox14: affected → wontfix
status-firefox15: affected → wontfix
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/175f65b65826
status-firefox16: affected → fixed
You need to log in before you can comment on or make changes to this bug.