Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG]:Oppia-Android app not opening on the Android 6.0.1 Marshmello #5093

Closed
prafulbbandre opened this issue Jul 18, 2023 · 8 comments · Fixed by #5291
Closed

[BUG]:Oppia-Android app not opening on the Android 6.0.1 Marshmello #5093

prafulbbandre opened this issue Jul 18, 2023 · 8 comments · Fixed by #5291
Assignees
Labels
bug End user-perceivable behaviors which are not desirable. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Low Solution is clear and broken into good-first-issue-sized chunks.

Comments

@prafulbbandre
Copy link
Contributor

prafulbbandre commented Jul 18, 2023

Describe the bug

I setup the oppia android project locally and deployed the app to my android device (6.0.1) but as i open the app it closes on its own.
But the app is working fine on higher specs devices.

Steps To Reproduce

setup oppia-android locally
deploy the app on a abdroid 6.0.1 device

Expected Behavior

It should work fine for the android 6.0.1 devices.

Screenshots/Videos

Screenrecording_20230717_162128.mp4

Stacktrace

E/AndroidRuntime: FATAL EXCEPTION: main
    Process: org.oppia.android, PID: 1041
    java.lang.NoClassDefFoundError: org.oppia.android.util.locale.AndroidLocaleFactory$createAndroidLocale$1
        at org.oppia.android.util.locale.AndroidLocaleFactory.createAndroidLocale(AndroidLocaleFactory.kt:59)
        at org.oppia.android.util.locale.DisplayLocaleImpl$formattingLocale$2.invoke(DisplayLocaleImpl.kt:25)
        at org.oppia.android.util.locale.DisplayLocaleImpl$formattingLocale$2.invoke(DisplayLocaleImpl.kt:17)
        at kotlin.SynchronizedLazyImpl.getValue(LazyJVM.kt:74)
        at org.oppia.android.util.locale.DisplayLocaleImpl.getFormattingLocale(DisplayLocaleImpl.kt)
        at org.oppia.android.util.locale.DisplayLocaleImpl.setAsDefault(DisplayLocaleImpl.kt:46)
        at org.oppia.android.domain.locale.LocaleController.setAsDefault(LocaleController.kt:239)
        at org.oppia.android.app.translation.ActivityLanguageLocaleHandler.initializeLocaleForActivity(ActivityLanguageLocaleHandler.kt:41)
        at org.oppia.android.app.activity.InjectableAppCompatActivity.onInitializeLocalization(InjectableAppCompatActivity.kt:77)
        at org.oppia.android.app.activity.InjectableAppCompatActivity.attachBaseContext(InjectableAppCompatActivity.kt:33)
        at android.app.Activity.attach(Activity.java:6288)
        at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2443)
        at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2577)
        at android.app.ActivityThread.access$1000(ActivityThread.java:166)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1414)
        at android.os.Handler.dispatchMessage(Handler.java:102)
        at android.os.Looper.loop(Looper.java:148)
        at android.app.ActivityThread.main(ActivityThread.java:5628)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:853)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:737)

What device/emulator are you using?

Vivo y55s

Which Android version is your device/emulator running?

Android 6.0.1

Which version of the Oppia Android app are you using?

App version 1.0

Additional Context

No response

@prafulbbandre prafulbbandre added bug End user-perceivable behaviors which are not desirable. triage needed labels Jul 18, 2023
@adhiamboperes adhiamboperes added Impact: High High perceived user impact (breaks a critical feature or blocks a release). Work: High It's not clear what the solution is. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). and removed Impact: High High perceived user impact (breaks a critical feature or blocks a release). labels Jul 26, 2023
@Shaik-Sirajuddin
Copy link

Hi @adhiamboperes
I would like to work on this issue.
I see this issue is due to the use of computeIfAbsent() method of ConcurrentHashMap . The method was introduced in API level 24 , which ensures atomicity across multiple threads calling into the function . But Android 6.0.1 is API level 23

