[Reader] Fix IllegalStateException in ReaderPostListFragment #20507
Conversation
|
 |
App Name |  WordPress |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr20507-88e9cab | |
| Commit | 88e9cab | |
| Direct Download | wordpress-prototype-build-pr20507-88e9cab.apk |
|
 |
App Name |  Jetpack |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr20507-88e9cab | |
| Commit | 88e9cab | |
| Direct Download | jetpack-prototype-build-pr20507-88e9cab.apk |
| @@ -745,6 +745,9 @@ public void onPause() { | |||
| @Override | |||
| public void onResume() { | |||
| super.onResume(); | |||
| if (!isAdded() || getView() == null) { | |||
There was a problem hiding this comment.
I don't think this will help though, it looks like the line that's crashing is line 2322 here, according to the stack trace of the crash:
java.lang.IllegalStateException: Fragment ReaderPostListFragment{b2c45df} (12cf4b19-e069-4888-ab5a-911d5ed99a51) not attached to an activity.
at androidx.fragment.app.Fragment.requireActivity(Fragment.java:1000)
at org.wordpress.android.ui.reader.ReaderPostListFragment$11.run(ReaderPostListFragment.java:2322)
If you look a couple of lines above the crashing line you will see that a similar check is already being made and even then the crash happened to a few users. I guess that, since the code is being run asynchronously, it might have been delayed enough that when line 2322 is reached the Fragment is no longer attached.
If that's indeed the case, we probably shouldn't be using requireActivity there, but instead just get the activity and only run the code if it is not null, since being null should mean that Fragment is no longer attached so that update is no longer needed anyway. Wdyt?
There was a problem hiding this comment.
Thank you for your suggestions 🙇 . I've made the adjustments as recommended, although it resulted in about 4 isAdded checks and 2 getActivity() != null conditions within a few lines of code. 😵💫
Side note: I believe the bottleneck isn't the shouldAutoUpdateTag function itself, but rather the delay in starting the thread, likely due to many other threads running concurrently to update the Reader in the background.
Regardless, we should have significantly reduced the likelihood of the crash occurring now.
…urrentTag Thread starts.
| @@ -2320,10 +2321,15 @@ private void updateCurrentTagIfTime() { | |||
| new Thread() { | |||
| @Override | |||
| public void run() { | |||
| // Check the fragment is attached to the activity when this Thread starts. | |||
| FragmentActivity activity = getActivity(); | |||
| if (activity == null) { | |||
There was a problem hiding this comment.
💡 I think doing the check here still has a great chance of not solving the crash, since the function that probably takes the most time to run is shouldAutoUpdateTag. This means that even with this check here, when the code reaches line 2330 or 2332 the fragment might already be dettached anyway, which will likely yield other issues.
Because of that, I think it would be better if the check is done in each of the if clause's blocks (before line 2330 and line 2332) below.
I know the code will look a bit repetitive, but since we're working with Java here, there's not much way around it. 😞
… the code that actually use the activity
|
daniloercoli
deleted the
issue/20228-Illegal-state-Exception---ReaderPostListFragment
branch
Fixes #20228
I was unable to reproduce the problem, and the occurrences of it (JETPACK-ANDROID-JMJ) are very low. Ensuring that the Fragment is still attached to the Activity in onResume should fix the problem.
To Test:
PR Submission Checklist:
RELEASE-NOTES.txtif necessary.Testing Checklist (strike-out the not-applying and unnecessary ones):