Issue/8199 memory leaks #8200
Issue/8199 memory leaks #8200
Conversation
There was a problem hiding this comment.
Nice solution to a not-so-easy crash! The code works as expected. I left a few comments about the code.
I was never able to reproduce the crash on develop without modifying the code. Since this crash is very rare with only two reported instances and 10.7 was released three days ago, I think these changes can wait until the 10.8 release.
| private LoadAvatarsTask mLoadAvatarsTask; | ||
| private final int mLikeAvatarSz; | ||
| private final int mMarginAvatar; | ||
| private final int mMarginReader; |
There was a problem hiding this comment.
I'm not sure why the mMarginAvatar and mMarginReader variables were created. They're only used in the getMaxAvatars() like the marginAvatar and marginReader variables before. Is there an advantage to moving them from method to class variables?
There was a problem hiding this comment.
This is just a super minor optimization -> the resources are loaded only once, when the view is created. However, if you prefer to move it back to getMaxAvatars(), no problem at all.
There was a problem hiding this comment.
The getMaxAvatars() method is called only once, which means the marginAvatar and marginReader method variables would be loaded only once similar to the mMarginAvatar and mMarginReader class variables, so I'm not sure about the optimization there. Albeit rather small, the class variables will make a larger footprint though, so it could be argued the method variables are actually an optimization.
I usually create a class variable only if it needs to be used by multiple classes or methods. Otherwise, I use a method variable to help show that it's only required within that method and as a tiny size optimization. However, that's fairly subjective. Let me know if you would like to update mLikeAvatarSz, mMarginAvatar, and mMarginReader or leave them as is.
| private final int mMaxAvatars; | ||
|
|
||
| LoadAvatarsTask(ReaderLikingUsersView view, long currentUserId, int likeAvatarSz, | ||
| int maxAvatars) { |
There was a problem hiding this comment.
The maxAvatars parameter can be on the same line with the method and other parameters for better readability.
| ReaderPost post = posts[0]; | ||
| ReaderUserIdList avatarIds = ReaderLikeTable.getLikesForPost(post); | ||
| return ReaderUserTable.getAvatarUrls(avatarIds, mMaxAvatars, mLikeAvatarSize, | ||
| mCurrentUserId); |
There was a problem hiding this comment.
The mCurrentUserId parameter can be on the same line with the method and other parameters for better readability.
|
Thank you @theck13!
I wasn't able to reproduce it without modifying the code either. What do you mean by |
|
Ohh I see, there are 2 reported instances on 10.7-rc1, but 74 instances on 10.6. |
There was a problem hiding this comment.
What do you mean by with only two reported instances, I see 74 crashes in production which doesn't seem negligible.
I was referring to your comment in the issue quoted below.
I've found two occurrences in the Fabric crashes.
If there are about 75 occurrences, then we can merge it into 10.7.
| private LoadAvatarsTask mLoadAvatarsTask; | ||
| private final int mLikeAvatarSz; | ||
| private final int mMarginAvatar; | ||
| private final int mMarginReader; |
There was a problem hiding this comment.
The getMaxAvatars() method is called only once, which means the marginAvatar and marginReader method variables would be loaded only once similar to the mMarginAvatar and mMarginReader class variables, so I'm not sure about the optimization there. Albeit rather small, the class variables will make a larger footprint though, so it could be argued the method variables are actually an optimization.
I usually create a class variable only if it needs to be used by multiple classes or methods. Otherwise, I use a method variable to help show that it's only required within that method and as a tiny size optimization. However, that's fairly subjective. Let me know if you would like to update mLikeAvatarSz, mMarginAvatar, and mMarginReader or leave them as is.
|
My reasoning was, that if we use this view in a RecyclerView, the method might be called every few ms. But since this is such a minor optimization and we don't use this view in a RecyclerView and I agree it looks cleaner with local variables, I've moved them back. :-) |
Fixes #8199
The app crashes when we try to load an image from a leaked context (after the activity has been destroyed).
This crash is quite rare, so it's almost impossible to reproduce it (usually occurs when the user has poor internet connection).
You can reproduce this crash by removing the
ReaderLikingUsersView.java.onDetachedFromWindowand addingThread.sleep(3000);toLoadAvatarsTask .doInBackground(..)