[BUG]: App can apparently crash due to an incompatible IETF BCP-47 language ID #5046
Assignees
Labels
bug
End user-perceivable behaviors which are not desirable.
Impact: Medium
Moderate perceived user impact (non-blocking bugs and general improvements).
Work: High
It's not clear what the solution is.
Milestone
Comments
BenHenning
added
bug
|
@adhiamboperes could you look into this issue and see if you can come up with possible scenarios where it might repro & then gauge potential impact? |
github-project-automation
bot
moved this to Todo
in [Team] Core Learner and Mastery flows & UI Frontend - Android
|
Looked a little into this. I haven't fully created a situation in which it can occur, but I think it has something to do with the set system locale missing a country value (which is possible per https://developer.android.com/reference/java/util/Locale#getCountry()) and the app deciding that it must try to create a forced profile which, when using IETF BCP-47, expects a region code to be present when a region definition is provided per: This being null causes the crash since the last profile being resolved is forced per: So, it seems that a region definition must also be provided and no country present in the matched system locale in order for this situation to occur. More investigation is needed into how that's possible. |
adhiamboperes
added
Impact: Medium
How does the app handle language selection on region-locked devices? Example case: The stack trace is different for the crash, but it exposes a problem in the Locale handling logic. |
adhiamboperes
moved this from Todo
to In Progress
in [Team] Core Learner and Mastery flows & UI Frontend - Android
6 tasks
adhiamboperes
added a commit
that referenced
this issue
Mar 1, 2024
## Explanation Fixes #5093 This replaces #5211. The core crash has come from using Java 8's [Map.computeIfAbsent](https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#computeIfAbsent-K-java.util.function.Function-) on devices running SDK <24. This function isn't supported on these SDK versions because: 1. Java 8 isn't fully supported until SDK 24 (https://stackoverflow.com/a/35934691). 2. While the build toolchain "desugars" Java 8 syntax and some APIs to provide support for Java 8 before SDK 24, not all functionality is present. While https://developer.android.com/studio/write/java8-support-table suggests that ``computeIfAbsent`` _should_ be present, it's not clear why it isn't (it could be an out-of-date desugarer, or some other issue). Only two uses for ``computeIfAbsent`` are present: - ``AndroidLocaleFactory``: the main cause of the crash. - ``FakeAssetRepository``: only used in tests, so ignored in this change (since it would require changing the production API for ``AssetRepository`` to fix). A new regex pattern check was added to ensure this method can't inadvertently be used in the future since it won't work on lower API versions. This PR mainly focuses on fixing ``AndroidLocaleFactory``. Need to review the code & finish it, and document the changes. #5211 explored different methods that we could take to keep synchronization when updating the factory to move away from ``computeIfAbsent``, but all of them required introducing locking mechanisms which could cause deadlocking. I also considered in this PR removing the memoization altogether, but this doesn't seem ideal either since the creation of profiles is non-trivial and locales are frequently fetched by nearly all core lesson data providers throughout the app. I landed on a solution that leveraged the app's blocking dispatcher and made profile creation asynchronous. This has some specific implications: - Technically a "live lock" is still possible if the blocking dispatcher is starved. A better solution would be to use an actor-like pattern and funnel changes through the background dispatcher, but that's out of scope for this change and represents a problem with all uses of the blocking dispatcher. - Creations of ``DisplayLocaleImpl`` get more complicated since creation of the ``Locale`` is now asynchronous. This was adjusted by passing in the display locale's ``Locale`` object rather than having it compute it, and callsites often already are operating within a coroutine context which makes the asynchronous aspect of ``Locale`` creation a bit simpler. - Two cases where asynchronous creation cannot be used are the edge cases of initial app startup failing to fetching & create the locale in a timely manner, and initializing locale for all activities prior to the current locale being fetched. In both cases, the factory was updated to create a non-memoized ``Locale`` object specifically for these purposes. This method should only be used when necessary since it avoids the performance benefits of the memoized version. - #5046 might be addressed since one plausible cause for that observed issue is a root locale being used, or a region-only locale (both of which should now be handled per this PR). During development, there were two other changes that were made to ease development: - ``AndroidLocaleProfile`` was refactored to use a sealed class to improve its typing, and to better facilitate the introduction of a root profile (which was added for better system compatibility in cases when the default app locale isn't supported on the system). Note that the new root profile only applies to app strings since it can actually have correct default behavior (whereas for content strings & audio voiceovers it can't actually be used effectively). The profile class was also updated to compute its own ``Locale`` (which is a simplification since before there were multiple ways to create a "correct" ``Locale``), and have an application-injectable factory. - ``DataProviders.transformAsync`` was updated to handle caught exceptions in the same way as ``DataProviders.transform`` which helped while debugging the crash, and seems like it would have downstream benefits in the future. The method's tests were correspondingly updated. The changes here led to some testing changes: - ``testCreateDisplayLocaleImpl_defaultInstance_hasDefaultInstanceContext`` was removed since using the default context isn't valid in most cases (see below point). - ``testReconstituteDisplayLocale_defaultContext_returnsDisplayLocaleForContext`` was renamed & a new test added to better represent that the default context is invalid except for app strings where it can represent root locale. For non-app cases, an exception should be thrown for default. This is also why ``testCreateLocale_appStrings_allIncompat_invalidLangType_throwsException`` was updated to instead verify that the root locale is returned. - In ``TranslationControllerTest``, ``testGetSystemLanguageLocale_rootLocale_returnsLocaleWithBlankContext`` and ``testGetAppLanguageLocale_uninitialized_returnsLocaleWithSystemLanguage`` were updated to correctly indicate that there is no specified language for the root locale cases. To ensure coverage for valid IETF BCP-47 & Android resource language IDs, a new test was added: ``testGetAppLanguageLocale_ptBrDefLocale_returnsLocaleWithIetfAndAndroidResourcesLangIds``. Finally, please note that this is fixing a release blocking issue for the upcoming 0.13 beta release of the app. ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only This is mainly an infrastructure change. The only user behavior impacted is that the app no longer crashes on startup on SDK versions below 24. Attempting to open the app on an SDK 23 emulator on develop (4a07d8d):  Attempting to open the app with the fixes from this branch:  --------- Co-authored-by: Adhiambo Peres <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
Three devices in the latest release (beta 0.11 RC02) candidate's pre-launch report triggered the following stack trace:
fg.fXcorresponds toorg.oppia.android.app.model.LanguageSupportDefinition$LanguageIdbut unfortunately we don't have the actual stringified version of the proto since this is a production build.The crash happens due to a new check introduced in #5010, and seems to only happen when the primary & secondary languages fail to fallback. What's interesting is this seems to be happening for an IETF BCP-47 language configuration, and the PR above changed this behavior to actually potentially fail (whereas before it silently defaulted). I think it's quite likely this exception is simply exposing an existing breakage in the app.
To Reproduce
Unfortunately, we have no idea yet how to reproduce the issue. I've tried running a monkey test (per https://developer.android.com/studio/test/other-testing-tools/monkey) a few times, but haven't yet this particular crash.
Expected behavior
The app shouldn't crash for this case--it's an exceptional case that's not expected to be hit.
Demonstration
N/A
Environment
Note that each of these devices were running with a Swahili locale which I think is noteworthy given the crash is occurring within core internationalization logic. Unfortunately I don't have a sense of whether any Swahili devices passed (since the pre-launch report doesn't seem to include non-failures).
Additional context
Add any other context about the problem here.
The text was updated successfully, but these errors were encountered: