Fix #3504 and #3669: Fixed app crashes in API 19 #3723

rt4914 · 2021-08-24T16:35:01Z

Explanation

Fixes #3504 : App crash fixed for API 19.
Fixes #3669: App crash fixed for API 19.

There were two errors occurring on API 19:

Error 1

On launch of app: java.lang.exceptionininitializererror which was pointing at this.

Solution 1

As per this the solution was to downgrade the okhttp based libraries from 4.x.x to 3.x.x.

Error 2

On click on any profile in ProfileChooser app was crashing. It was because of use of Optional here

Solution 2

Get rid of optional as the code can work without that too.
The developer options is working correctly.

Note

I haven't tested the app beyond these screens. So this PR only solves these errors but there should be a full audit on API 19 so that if there are any errors/crashes on any other screen too, we can identify them.

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The PR follows the style guide.
The PR does not contain any unnecessary auto-generated code from Android Studio.
The PR is made from a branch that's not called "develop".
The PR is made from a branch that is up-to-date with "develop".
The PR's branch is based on "develop" and not on any other branch.
The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

rt4914 · 2021-08-24T18:04:40Z

third_party/maven_install.json

            "androidx.core:core:1.0.1": "androidx.core:core:1.3.0",
            "androidx.recyclerview:recyclerview:1.0.0": "androidx.recyclerview:recyclerview:1.1.0",
            "androidx.test:core:1.0.0": "androidx.test:core:1.2.0",
+            "com.squareup.okhttp3:okhttp:3.12.12": "com.squareup.okhttp3:okhttp:4.1.0",


@BenHenning After downgrading the deps I ran the
REPIN=1 bazel run @unpinned_maven//:pin which was successful but now its showing 2 versions of same library.
I think this might be because of Retrofit library.

This seems really problematic. We need to be able to update to newer versions otherwise we might lose out on security patches.

Did OkHttp officially drop support for pre-SDK 21?

square/okhttp#4481 paints a pretty clear picture here. We might need to find a different HTTP library to use instead of OkHttp since their <21 compatibility store isn't great. Retrofit will probably force us to use a newer version of OkHttp.

I'm not sure on next steps yet. Retrofit seems to only support OkHttp, and we would either need to backport Retrofit support (limiting our update choices) or find an alternative. For alternatives, I've looked into gRPC (which would be better long-term, anyway), but App Engine doesn't currently support gRPC (or rather, its load balancer will block gRPC traffic).

