1449569 - Crash in java.lang.NullPointerException: Attempt to invoke virtual method 'android.content.Context android.view.View.getContext()' on a null object reference at android.view.inputmethod.BaseInputConnection.<init>(BaseInputConnection.java)

Status: RESOLVED FIXED

(GeckoView :: General, defect, P2)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-f5c19902-e80e-489f-9f6a-aaaf70180323.
=============================================================

java.lang.NullPointerException: Attempt to invoke virtual method 'android.content.Context android.view.View.getContext()' on a null object reference
	at android.view.inputmethod.BaseInputConnection.<init>(BaseInputConnection.java:71)
	at org.mozilla.gecko.GeckoInputConnection.<init>(GeckoInputConnection.java:101)
	at org.mozilla.gecko.GeckoInputConnection.create(GeckoInputConnection.java:94)
	at org.mozilla.gecko.TextInputController.ensureInputConnection(TextInputController.java:147)
	at org.mozilla.gecko.TextInputController.onCreateInputConnection(TextInputController.java:192)
	at org.mozilla.gecko.GeckoView.onCreateInputConnection(GeckoView.java:378)
	at android.view.inputmethod.InputMethodManager.startInputInner(InputMethodManager.java:1280)
	at android.view.inputmethod.InputMethodManager.onPostWindowFocus(InputMethodManager.java:1608)
	at android.view.ViewRootImpl$ViewRootHandler.handleMessage(ViewRootImpl.java:4131)
	at android.os.Handler.dispatchMessage(Handler.java:108)
	at android.os.Looper.loop(Looper.java:166)
	at android.app.ActivityThread.main(ActivityThread.java:7390)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.Zygote$MethodAndArgsCaller.run(Zygote.java:245)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:926)

=============================================================

crash reports with this signature start regressing since firefox 59 - probably related to bug 1416918.

perhaps the comment to this report gives a clue what's going on/how to reproduce:
"If Firefox has focus when switching users, switching back again results in a crash. "

Comment 1

[Chris Peterson [:cpeterson]]

IME crash

Comment 2

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

Attachment: Bug 1449569 - 1. Move key event handling to GeckoEditable;

Move key event handling code from GeckoInputConnection to GeckoEditable,
so that we can still handle key events when we don't have a
View/InputConnection.

Review commit: https://reviewboard.mozilla.org/r/235502/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235502/

Comment 3

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

Attachment: Bug 1449569 - 2. Don't force-create GeckoInputConnection;

Now that we don't require a GeckoInputConnection to handle key events,
we should no longer force-create GeckoInputConnection. Instead, only
create GeckoInputConnection when we have a View, because without a View,
we will always crash when creating GeckoInputConnection under a recent
Android version.

Review commit: https://reviewboard.mozilla.org/r/235504/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235504/

Comment 4

[Andrei Lazar]

Attachment: Bug 1449569 - Use current input session view instead of passing a null value.

Review commit: https://reviewboard.mozilla.org/r/235642/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235642/

Comment 5

[Eugen Sawin [:esawin]]

Comment on attachment 8966823 [details]
Bug 1449569 - 1. Move key event handling to GeckoEditable;

https://reviewboard.mozilla.org/r/235502/#review241380

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java:867
(Diff revision 1)
> +
> +        final KeyListener keyListener = TextKeyListener.getInstance();
> +        event = translateKey(event.getKeyCode(), event);
> +
> +        // We only let TextKeyListener do UI things on the UI thread.
> +        final View v = ThreadUtils.isOnUiThread() ? view : null;

Please avoid using single-char variable names.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java:934
(Diff revision 1)
>      }
>  
> +    private boolean shouldSkipKeyListener(final int keyCode, final @NonNull KeyEvent event) {
> +        // Preserve enter and tab keys for the browser
> +        if (keyCode == KeyEvent.KEYCODE_ENTER ||
> +                keyCode == KeyEvent.KEYCODE_TAB) {

Indentation.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java:940
(Diff revision 1)
> +            return true;
> +        }
> +        // BaseKeyListener returns false even if it handled these keys for us,
> +        // so we skip the key listener entirely and handle these ourselves
> +        if (keyCode == KeyEvent.KEYCODE_DEL ||
> +                keyCode == KeyEvent.KEYCODE_FORWARD_DEL) {

Indentation.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java:1740
(Diff revision 1)
> +                }
> +            }
> +            return true;
> +        }
> +
> +        while ((repeatCount--) != 0) {

Maybe > 0 is safer to avoid blocking on bad input.

Comment 6

[Eugen Sawin [:esawin]]

Comment on attachment 8966824 [details]
Bug 1449569 - 2. Don't force-create GeckoInputConnection;

https://reviewboard.mozilla.org/r/235504/#review241386

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/SessionTextInput.java:178
(Diff revision 1)
>       *
>       * @param attrs EditorInfo instance to be filled on return.
>       * @return InputConnection instance or null if input method is not active.
>       */
> -    public @Nullable InputConnection onCreateInputConnection(final @NonNull EditorInfo attrs) {
> +    public synchronized @Nullable InputConnection onCreateInputConnection(
> +            final @NonNull EditorInfo attrs) {

I actually prefer the 3-way indentation for non-aligned method arguments, can we agree on that style and enforce it?

Comment 7

[Andrei Lazar]

I investigated this issue and I have found a speculative fix for this.

Comment 8

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

Comment on attachment 8966957 [details]
Bug 1449569 - Use current input session view instead of passing a null value.

https://reviewboard.mozilla.org/r/235642/#review241498

Thanks for the patch, though I'm not sure it would work.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/SessionTextInput.java:152
(Diff revision 1)
>          if (!mQueue.isReady()) {
>              return false;
>          }
>  
>          if (mInputConnection == null) {
> -            mInputConnection = GeckoInputConnection.create(mSession,
> +            mInputConnection = GeckoInputConnection.create(mSession, mEditable);

Inside `GeckoInputConnection.create`, `session.getTextInput().getView()` will return null because `mInputConnection` is null. So the behavior is the same as before.

Comment 9

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

Comment on attachment 8966823 [details]
Bug 1449569 - 1. Move key event handling to GeckoEditable;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235502/diff/1-2/

Comment 10

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

Comment on attachment 8966824 [details]
Bug 1449569 - 2. Don't force-create GeckoInputConnection;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235504/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3401d063be3f
1. Move key event handling to GeckoEditable; r=esawin
https://hg.mozilla.org/integration/autoland/rev/1376d3c78039
2. Don't force-create GeckoInputConnection; r=esawin

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3401d063be3f
https://hg.mozilla.org/mozilla-central/rev/1376d3c78039

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Sadly, it would appear this missed the boat for possible uplift to Fx60.