Here is the exact code block of this :

memoizedLocales.computeIfAbsent(localeContext) {
       val chooser = profileChooserSelector.findBestChooser(localeContext  )
       val primaryLocaleSource = LocaleSource.createFromPrimary(localeContext)
       val fallbackLocaleSource = LocaleSource.createFromFallback(localeContext)
       val proposal = chooser.findBestProposal(primaryLocaleSource, fallbackLocaleSource)
       return@computeIfAbsent proposal.computedLocale
     }

By using this method , the code ensures that the computation is only performed once when called by multiple threads , this is done only once by acquiring a lock internally by computeIfAbsent method.

To fix this issue I am thinking of using getOrPut for Api level's below 24.

@adhiamboperes
Copy link
Collaborator

Hi @adhiamboperes I would like to work on this issue. I see this issue is due to the use of computeIfAbsent() method of ConcurrentHashMap . The method was introduced in API level 24 , which ensures atomicity across multiple threads calling into the function . But Android 6.0.1 is API level 23

Here is the exact code block of this :

memoizedLocales.computeIfAbsent(localeContext) {
       val chooser = profileChooserSelector.findBestChooser(localeContext  )
       val primaryLocaleSource = LocaleSource.createFromPrimary(localeContext)
       val fallbackLocaleSource = LocaleSource.createFromFallback(localeContext)
       val proposal = chooser.findBestProposal(primaryLocaleSource, fallbackLocaleSource)
       return@computeIfAbsent proposal.computedLocale
     }

By using this method , the code ensures that the computation is only performed once when called by multiple threads , this is done only once by acquiring a lock internally by computeIfAbsent method.

To fix this issue I am thinking of using getOrPut for Api level's below 24.

Can you reproduce this issue if you use an API level 23 emulator?

@Shaik-Sirajuddin
Copy link

Shaik-Sirajuddin commented Oct 28, 2023

Yes @adhiamboperes , Was able to debug the issue by launching the app on emulator ( Android version 6.0 )
App crashes on launch.

@adhiamboperes
Copy link
Collaborator

Sure, I'm assigning the issue to you. In your PR, please include screenshot/video of before and after showing the bug and the fix as reproduced by you.

@adhiamboperes adhiamboperes added Work: Low Solution is clear and broken into good-first-issue-sized chunks. and removed Work: High It's not clear what the solution is. labels Oct 28, 2023
@Shaik-Sirajuddin
Copy link

Thanks , I will ensure that.

Sure, I'm assigning the issue to you. In your PR, please include screenshot/video of before and after showing the bug and the fix as reproduced by you.

@BenHenning
Copy link
Member

BenHenning commented Oct 31, 2023

Keeping this as a 0.12 blocker for best-effort fixing, but currently M users make up just 0.7% of beta app usage.

Edit: It occurred to me that this might be a lower-than-correct representation since the app not being able to open will significantly de-bias data.

@seanlip
Copy link
Member

seanlip commented Nov 4, 2023

Following up from an earlier discussion with @BenHenning -- just confirming that the 1-star feedback we got on the beta version a couple of weeks ago, saying that "the application does not open", is indeed correlated to Marshmallow (Android 6.0, SDK 23).

@BenHenning
Copy link
Member

NB we decided to finalize the 0.12 release and release a new 0.13 rather than hotfix 0.12 (since it's now been out for a month).

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):


![open_app_without_fix_smaller](https://github.com/oppia/oppia-android/assets/12983742/458b2baf-4157-4a3f-9809-27368a9c3e04)

Attempting to open the app with the fixes from this branch:

![open_app_with_fix_smaller](https://github.com/oppia/oppia-android/assets/12983742/b9ce13d9-ecfd-4205-b87f-6fbf42dfd995)

---------

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
Labels
bug End user-perceivable behaviors which are not desirable. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Low Solution is clear and broken into good-first-issue-sized chunks.
5 participants