We could potentially compile out the data module to remove the dependency entirely but that requires shipping the app with Bazel and we can't do that until the app bundles portion of #2432 is addressed. I think that's currently in progress, but probably won't be ready for a few more weeks. We could look into manually building the AAB temporarily, but per Android developer documentation that's probably going to be difficult with databinding (can't be sure until we try, but it requires manually running aapt2 to build out resources).

Finally, we could manually force the data module out of the app via Gradle but that may result in a potential risk to regress existing tests that expect everything to hook-up correctly.

I actually think our hands might be tied here--I don't see an obvious way forward that may not result in a large time commitment to make this work.

Update: Per #2432 (comment) if we can build AABs via Bazel then we can isolate the data module for SDK 19 devices. It doesn't solve the long-term solution, but it might be a viable workaround to at least unblock the immediate need to have SDK 19 support in Alpha MR3.

rt4914 · 2021-08-24T18:07:58Z

@BenHenning I am unable to fix the test failures for robolectric and build binary for bazel.

I am getting following error when executing this on gradle: ./gradlew clean assembleDebug

Can you please help in this? If this fix is urgent, you can contribute on this PR directly.

BenHenning

Thanks for the quick PR @rt4914. Had a few comments--PTAL.

/cc @isalooo since this was something we recently discussed.

BenHenning · 2021-08-24T19:10:36Z

app/src/main/java/org/oppia/android/app/drawer/NavigationDrawerFragmentPresenter.kt

 import org.oppia.android.util.data.AsyncResult
 import org.oppia.android.util.data.DataProviders.Companion.toLiveData
 import org.oppia.android.util.statusbar.StatusBarColor
-import java.util.Optional


Can you add a regex pattern check + test in our new regex static analysis check to ensure this can't regress anywhere in the app?

BenHenning · 2021-08-24T19:11:39Z

third_party/maven_install.json

            "androidx.core:core:1.0.1": "androidx.core:core:1.3.0",
            "androidx.recyclerview:recyclerview:1.0.0": "androidx.recyclerview:recyclerview:1.1.0",
            "androidx.test:core:1.0.0": "androidx.test:core:1.2.0",
+            "com.squareup.okhttp3:okhttp:3.12.12": "com.squareup.okhttp3:okhttp:4.1.0",


This seems really problematic. We need to be able to update to newer versions otherwise we might lose out on security patches.

Did OkHttp officially drop support for pre-SDK 21?

BenHenning · 2021-08-24T19:12:37Z

app/src/main/java/org/oppia/android/app/drawer/NavigationDrawerFragmentPresenter.kt

  private val oppiaLogger: OppiaLogger,
  private val headerViewModelProvider: ViewModelProvider<NavigationDrawerHeaderViewModel>,
  private val footerViewModelProvider: ViewModelProvider<NavigationDrawerFooterViewModel>,
-  private val developerOptionsStarter: Optional<DeveloperOptionsStarter>


Won't this cause build errors if in prod builds of the app? I'm fairly certain we need to keep this.

Can you try using the Guava Optional, instead, to see if it builds? That's a drop-in replacement in Dagger for Java 8 Optional.

oppiabot · 2021-08-24T19:16:07Z

Unassigning @BenHenning since the review is done.

oppiabot · 2021-08-24T19:16:10Z

Hi @rt4914, it looks like some changes were requested on this pull request by @BenHenning. PTAL. Thanks!

BenHenning · 2021-08-24T19:54:04Z

@BenHenning I am unable to fix the test failures for robolectric and build binary for bazel.

I am getting following error when executing this on gradle: ./gradlew clean assembleDebug

Can you please help in this? If this fix is urgent, you can contribute on this PR directly.

The screenshot is missing a lot of details--I can't help based on this information alone. Can you provide the full stacktrace?

I'm also wondering if this fix will actually be sufficient per my other comment. If we're still pulling in newer OkHttp due to Retrofit, this might not actually fix the underlying issue.

BenHenning · 2021-08-25T16:19:19Z

Other idea: we might be able to just remove the OkHttp client construction in NetworkModule locally, but it's not clear how much this should bubble up.

oppiabot · 2021-09-01T16:39:28Z

Hi @rt4914, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 3 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

anandwana001 · 2021-09-06T10:53:36Z

@rt4914 Is this solving the Bazel APK or this is just for the Gradle?

oppiabot · 2021-09-13T11:47:07Z

Hi @rt4914, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 3 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

Fixed crashes in API 19

55174b1

rt4914 requested a review from BenHenning as a code owner August 24, 2021 16:35

rt4914 changed the title ~~Fixed crashes in API 19~~ Fix #3504: Fixed app crashes in API 19 Aug 24, 2021

rt4914 changed the title ~~Fix #3504: Fixed app crashes in API 19~~ Fix #3504 and #3669: Fixed app crashes in API 19 Aug 24, 2021

rt4914 added 2 commits August 24, 2021 22:16

Nit fixed

3b48320

Fixed data layer tests

5e965ba

rt4914 commented Aug 24, 2021

View reviewed changes

rt4914 assigned BenHenning Aug 24, 2021

BenHenning requested changes Aug 24, 2021

View reviewed changes

oppiabot bot unassigned BenHenning Aug 24, 2021

oppiabot bot assigned rt4914 Aug 24, 2021

BenHenning added this to the Alpha MR3 milestone Aug 25, 2021

oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Sep 1, 2021

oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Sep 6, 2021

oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Sep 13, 2021

oppiabot bot closed this Sep 20, 2021

rt4914 reopened this Sep 28, 2021

oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Sep 28, 2021

rt4914 closed this Sep 28, 2021

rt4914 deleted the fix-api-19-crash branch September 28, 2021 13:44

BenHenning mentioned this pull request Oct 7, 2021

Fix #3803, #3804, #3853, #3669, #3383: Add basic KitKat support in Bazel #3910

Merged

6 tasks

Fix #3504 and #3669: Fixed app crashes in API 19 #3723

Fix #3504 and #3669: Fixed app crashes in API 19 #3723

Uh oh!

Conversation

rt4914 commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

Error 1

Solution 1

Error 2

Solution 2

Note

Checklist

Uh oh!

rt4914 Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

BenHenning Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

BenHenning Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

BenHenning Aug 25, 2021

Choose a reason for hiding this comment

Uh oh!

rt4914 commented Aug 24, 2021

Uh oh!

BenHenning left a comment

Choose a reason for hiding this comment

Uh oh!

BenHenning Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

BenHenning Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

BenHenning Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

oppiabot bot commented Aug 24, 2021

Uh oh!

oppiabot bot commented Aug 24, 2021

Uh oh!

BenHenning commented Aug 24, 2021

Uh oh!

BenHenning commented Aug 25, 2021

Uh oh!

oppiabot bot commented Sep 1, 2021

Uh oh!

anandwana001 commented Sep 6, 2021

Uh oh!

oppiabot bot commented Sep 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rt4914 commented Aug 24, 2021 •

edited

Loading