1330515 - Try to recover from IME errors in release builds

Status: RESOLVED FIXED

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

Description

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

Instead of throwing exceptions due to known or unknown IME bugs in release builds, we want to try to recover from the errors.

Comment 1

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

Attachment: 1. Add RELEASE_OR_BETA and NIGHTLY_BUILD flags to BuildConfig (v1)

RELEASE_OR_BETA and NIGHTLY_BUILD are based off of MOZILLA_VERSION,
which is already in BuildConfig, so I think it's okay to add the two
flags to BuildConfig.

Comment 2

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

Attachment: 2. Try to recover from IME errors (v1)

Instead of throwing exceptions in release builds, try to recover from
IME errors by flushing the entire text unless we already tried that
before. This prevents annoying crashes, and deals with known IME bugs
that are too risky to uplift to older releases.

Comment 3

[Eugen Sawin [:esawin]]

Comment on attachment 8826055 [details] [diff] [review]
2. Try to recover from IME errors (v1)

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoEditable.java
@@ +1149,5 @@
>              Log.e(LOGTAG, "invalid selection notification range: " +
>                    start + " to " + end + ", length: " + currentLength);
> +            if (!BuildConfig.RELEASE_OR_BETA) {
> +                throw new IllegalArgumentException("invalid selection notification range");
> +            }

You could move this out into a function to reduce redundancy.

Comment 4

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8826054 [details] [diff] [review]
1. Add RELEASE_OR_BETA and NIGHTLY_BUILD flags to BuildConfig (v1)

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

You'll need to bump https://dxr.mozilla.org/mozilla-central/source/mobile/android/geckoview/build.gradle#40 as well, since these aren't (yet) generated from a single source.

Are you really sure you need this?  These flags *make no sense* for GV, which is a library with (hopefully, in the future) many consumers.  In general I'm fine with whatever BuildConfig wrangling y'all need for GV to get it working, but you need to try to keep it small and to have it make sense for arbitrary consumers.  So I'll accept this with the expectation that we'll have to revert this later.

Comment 5

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

I think the flags here would make sense if GV tracks the Gecko platform releases. For example, people may build a stable GV library that tracks the release branch, or an experimental GV library that tracks the nightly branch. Or maybe we can provide our own "stable build" flag that doesn't use the release/beta/nightly wording.

But for this bug, after thinking about it more, I don't think we necessarily need these flags.

Comment 6

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

Attachment: Try to recover from IME errors (v2)

I think this is a better approach. Instead of not throwing the exception at
all, we catch it in native code. We then try flushing the text if this is the
first time we encounter the exception. Otherwise, we crash like before. Not
relying on the release build flag means the recovery code gets tested on
aurora/nightly, instead of leaving it untested until beta/release.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb6a2290e35d
Try to recover from IME errors; r=esawin

Comment 8

[Phil Ringnalda (:philor)]

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

Comment 9

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

Comment on attachment 8826301 [details] [diff] [review]
Try to recover from IME errors (v2)

Uplift request for 52.

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: More possible IME crashes due to known IME bugs that are too late or too risky to uplift.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not risky
[Why is the change risky/not risky?]: Change only impacts what happens when we are about to crash due to IME errors. Instead of crashing, we try to recover from the situation.
[String changes made/needed]: None

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8826301 [details] [diff] [review]
Try to recover from IME errors (v2)

I have some qualms about landing this on beta - we don't have a lot of time left to test (and to land more work and respin a build if we have to).  On the other hand, if the impact is only going to matter when we'd be crashing anyway, seems like a good bet!

We have to land on m-r also, because the beta to release merge happened yesterday. 

Jim, can you watch for possible crashes near the end of this week after we release 51 RC2?

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/11c4cd93ffc0

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Beta/Release 51.

Comment 13

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

Attachment: Patch for Beta (v1)

Comment 14

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bc44a8b30d11
https://hg.mozilla.org/releases/mozilla-release/rev/65c61eec146d

Comment 15

[u549602]

This issue is verified as fixed on a Nexus 6P (Android 7.0) and on a Samsung Galaxy Edge 6 (Android 6.0.1) by using Jim's steps:
"One way to test is 1) Go to http://telegra.ph 2) Enter a, b, c, then enter key 3) On my Nexus device, this will crash 51 beta but not 51 release."
I can confirm that this behavior is present on both devices(mentioned above), on Beta 15 the crash (https://crash-stats.mozilla.com/report/index/978ff5ac-3da8-4b4f-9ad6-53f642170125) is present but on 51.0 build 3, the crash can no longer be triggered.
This issue was also verified as fixed on 52.0b1 and 53.0a2.

Comment 16

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

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #10)
> Comment on attachment 8826301 [details] [diff] [review]
> Try to recover from IME errors (v2)
> 
> Jim, can you watch for possible crashes near the end of this week after we
> release 51 RC2?

I didn't see any crashes appearing.