Closed Bug 1250314 Opened 4 years ago Closed 4 years ago

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

Categories

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

Unspecified
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 2 obsolete files)

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.
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.
Attachment #8722201 - Flags: review?(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.
Attachment #8722201 - Flags: review?(esawin) → review-
(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 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.
Attachment #8722201 - Flags: review- → review+
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.
Attachment #8723388 - Flags: review?(esawin)
Attachment #8722201 - Attachment is obsolete: true
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
Attachment #8723388 - Flags: review?(esawin) → review+
Attachment #8723388 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/e29963221449
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.