Closed Bug 1330515 Opened 3 years ago Closed 3 years ago

Try to recover from IME errors in release builds

Categories

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

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox51 --- verified
firefox52 --- verified
firefox53 --- verified

People

(Reporter: jchen, Assigned: jchen)

Details

Attachments

(2 files, 2 obsolete files)

Instead of throwing exceptions due to known or unknown IME bugs in release builds, we want to try to recover from the errors.
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.
Attachment #8826054 - Flags: review?(nalexander)
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.
Attachment #8826055 - Flags: review?(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.
Attachment #8826055 - Flags: review?(esawin) → review+
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.
Attachment #8826054 - Flags: review?(nalexander) → review+
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.
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.
Attachment #8826301 - Flags: review?(esawin)
Attachment #8826054 - Attachment is obsolete: true
Attachment #8826055 - Attachment is obsolete: true
Attachment #8826301 - Flags: review?(esawin) → review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb6a2290e35d
Try to recover from IME errors; r=esawin
https://hg.mozilla.org/mozilla-central/rev/fb6a2290e35d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
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
Attachment #8826301 - Flags: approval-mozilla-beta?
Attachment #8826301 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(nchen)
Attachment #8826301 - Flags: approval-mozilla-release+
Attachment #8826301 - Flags: approval-mozilla-beta?
Attachment #8826301 - Flags: approval-mozilla-beta+
Attachment #8826301 - Flags: approval-mozilla-aurora?
Attachment #8826301 - Flags: approval-mozilla-aurora+
Needs rebasing for Beta/Release 51.
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.
(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.
Flags: needinfo?(nchen)
You need to log in before you can comment on or make changes to this bug.