1134205 - Debug assertion on text input in input forms

Status: RESOLVED WORKSFORME

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

Description

[Eugen Sawin [:esawin]]

STR (debug build required)
1. Open https://google.com
2. Type "ab" into the search form

Expected: input is accepted and Google shows search suggestions as you type.

Actual: Crash due to assertion in IMEStateManager::DispatchCompositionEvent.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1134205-Fix-and-refactor-GeckoEditable-debug-out.patch

The reflection-based name resolution for debug output is broken, which makes it hard to follow events by their IDs. This patch switches the event types to enums for increased type safety and robust debug output.

Comment 2

[Chris Peterson [:cpeterson]]

Comment on attachment 8566008 [details] [diff] [review]
0001-Bug-1134205-Fix-and-refactor-GeckoEditable-debug-out.patch

Jim?

Comment 3

[Chris Peterson [:cpeterson]]

Comment on attachment 8566008 [details] [diff] [review]
0001-Bug-1134205-Fix-and-refactor-GeckoEditable-debug-out.patch

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

LGTM!

Is this crash a regression? Do we need to uplift this fix to Aurora or Beta?

::: mobile/android/base/GeckoEditable.java
@@ +105,5 @@
> +            NONE(9999);
> +
> +            final int value;
> +
> +            Type(int value) {

Can you make the Type constructor private instead of package-visible? It looks like nothing prevents a caller from creating their own Type instance with an invalid value. (Same questions for NotifyIME and IMEState enums below.)

@@ +115,5 @@
> +                    if (t.value == value) {
> +                        return t;
> +                    }
> +                }
> +                return NONE;

Is NONE actually a valid value? We didn't have a NONE type before. Can we just throw an IllegalArgumentException here? (Same questions for NotifyIME and IMEState enums below.)

@@ +832,4 @@
>              ThreadUtils.assertOnGeckoThread();
>              Log.d(LOGTAG, "onSelectionChange(" + start + ", " + end + ")");
>          }
> +        // rabbit

rabbit? You can probably remove this. :)

Comment 4

[Eugen Sawin [:esawin]]

Thanks for the quick review!

> Is this crash a regression? Do we need to uplift this fix to Aurora or Beta?

This patch just fixes the debug output, not the crash. I'm still investigating the actual crash and the fixed debug output is helpful in this case. So I can't tell whether this is a regression yet.

What I found out so far is that we dispatch a NS_COMPOSITION_START event instead of NS_COMPOSITION_UPDATE when processing the second character input for a "freshly loaded" input form. Deleting and re-typing the input again without reloading doesn't show the same issue.
 
> ::: mobile/android/base/GeckoEditable.java
> @@ +105,5 @@
> > +            NONE(9999);
> > +
> > +            final int value;
> > +
> > +            Type(int value) {
> 
> Can you make the Type constructor private instead of package-visible? It
> looks like nothing prevents a caller from creating their own Type instance
> with an invalid value. (Same questions for NotifyIME and IMEState enums
> below.)

Yes, will do.


> @@ +115,5 @@
> > +                    if (t.value == value) {
> > +                        return t;
> > +                    }
> > +                }
> > +                return NONE;
> 
> Is NONE actually a valid value? We didn't have a NONE type before. Can we
> just throw an IllegalArgumentException here? (Same questions for NotifyIME
> and IMEState enums below.)

NONE is not a valid value, you're right we should better throw in this case.

Comment 5

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1134205-Fix-and-refactor-GeckoEditable-debug-out.patch

Comment 6

[Eugen Sawin [:esawin]]

What happens is that we keep losing focus during compositions, reset mIMEComposingText in [1], which results in the state getting out of sync at [2] and we therefore start a new composition in [3]. Since the composition is still around in IMEStateManager we hit the assertion in [4].

I'm not sure why we receive NOTIFY_IME_OF_BLUR events during a composition, is that the issue or is that expected behavior and we supposed to handle that situation robustly?

[1] https://dxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#2087
[2] https://dxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#1798
[3] https://dxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#1833
[4] https://dxr.mozilla.org/mozilla-central/source/dom/events/IMEStateManager.cpp#948

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Eugen Sawin [:esawin] from comment #6)
> I'm not sure why we receive NOTIFY_IME_OF_BLUR events during a composition,
> is that the issue or is that expected behavior and we supposed to handle
> that situation robustly?

Hmm, basically, REQUEST_TO_COMMIT or REQUEST_TO_CANCEL should be notified first... You might need to request to commit composition manually at receiving NOTIFY_IME_OF_BLUR.

On the other hand, if Gecko for Android should keep composition even while it's deactivated, use NOTIFY_DURING_DEACTIVE.
http://mxr.mozilla.org/mozilla-central/source/widget/nsIWidget.h#251

But when this is used, existing two or more compositions on different widgets isn't supported due to impossible.