-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 memory leaks on viewing a post from a notification #21013
Conversation
|
App Name | ![]() |
|
Configuration | Release-Alpha | |
Build Number | pr21013-e44432e | |
Version | 22.7 | |
Bundle ID | com.jetpack.alpha | |
Commit | e44432e | |
App Center Build | jetpack-installable-builds #5238 |
|
App Name | ![]() |
|
Configuration | Release-Alpha | |
Build Number | pr21013-e44432e | |
Version | 22.7 | |
Bundle ID | org.wordpress.alpha | |
Commit | e44432e | |
App Center Build | WPiOS - One-Offs #6212 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -15,7 +15,7 @@ class ReaderCommentsFollowPresenter: NSObject { | |||
|
|||
private let post: ReaderPost | |||
private weak var delegate: ReaderCommentsFollowPresenterDelegate? | |||
private let presentingViewController: UIViewController | |||
private unowned let presentingViewController: UIViewController |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you chose unowned
so the type didn't have to become wrapped in Optional
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Given this is a "presenter" type, it should not use the view controller reference if the view controller is released. If this unowned reference causes a runtime crash in the production app, I think we may have other issues.
Relates to #20989.
Issue
When done viewing a post from the Notifications tab, the post detail view isn't released.
To reproduce this issue:
viewDidLoad
anddeinit
functions inReaderDetailViewController
.ReaderDetailViewController
.ReaderDetailViewController.viewDidLoad
. This step is just to confirm that theReaderDetailViewController
has been createdReaderDetailsViewController.deinit
.Performing the above steps on the trunk branch doesn't hit the
deinit
breakpoint, which means the view controller is not released.You can test this PR by repeating the same steps on this PR branch and verify that Xcode stops at
deinit
breakpoint.Regression Notes
Potential unintended areas of impact
None.
What I did to test those areas of impact (or what existing automated tests I relied on)
Viewing notifications and posts.
What automated tests I added (or what prevented me from doing so)
None.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.UI Changes testing checklist: N/A