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

Misuse of AsyncTask #3051

Open
pangeneral opened this issue Aug 13, 2019 · 2 comments · May be fixed by #3058
Open

Misuse of AsyncTask #3051

pangeneral opened this issue Aug 13, 2019 · 2 comments · May be fixed by #3058
Labels
Bug This is an issue with Slide. This is where something in the app isn't working as intended. Good First Issue This issue is perfect for you if you are new to Slide. Has Solution There is a solution to this issue posted in the comment.

Comments

@pangeneral
Copy link

According to our research, there are misuses about the following classes:

  1. me.ccrama.redditslide.Vote
  2. me.ccrama.redditslide.Activities.CommentsScreenSingle.AsyncGetSubredditName
  3. me.ccrama.redditslide.Activities.Inbox
  4. me.ccrama.redditslide.Activities.LiveThread

The problems are:

  1. There are many annoymous inner AsyncTask classes in LiveThread and Inbox. They hold strong reference to the GUI element of Activity, which can lead to memory leak when the Activity is destroyed and the AsyncTask did not finish. Similar problem also exists in Vote and AsyncGetSubredditName.
  2. The instances of these AsyncTask classes are not cancelled before the Activity is destroyed, which can lead to the wrong invocation of onPostExecute

I think we can make following changes to fix the misuse problems:

  1. I think the GUI-related fields should be wrapped into WeakReference. Take private View v in Vote as example, it can be changed tp private WeakReference<view> v. Besides, use non-anonymous inner static classes to replace these annoymous classes.
  2. Add a AsyncTask field in the corresponding Activities which use AsyncTask. These field refer to the annoymous AsyncTask object. Then invoke cancel() in the onDestroy() method of Activities.

These are my suggestions above, thank you.

@Alexendoo Alexendoo added Bug This is an issue with Slide. This is where something in the app isn't working as intended. Good First Issue This issue is perfect for you if you are new to Slide. labels Aug 21, 2019
@Tensounder54 Tensounder54 added the High Priority This issue is high on our todo list. It needs to be fixed as soon as possible. label Aug 23, 2019
@hassan963
Copy link

Following changes may need to solve this issue, following @pangeneral advices

  1. For anonymous inner AsyncTask classes in LiveThread and Inbox , make separate AsyncTask classes for them and call them from LiveThread and Inbox. By this we can make them static class.
  2. Call cancel() in onPause() method in Activity.

I am working on this issue, please let me know if this is the correct way. Thank you.

@pangeneral
Copy link
Author

Following changes may need to solve this issue, following @pangeneral advices

  1. For anonymous inner AsyncTask classes in LiveThread and Inbox , make separate AsyncTask classes for them and call them from LiveThread and Inbox. By this we can make them static class.
  2. Call cancel() in onPause() method in Activity.

I am working on this issue, please let me know if this is the correct way. Thank you.

I think that's correct. Happy to help you :)

@hassan963 hassan963 linked a pull request Aug 28, 2019 that will close this issue
@Alexendoo Alexendoo removed the High Priority This issue is high on our todo list. It needs to be fixed as soon as possible. label Sep 2, 2019
@Tensounder54 Tensounder54 added the Has Solution There is a solution to this issue posted in the comment. label Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug This is an issue with Slide. This is where something in the app isn't working as intended. Good First Issue This issue is perfect for you if you are new to Slide. Has Solution There is a solution to this issue posted in the comment.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants