Conversation
Assigning function directly to a closure create a retain cycle between buttonViewController and viewController. Breaking it by using weak self.
Issue: - NavigationController holds onto GetStartedViewController - GetStartedViewController holds onto PasswordCoordinator - PasswordCoordinator holds onto NavigationController Solution: - Break the retain cycle by reference navigationController weakly
Issue: - NavigationController holds onto LoginPrologueViewController - LoginPrologueViewController holds onto StoredCredentialsAuthenticator - StoredCredentialsAuthenticator holds onto NavigationController Solution: - Break the retain cycle by reference navigationController weakly
0ba6dd5 to
c06ffa1
Compare
|
I'm thinking about how to better test it for the reviewer. Do you think it's better to expand the testing suite here (add unit tests, add more example UI) or is it enough to build and test wordpress-mobile/WordPress-iOS#21092 locally together with these changes? Most of the changes are providing |
…ain cycles - Created verifyObjectsDeallocatedAfterTeardown method that can be used for confirming that any object is deallocated after it's lifecycle should end - Testing most of the view controllers confirming that there aren't easily reproducible memory leaks remaining
|
@guarani To save the time, here's the test in action showing it failing when reverting some of the changes I made: Memory.Management.tests.mov |
NUXViewControllerBase is implemented as a protocol. In order not to change architecture using Obj-C runtime feature to create observers and deinitialize them on deinit
|
👋 Hey @staskus, these tests for memory leaks look promising. |
guarani
left a comment
There was a problem hiding this comment.
Changes look good! I first checked that memory usage was going up when navigating the demo app on trunk and then verified that it stays the same on this branch. I also checked the Memory Graph and saw that none of the view controllers beyond the demo's home screen stay in memory.
| private var wordpressSupportNotificationReceivedObserver: NSObjectProtocol? { | ||
| get { | ||
|
|
||
| objc_getAssociatedObject(self, &wordpressSupportNotificationReceivedKey) as? NSObjectProtocol | ||
| } | ||
| set { | ||
| objc_setAssociatedObject(self, &wordpressSupportNotificationReceivedKey, newValue, .OBJC_ASSOCIATION_RETAIN) | ||
| } | ||
| } | ||
|
|
||
| private var wordpressSupportNotificationClearedObserver: NSObjectProtocol? { | ||
| get { | ||
| objc_getAssociatedObject(self, &wordpressSupportNotificationClearedKey) as? NSObjectProtocol | ||
| } | ||
| set { | ||
| objc_setAssociatedObject(self, &wordpressSupportNotificationClearedKey, newValue, .OBJC_ASSOCIATION_RETAIN) | ||
| } | ||
| } |
There was a problem hiding this comment.
nitpick: Another option that came to mind is letting the classes that implement this protocol store the observers. These variables would move to the protocol:
var wordpressSupportNotificationReceivedObserver: NSObjectProtocol? { get set }
var wordpressSupportNotificationClearedObserver: NSObjectProtocol? { get set }
then the protocol could be class-bound:
public protocol NUXViewControllerBase: AnyObject {
...
Then each implementing class would need to implement these variables:
class NUXTableViewController {
public var wordpressSupportNotificationReceivedObserver: NSObjectProtocol?
public var wordpressSupportNotificationClearedObserver: NSObjectProtocol?
...
However, neither option seems clearly better than the other, so happy to leave it as-is.
There was a problem hiding this comment.
Thanks! Yes, I don't like adding this Obj-C magic to the code, I didn't want to refactor the structure of the code too much though. I will check your approach, maybe it will be a sweet spot between avoiding the Obj-C runtime and having to refactor too much in the original code.
There was a problem hiding this comment.
I looked at the change and it seems like a good approach (not one I would have thought of) 👍
addNotificationObserver caught my eye because it's storing the observer (after its added), despite the name being being add. Definitely not an issue, just an observation and no need (from my side) to change it.
There was a problem hiding this comment.
I forgot to mention, the approach you suggested wasn't acceptable to the compiler. If we wanted to set observer value, we had to make all the methods inside the extension mutable. Also, it prevented from calling those methods indeinit.
addNotificationObserver caught my eye because it's storing the observer (after its added), despite the name being being add. Definitely not an issue, just an observation and no need (from my side) to change it.
Yes, probably not the best naming. Due to the mutable issue I described above, I need to make the value-changing operations from inside the viewControllers. I thought using array instead of separate observer vars will be a cleaner approach.
There was a problem hiding this comment.
Do you recall if you made the protocol class-bound? I also ran into this when I was trying out the above last week, and then realized that the error only happens because the compiler thinks the protocol could be used on structs as well. Adding : AnyObject to the protocol fixed it for me.
There was a problem hiding this comment.
Do you recall if you made the protocol class-bound?
No, I haven't, that's a good point, I missed that from the mutable hint of the compiler 🥲 Yes, then that would've worked.
Part of: wordpress-mobile/WordPress-iOS#21075
Assigning a function directly to a closure creates a retain cycle between buttonViewController and viewController.
Solution
Pass a closure and use a
weak selfinstead. Explaining each fix in the commits.CHANGELOG.mdif necessary.