1250314 - crash in java.lang.IllegalStateException: composition not cancelled at org.mozilla.gecko.GeckoEditable.notifyCancelComposition(GeckoEditable.java)

Status: RESOLVED FIXED

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

Description

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

This bug was filed from the Socorro interface and is 
report bp-b0f14ccb-f627-4513-a6c5-1e5ae2160218.
=============================================================

Side effect of the diagnostic patch in bug 1248459.

Comment 1

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

Attachment: Flush changes when committing or canceling composition (v1)

Flush IME changes when committing or canceling the composition so that
the Gecko and Java sides are on the same page.

Also, use the GeckoEditableListener constants when calling notifyIME so
we don't rely on the Gecko platform constants having the same values as
our Java constants.

Comment 2

[Eugen Sawin [:esawin]]

Comment on attachment 8722201 [details] [diff] [review]
Flush changes when committing or canceling composition (v1)

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

::: widget/android/nsWindow.cpp
@@ +2787,5 @@
> +
> +            // Keep a strong reference to the window to keep 'this' alive.
> +            RefPtr<nsWindow> window(&this->window);
> +
> +            nsAppShell::PostEvent([this, window] {

I don't think it's safe to capture refcounted variables by a lambda.

@@ +2809,2 @@
>  
> +            nsAppShell::PostEvent([this, window] {

See above.

Comment 3

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

(In reply to Eugen Sawin [:esawin] from comment #2)
> Comment on attachment 8722201 [details] [diff] [review]
> Flush changes when committing or canceling composition (v1)
> 
> Review of attachment 8722201 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/android/nsWindow.cpp
> @@ +2787,5 @@
> > +
> > +            // Keep a strong reference to the window to keep 'this' alive.
> > +            RefPtr<nsWindow> window(&this->window);
> > +
> > +            nsAppShell::PostEvent([this, window] {
> 
> I don't think it's safe to capture refcounted variables by a lambda.
> 
Why's that? If you're thinking of [1], it's talking about capturing a raw pointer, which indeed is dangerous. But capturing a smart pointer should be okay.

[1] https://groups.google.com/d/topic/mozilla.dev.platform/Ec2y6BWKrbM/discussion

Comment 4

[Eugen Sawin [:esawin]]

Comment on attachment 8722201 [details] [diff] [review]
Flush changes when committing or canceling composition (v1)

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

After further discussion on IRC, I agree that capturing a RefPtr by value should be safe.

Comment 5

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

Attachment: Let changes flush when committing or canceling composition (v2)

Turns out it's not good to commit or cancel the composition asynchronously. The
compromise then is to commit or cancel synchronously, but notify the Java side
asynchronously.

Comment 6

[Eugen Sawin [:esawin]]

Comment on attachment 8723388 [details] [diff] [review]
Let changes flush when committing or canceling composition (v2)

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

::: widget/android/nsWindow.cpp
@@ +2802,5 @@
> +                    return;
> +                }
> +
> +                mEditable->NotifyIME(editableNotification);
> +            });

This is a generic procedure, we could move it into its own function, which would improve code readability by providing a new "verb" (async-notify) and the cancel/commit case handling could be handled separately as before without code duplication

Comment 7

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

Attachment: Let changes flush when committing or canceling composition (v2.1)

Added AsyncNotifyIME function.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e29963221449

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e29963221449