1236643 - Revisit IME shutdown sequence on nsWindow closing

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

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

Bug 1211704 tried to fix some of the bugs related to IME shutdown, but it's incomplete. In particular, after nsWindow closes and GeckoEditable shuts down, GeckoView can be left in an indeterminate state that can lead to crashes.

Comment 1

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

Attachment: Reorder GeckoEditable destruction sequence (v1)

To guarantee that GeckoInputConnection and GeckoEditable are not used by
GeckoView after GeckoEditable has been destroyed, we need to make sure a
certain sequence is followed. We should first unset the
InputConnectionListener in GeckoView on the UI thread; then unset the
GeckoEditableListener on the IC thread; and finally finish destroying
the GeckoEditable instance through disposeNative. This patch merges this
logic with the initialization logic in GeckoEditable.onViewChange, so
that onViewChange can be used for both initialization and destruction.

Comment 2

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

Attachment: Let GeckoInputConnection keep its view (v1)

Right now GeckoInputConnection uses GeckoAppShell to get its view, but
it should be keeping its own View reference to support multiple
GeckoViews.

Comment 3

[Eugen Sawin [:esawin]]

Comment on attachment 8703774 [details] [diff] [review]
Reorder GeckoEditable destruction sequence (v1)

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

::: mobile/android/base/java/org/mozilla/gecko/GeckoEditable.java
@@ +420,5 @@
>                      Log.d(LOGTAG, "onViewChange (set listener)");
>                  }
> +
> +                if (newListener != null) {
> +                    // Make sure there are no other things going on

.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef20ed15509b
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7af5278a657

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ef20ed15509b
https://hg.mozilla.org/mozilla-central/rev/a7af5278a657

Comment 6

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

Comment on attachment 8703774 [details] [diff] [review]
Reorder GeckoEditable destruction sequence (v1)

Requesting uplift for both patches. (Asking for Aurora now; I think Beta may be affected but I'm not sure of the impact at the moment. We should uplift to Beta if we see lots of shutdown/restart crashes.)

Approval Request Comment
[Feature/regressing bug #]: Bug 1051556
[User impact if declined]: Possible crash when shutting down / restarting Fennec (e.g. after installing an addon)
[Describe test coverage new/current, TreeHerder]: Locally, m-c
[Risks and why]: Small. There is some risk in introducing new code, but it only affects shutdown, so the overall risk is limited.
[String/UUID change made/needed]: None

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8703774 [details] [diff] [review]
Reorder GeckoEditable destruction sequence (v1)

Fix some potential crashes, taking it.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b0fbc87bf361
https://hg.mozilla.org/releases/mozilla-aurora/rev/af3852b5e369

Comment 9

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: Crash regression on 44

Comment 10

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

Attachment: Patch for Beta

Approval Request Comment
[Feature/regressing bug #]: Bug 1051556
[User impact if declined]: Top-crash for beta
[Describe test coverage new/current, TreeHerder]: Locally, m-c
[Risks and why]: Small; patch only affects shutdown path so impact is limited
[String/UUID change made/needed]: None

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Jim, Kevin: Do we have data from Aurora/Nightly that supports the fact that these patches do fix these crashes? Given the size of the patches here, I am being extra cautious and want to make sure we are not making rushed decisions. Thoughts?

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8708016 [details] [diff] [review]
Patch for Beta

As such the size of this patches gives me a lot of anxiety but I trust Margaret's judgement on the risk involved. Let's uplift to Beta44 with the hopes that this helps improve Fennec stability.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/db934cef3dff

Comment 14

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

I haven't looked at the overall numbers. I did look at a few Beta crash reports, and this patch will definitely fix some of them, or even most of them (the quality of the crash reports is not good enough to say for sure).

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Jim Chen [:jchen] [:darchons] from comment #14)
> I haven't looked at the overall numbers. I did look at a few Beta crash
> reports, and this patch will definitely fix some of them, or even most of
> them (the quality of the crash reports is not good enough to say for sure).

Great! Thanks for the prompt reply and I am really glad we are taking a fix that is already showing good signs of fixing the crash.

Comment 16

[Kevin Brosnan [Ex-Mozilla]]

This signature does not appear on 46.0a1 or 45.0a2 after the fix was applied.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Kevin Brosnan [:kbrosnan] from comment #16)
> This signature does not appear on 46.0a1 or 45.0a2 after the fix was applied.

Nice! Thanks Kevin.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/db934cef3dff

Comment 19

[u549602]

Could not verify this since no crashes were encountered in our today's test session.
Will verify after more data from the crash stats is gathered.