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

Fix crash on Samsung Android 10 that call this during parent init. #62

Merged
merged 1 commit into from
Nov 5, 2019

Conversation

Tolriq
Copy link
Contributor

@Tolriq Tolriq commented Nov 3, 2019

As per title.

@zhanghai
Copy link
Owner

zhanghai commented Nov 5, 2019

Can you move the check to the beginning of getProgressTintList() and add a comment saying:

// Samsung Android 10 might call this in super class constructor.

?

Meanwhile, did you confirm if this is the only method that's called in super class constructor? Or, can you confirm that this is enough to fix crash on Samsung devices? I cann't test for myself because I don't own a Samsung device.

@Tolriq
Copy link
Contributor Author

Tolriq commented Nov 5, 2019

I can confirm this fix the issue for my users, I can't reproduce as no S10 either and strangely the crash data never reached Play Console so had to ask for users full bug Reports, maybe because it's still a beta build of Android.

Anyway PR updated.

@zhanghai zhanghai merged commit 1005cd3 into zhanghai:master Nov 5, 2019
@zhanghai
Copy link
Owner

zhanghai commented Nov 5, 2019

Thanks!

@Tolriq
Copy link
Contributor Author

Tolriq commented Nov 5, 2019

Any chance to have a 1.3.3 release so I can remove my custom aar ? :)

@zhanghai
Copy link
Owner

zhanghai commented Nov 7, 2019

Planning to do this tomorrow.

@zhanghai
Copy link
Owner

zhanghai commented Nov 8, 2019

Released in v1.3.3.

@Vibragimov
Copy link

@zhanghai @Tolriq This is not a good solution, by code mProgressTintInfo can never be null. Perhaps Samsung is trying to somehow influence through reflection on this field for its OS shell. Look at the ProgressBar source code, it has exactly the same field. I suggest renaming this field so that the UI displays user/library colors instead of Samsung

@Tolriq
Copy link
Contributor Author

Tolriq commented Nov 8, 2019

@Vibragimov this is the correct solution and best of all it works, learn about initialization phases in Java when calling super constructors in child constructors :)

Samsung is just trying to get current tintlist during init phase probably because they are using some stupid hacks to implement their dark mode that is a little different than stock Android, but this does not change anything about the fact that the library will have the proper values applied later on for the rendering.

@zhanghai
Copy link
Owner

zhanghai commented Nov 9, 2019

I agree with @Tolriq's analysis. It doesn't make sense for Samsung to use reflection, because they can modify any framework code easily. Meanwhile, this fix did fix the issue according to his testing.

@yccheok
Copy link

yccheok commented Jan 2, 2020

I have an exact same experience.

  • The crash is not reported in Google Play Console's ANR & crashes
  • The crash is reported in Firebase Crashlytics.

Since, we just install Firebase Crashlytics few weeks back, we are not sure, whether this issue has occured since the early day, or since Samsung Galaxy S10 has performed some software update (around November time frame?!)

We have another high number of crashes (Not related to MaterialRatingBar) these day around December time frame, for Samsung Galaxy S10 - https://issuetracker.google.com/issues/147020028

I was wondering, does Samsung Galaxy S10 introduces some buggy software code update recently, around November time frame?

@yccheok
Copy link

yccheok commented Jan 2, 2020

I can confirm this fix the issue for my users, I can't reproduce as no S10 either and strangely the crash data never reached Play Console so had to ask for users full bug Reports, maybe because it's still a beta build of Android.

Anyway PR updated.

Hi Tolriq,

Did you see this crash at Samsung Galaxy S10 Android 10 only?

Did you see it in Samsung Galaxy S10 Android 9?

As, I saw you reported this problem in early of Nov.

But, from Internet news, it seems that Samsung only push Android 9 to Android 10 update to users, around December time frame.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants