Merge pull request #775 from wordpress-mobile/fix/21075-fix-retain-cycles-between-button-actions-and-view-controllers

staskus · web-flow · commit 61af6e842965 · 2023-07-17T10:36:52.000+03:00
Break retain cycles between button onTap action and view controllers
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -48,6 +48,12 @@ _None._
 
 _None._
 
+## 6.3.0
+
+### Bug Fixes
+
+- Fix retain cycles by using `weak self` in action closures. [#775]
+
 ## 6.2.0
 
 ### Bug Fixes
diff --git a/Podfile.lock b/Podfile.lock
@@ -22,7 +22,7 @@ PODS:
   - SVProgressHUD (2.2.5)
   - SwiftLint (0.49.1)
   - UIDeviceIdentifier (2.2.0)
-  - WordPressAuthenticator (6.2.0):
+  - WordPressAuthenticator (6.3.0-beta.1):
     - GoogleSignIn (~> 6.0.1)
     - Gridicons (~> 1.0)
     - "NSURL+IDN (= 0.4)"
@@ -94,7 +94,7 @@ SPEC CHECKSUMS:
   SVProgressHUD: 1428aafac632c1f86f62aa4243ec12008d7a51d6
   SwiftLint: 32ee33ded0636d0905ef6911b2b67bbaeeedafa5
   UIDeviceIdentifier: f33af270ba9045ea18b31d9aab88e42a0082ea67
-  WordPressAuthenticator: 8a27a3c61ca0d740df66f260902aa2ca8528c61b
+  WordPressAuthenticator: 5e3514db04817d1ee24feda3e7e979984c991af8
   WordPressKit: 8e1c5a64645a59493a7316f38468ac036de9c79b
   WordPressShared: 0aa459e5257a77184db87805a998f447443c9706
   WordPressUI: 1cf47a3b78154faf69caa18569ee7ece1e510fa0
diff --git a/WordPressAuthenticator.podspec b/WordPressAuthenticator.podspec
@@ -2,7 +2,7 @@
 
 Pod::Spec.new do |s|
   s.name          = 'WordPressAuthenticator'
-  s.version       = '6.2.0'
+  s.version       = '6.3.0-beta.1'
 
   s.summary       = 'WordPressAuthenticator implements an easy and elegant way to authenticate your WordPress Apps.'
   s.description   = <<-DESC
diff --git a/WordPressAuthenticator.xcodeproj/project.pbxproj b/WordPressAuthenticator.xcodeproj/project.pbxproj
@@ -7,6 +7,7 @@
 	objects = {
 
 /* Begin PBXBuildFile section */
+		0193F7752A615521004D7C16 /* MemoryManagementTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0193F7742A615521004D7C16 /* MemoryManagementTests.swift */; };
 		020BE74A23B0BD2E007FE54C /* WordPressAuthenticatorDisplayImages.swift in Sources */ = {isa = PBXBuildFile; fileRef = 020BE74923B0BD2E007FE54C /* WordPressAuthenticatorDisplayImages.swift */; };
 		020DEF6428AA091100C85D51 /* MagicLinkRequester.swift in Sources */ = {isa = PBXBuildFile; fileRef = 020DEF6328AA091100C85D51 /* MagicLinkRequester.swift */; };
 		02A526CA28A3499C00FD1812 /* MagicLinkRequestedViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 02A526C828A3499C00FD1812 /* MagicLinkRequestedViewController.swift */; };
@@ -261,6 +262,7 @@
 /* End PBXCopyFilesBuildPhase section */
 
 /* Begin PBXFileReference section */
+		0193F7742A615521004D7C16 /* MemoryManagementTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MemoryManagementTests.swift; sourceTree = "<group>"; };
 		020BE74923B0BD2E007FE54C /* WordPressAuthenticatorDisplayImages.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = WordPressAuthenticatorDisplayImages.swift; sourceTree = "<group>"; };
 		020DEF6328AA091100C85D51 /* MagicLinkRequester.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MagicLinkRequester.swift; sourceTree = "<group>"; };
 		02A526C828A3499C00FD1812 /* MagicLinkRequestedViewController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MagicLinkRequestedViewController.swift; sourceTree = "<group>"; };
@@ -956,6 +958,7 @@
 				3F86A83F29D280DC005D20C0 /* SingIn */,
 				F18DF0E32525009200D83AFE /* SupportingFiles */,
 				3F338B6B289B87E60014ADC5 /* UnitTests.xctestplan */,
+				0193F7742A615521004D7C16 /* MemoryManagementTests.swift */,
 			);
 			path = WordPressAuthenticatorTests;
 			sourceTree = "<group>";
@@ -1552,6 +1555,7 @@
 			buildActionMask = 2147483647;
 			files = (
 				3F4E64782990BBD4000DB555 /* IDTokenTests.swift in Sources */,
+				0193F7752A615521004D7C16 /* MemoryManagementTests.swift in Sources */,
 				3F86A84E29D3B53D005D20C0 /* SocialServiceTests.swift in Sources */,
 				3F879FDB293A49AA005C2B48 /* NewGoogleAuthenticatorTests.swift in Sources */,
 				F18DF0E5252500A600D83AFE /* WordPressAuthenticatorTests-Bridging-Header.h in Sources */,
diff --git a/WordPressAuthenticator/NUX/NUXTableViewController.swift b/WordPressAuthenticator/NUX/NUXTableViewController.swift
@@ -18,6 +18,9 @@ open class NUXTableViewController: UITableViewController, NUXViewControllerBase,
         return UIDevice.isPad() ? .all : .portrait
     }
 
+    // MARK: - Private
+    private var notificationObservers: [NSObjectProtocol] = []
+
     override open func viewDidLoad() {
         super.viewDidLoad()
         setupHelpButtonIfNeeded()
@@ -27,4 +30,17 @@ open class NUXTableViewController: UITableViewController, NUXViewControllerBase,
     public func shouldShowCancelButton() -> Bool {
         return shouldShowCancelButtonBase()
     }
+
+    // MARK: - Notification Observers
+
+    public func addNotificationObserver(_ observer: NSObjectProtocol) {
+        notificationObservers.append(observer)
+    }
+
+    deinit {
+        for observer in notificationObservers {
+            NotificationCenter.default.removeObserver(observer)
+        }
+        notificationObservers.removeAll()
+    }
 }
diff --git a/WordPressAuthenticator/NUX/NUXViewController.swift b/WordPressAuthenticator/NUX/NUXViewController.swift
@@ -17,6 +17,9 @@ open class NUXViewController: UIViewController, NUXViewControllerBase, UIViewCon
         }
     }
 
+    // MARK: - Private
+    private var notificationObservers: [NSObjectProtocol] = []
+
     override open var supportedInterfaceOrientations: UIInterfaceOrientationMask {
         return UIDevice.isPad() ? .all : .portrait
     }
@@ -53,6 +56,19 @@ open class NUXViewController: UIViewController, NUXViewControllerBase, UIViewCon
     public func shouldShowCancelButton() -> Bool {
         return shouldShowCancelButtonBase()
     }
+
+    // MARK: - Notification Observers
+
+    public func addNotificationObserver(_ observer: NSObjectProtocol) {
+        notificationObservers.append(observer)
+    }
+
+    deinit {
+        for observer in notificationObservers {
+            NotificationCenter.default.removeObserver(observer)
+        }
+        notificationObservers.removeAll()
+    }
 }
 
 extension NUXViewController {
diff --git a/WordPressAuthenticator/NUX/NUXViewControllerBase.swift b/WordPressAuthenticator/NUX/NUXViewControllerBase.swift
@@ -28,6 +28,9 @@ public protocol NUXViewControllerBase {
     ///
     func shouldShowCancelButton() -> Bool
     func setupCancelButtonIfNeeded()
+
+    /// Notification observers that can be tied to the lifecycle of the entities implementing the protocol
+    func addNotificationObserver(_ observer: NSObjectProtocol)
 }
 
 /// extension for NUXViewControllerBase where the base class is UIViewController (and thus also NUXTableViewController)
@@ -286,12 +289,17 @@ extension NUXViewControllerBase where Self: UIViewController, Self: UIViewContro
     private func setupNotificationsIndicator() {
         helpNotificationIndicator.isHidden = true
 
-        NotificationCenter.default.addObserver(forName: .wordpressSupportNotificationReceived, object: nil, queue: nil) { [weak self] _ in
-            self?.refreshSupportNotificationIndicator()
-        }
-        NotificationCenter.default.addObserver(forName: .wordpressSupportNotificationCleared, object: nil, queue: nil) { [weak self] _ in
-            self?.refreshSupportNotificationIndicator()
-        }
+        addNotificationObserver(
+            NotificationCenter.default.addObserver(forName: .wordpressSupportNotificationReceived, object: nil, queue: nil) { [weak self] _ in
+                self?.refreshSupportNotificationIndicator()
+            }
+        )
+
+        addNotificationObserver(
+            NotificationCenter.default.addObserver(forName: .wordpressSupportNotificationCleared, object: nil, queue: nil) { [weak self] _ in
+                self?.refreshSupportNotificationIndicator()
+            }
+        )
     }
 
     private func layoutNotificationIndicatorView(_ view: UIView, to superView: UIView) {
diff --git a/WordPressAuthenticator/Signin/LoginPrologueLoginMethodViewController.swift b/WordPressAuthenticator/Signin/LoginPrologueLoginMethodViewController.swift
@@ -60,7 +60,9 @@ class LoginPrologueLoginMethodViewController: NUXViewController {
             self.emailTapped?()
         }
 
-        buttonViewController.setupButtomButtonFor(socialService: .google, onTap: handleGoogleButtonTapped)
+        buttonViewController.setupButtomButtonFor(socialService: .google) { [weak self] in
+            self?.handleGoogleButtonTapped()
+        }
 
         if !LoginFields().restrictToWPCom && selfHostedTapped != nil {
             let selfHostedLoginButton = WPStyleGuide.selfHostedLoginButton(alignment: .center)
@@ -69,7 +71,9 @@ class LoginPrologueLoginMethodViewController: NUXViewController {
         }
 
         if WordPressAuthenticator.shared.configuration.enableSignInWithApple {
-            buttonViewController.setupTertiaryButtonFor(socialService: .apple, onTap: handleAppleButtonTapped)
+            buttonViewController.setupTertiaryButtonFor(socialService: .apple) { [weak self] in
+                self?.handleAppleButtonTapped()
+            }
         }
 
         buttonViewController.backgroundColor = WordPressAuthenticator.shared.style.buttonViewBackgroundColor
diff --git a/WordPressAuthenticator/Signin/LoginPrologueSignupMethodViewController.swift b/WordPressAuthenticator/Signin/LoginPrologueSignupMethodViewController.swift
@@ -56,7 +56,9 @@ class LoginPrologueSignupMethodViewController: NUXViewController {
             self?.emailTapped?()
         }
 
-        buttonViewController.setupButtomButtonFor(socialService: .google, onTap: handleGoogleButtonTapped)
+        buttonViewController.setupButtomButtonFor(socialService: .google) { [weak self] in
+            self?.handleGoogleButtonTapped()
+        }
 
         let termsButton = WPStyleGuide.termsButton()
         termsButton.on(.touchUpInside) { [weak self] _ in
@@ -76,7 +78,9 @@ class LoginPrologueSignupMethodViewController: NUXViewController {
         buttonViewController.stackView?.insertArrangedSubview(termsButton, at: 0)
 
         if WordPressAuthenticator.shared.configuration.enableSignInWithApple {
-            buttonViewController.setupTertiaryButtonFor(socialService: .apple, onTap: handleAppleButtonTapped)
+            buttonViewController.setupTertiaryButtonFor(socialService: .apple) { [weak self] in
+                self?.handleAppleButtonTapped()
+            }
         }
 
         buttonViewController.backgroundColor = WordPressAuthenticator.shared.style.buttonViewBackgroundColor
diff --git a/WordPressAuthenticator/Signin/LoginPrologueViewController.swift b/WordPressAuthenticator/Signin/LoginPrologueViewController.swift
@@ -30,9 +30,9 @@ class LoginPrologueViewController: LoginViewController {
     private let configuration = WordPressAuthenticator.shared.configuration
     private let style = WordPressAuthenticator.shared.style
 
-    private lazy var storedCredentialsAuthenticator = StoredCredentialsAuthenticator(onCancel: {
+    private lazy var storedCredentialsAuthenticator = StoredCredentialsAuthenticator(onCancel: { [weak self] in
         // Since the authenticator has its own flow
-        self.tracker.resetState()
+        self?.tracker.resetState()
     })
 
     /// We can't rely on `isMovingToParent` to know if we need to track the `.prologue` step
diff --git a/WordPressAuthenticator/Unified Auth/StoredCredentialsAuthenticator.swift b/WordPressAuthenticator/Unified Auth/StoredCredentialsAuthenticator.swift
@@ -41,7 +41,7 @@ class StoredCredentialsAuthenticator: NSObject {
     // MARK: - UI
 
     private let picker = StoredCredentialsPicker()
-    private var navigationController: UINavigationController?
+    private weak var navigationController: UINavigationController?
 
     // MARK: - Tracking Support
 
diff --git a/WordPressAuthenticator/Unified Auth/View Related/Get Started/GetStartedViewController.swift b/WordPressAuthenticator/Unified Auth/View Related/Get Started/GetStartedViewController.swift
@@ -785,10 +785,14 @@ private extension GetStartedViewController {
         buttonViewController.hideShadowView()
 
         if configuration.enableSignInWithApple {
-            buttonViewController.setupTopButtonFor(socialService: .apple, onTap: appleTapped)
+            buttonViewController.setupTopButtonFor(socialService: .apple) { [weak self] in
+                self?.appleTapped()
+            }
         }
 
-        buttonViewController.setupButtomButtonFor(socialService: .google, onTap: googleTapped)
+        buttonViewController.setupButtomButtonFor(socialService: .google) { [weak self] in
+            self?.googleTapped()
+        }
 
         let termsButton = WPStyleGuide.signupTermsButton()
         buttonViewController.stackView?.addArrangedSubview(termsButton)
@@ -807,14 +811,16 @@ private extension GetStartedViewController {
         //
         buttonViewController.setupTopButton(title: ButtonConfiguration.Continue.title,
                                             isPrimary: true,
-                                            accessibilityIdentifier: ButtonConfiguration.Continue.accessibilityIdentifier,
-                                            onTap: handleSubmitButtonTapped)
+                                            accessibilityIdentifier: ButtonConfiguration.Continue.accessibilityIdentifier) { [weak self] in
+            self?.handleSubmitButtonTapped()
+        }
 
         // Setup Sign in with site credentials button
         buttonViewController.setupBottomButton(attributedTitle: WPStyleGuide.formattedSignInWithSiteCredentialsString(),
                                                isPrimary: false,
-                                               accessibilityIdentifier: ButtonConfiguration.SignInWithSiteCredentials.accessibilityIdentifier,
-                                               onTap: handleSiteCredentialsButtonTapped)
+                                               accessibilityIdentifier: ButtonConfiguration.SignInWithSiteCredentials.accessibilityIdentifier) { [weak self] in
+            self?.handleSiteCredentialsButtonTapped()
+        }
     }
 
     func configureButtonViewControllerWithoutSocialLogin() {
@@ -828,8 +834,9 @@ private extension GetStartedViewController {
         //
         buttonViewController.setupTopButton(title: ButtonConfiguration.Continue.title,
                                             isPrimary: true,
-                                            accessibilityIdentifier: ButtonConfiguration.Continue.accessibilityIdentifier,
-                                            onTap: handleSubmitButtonTapped)
+                                            accessibilityIdentifier: ButtonConfiguration.Continue.accessibilityIdentifier) { [weak self] in
+            self?.handleSubmitButtonTapped()
+        }
     }
 
     @objc func appleTapped() {
diff --git a/WordPressAuthenticator/Unified Auth/View Related/Password/PasswordCoordinator.swift b/WordPressAuthenticator/Unified Auth/View Related/Password/PasswordCoordinator.swift
@@ -2,7 +2,7 @@
 /// Based on the configuration, it could automatically send a magic link and proceed the magic link requested screen on success and fall back to password.
 @MainActor
 final class PasswordCoordinator {
-    private let navigationController: UINavigationController
+    private weak var navigationController: UINavigationController?
     private let source: SignInSource?
     private let loginFields: LoginFields
     private let tracker: AuthenticatorAnalyticsTracker
@@ -50,7 +50,7 @@ private extension PasswordCoordinator {
         let vc = MagicLinkRequestedViewController(email: loginFields.username) { [weak self] in
             self?.showPassword()
         }
-        navigationController.pushViewController(vc, animated: true)
+        navigationController?.pushViewController(vc, animated: true)
     }
 
     /// Navigates the user to enter WP.com password.
@@ -63,6 +63,6 @@ private extension PasswordCoordinator {
         vc.loginFields = loginFields
         vc.trackAsPasswordChallenge = false
 
-        navigationController.pushViewController(vc, animated: true)
+        navigationController?.pushViewController(vc, animated: true)
     }
 }
diff --git a/WordPressAuthenticator/Unified Auth/View Related/VerifyEmail/VerifyEmailViewController.swift b/WordPressAuthenticator/Unified Auth/View Related/VerifyEmail/VerifyEmailViewController.swift
@@ -106,13 +106,15 @@ private extension VerifyEmailViewController {
 
         // Setup `Send email verification link` button
         buttonViewController.setupTopButton(title: ButtonConfiguration.SendEmailVerificationLink.title,
-                                            isPrimary: true,
-                                            onTap: handleSendEmailVerificationLinkButtonTapped)
+                                            isPrimary: true) { [weak self] in
+            self?.handleSendEmailVerificationLinkButtonTapped()
+        }
 
         // Setup `Login with account password` button
         buttonViewController.setupBottomButton(title: ButtonConfiguration.LoginWithAccountPassword.title,
-                                            isPrimary: false,
-                                            onTap: handleLoginWithAccountPasswordButtonTapped)
+                                            isPrimary: false) { [weak self] in
+            self?.handleLoginWithAccountPasswordButtonTapped()
+        }
     }
 
     // MARK: - Actions
diff --git a/WordPressAuthenticatorTests/MemoryManagementTests.swift b/WordPressAuthenticatorTests/MemoryManagementTests.swift
@@ -0,0 +1,60 @@
+@testable import WordPressAuthenticator
+import XCTest
+
+final class MemoryManagementTests: XCTestCase {
+    override func setUp() {
+        super.setUp()
+
+        WordPressAuthenticator.initialize(
+          configuration: WordpressAuthenticatorProvider.wordPressAuthenticatorConfiguration(),
+          style: WordpressAuthenticatorProvider.wordPressAuthenticatorStyle(.random),
+          unifiedStyle: WordpressAuthenticatorProvider.wordPressAuthenticatorUnifiedStyle(.random)
+        )
+    }
+
+    func testViewControllersDeallocatedAfterDismissing() {
+        let viewControllers: [UIViewController] = [
+            Storyboard.login.instance.instantiateInitialViewController()!,
+            LoginPrologueLoginMethodViewController.instantiate(from: .login)!,
+            LoginPrologueSignupMethodViewController.instantiate(from: .login)!,
+            Login2FAViewController.instantiate(from: .login)!,
+            LoginEmailViewController.instantiate(from: .login)!,
+            LoginSelfHostedViewController.instantiate(from: .login)!,
+            LoginSiteAddressViewController.instantiate(from: .login)!,
+            LoginUsernamePasswordViewController.instantiate(from: .login)!,
+            LoginWPComViewController.instantiate(from: .login)!,
+            SignupEmailViewController.instantiate(from: .signup)!,
+            SignupGoogleViewController.instantiate(from: .signup)!,
+            GetStartedViewController.instantiate(from: .getStarted)!,
+            VerifyEmailViewController.instantiate(from: .verifyEmail)!,
+            PasswordViewController.instantiate(from: .password)!,
+            TwoFAViewController.instantiate(from: .twoFA)!,
+            GoogleAuthViewController.instantiate(from: .googleAuth)!,
+            SiteAddressViewController.instantiate(from: .siteAddress)!,
+            SiteCredentialsViewController.instantiate(from: .siteAddress)!
+        ]
+
+        for viewController in viewControllers {
+            viewController.loadViewIfNeeded()
+        }
+
+        verifyObjectsDeallocatedAfterTeardown(viewControllers)
+    }
+
+    // MARK: - Helpers
+
+    private func verifyObjectsDeallocatedAfterTeardown(_ objects: [AnyObject]) {
+        /// Create the array of weak objects so we could assert them in the teardown block
+        let weakObjects: [() -> AnyObject?] = objects.map { object in { [weak object] in
+                return object
+            }
+        }
+
+        /// All the weak items should be deallocated in the teardown block unless there's a retain cycle holding them
+        addTeardownBlock {
+            for object in weakObjects {
+                XCTAssertNil(object(), "\(object()!.self) is not deallocated after teardown")
+            }
+        }
+    }
+}

Original file line number	Diff line number	Diff line change
`@@ -17,6 +17,9 @@ open class NUXViewController: UIViewController, NUXViewControllerBase, UIViewCon`
`17`	`17`	`}`
`18`	`18`	`}`
`19`	`19`
	`20`	`+ // MARK: - Private`
	`21`	`+ private var notificationObservers: [NSObjectProtocol] = []`
	`22`	`+`
`20`	`23`	`override open var supportedInterfaceOrientations: UIInterfaceOrientationMask {`
`21`	`24`	`return UIDevice.isPad() ? .all : .portrait`
`22`	`25`	`}`
`@@ -53,6 +56,19 @@ open class NUXViewController: UIViewController, NUXViewControllerBase, UIViewCon`
`53`	`56`	`public func shouldShowCancelButton() -> Bool {`
`54`	`57`	`return shouldShowCancelButtonBase()`
`55`	`58`	`}`
	`59`	`+`
	`60`	`+ // MARK: - Notification Observers`
	`61`	`+`
	`62`	`+ public func addNotificationObserver(_ observer: NSObjectProtocol) {`
	`63`	`+ notificationObservers.append(observer)`
	`64`	`+ }`
	`65`	`+`
	`66`	`+ deinit {`
	`67`	`+ for observer in notificationObservers {`
	`68`	`+ NotificationCenter.default.removeObserver(observer)`
	`69`	`+ }`
	`70`	`+ notificationObservers.removeAll()`
	`71`	`+ }`
`56`	`72`	`}`
`57`	`73`
`58`	`74`	`extension NUXViewController {`
Original file line number	Diff line number	Diff line change
`@@ -60,7 +60,9 @@ class LoginPrologueLoginMethodViewController: NUXViewController {`
`60`	`60`	`self.emailTapped?()`
`61`	`61`	`}`
`62`	`62`
`63`		`- buttonViewController.setupButtomButtonFor(socialService: .google, onTap: handleGoogleButtonTapped)`
	`63`	`+ buttonViewController.setupButtomButtonFor(socialService: .google) { [weak self] in`
	`64`	`+ self?.handleGoogleButtonTapped()`
	`65`	`+ }`
`64`	`66`
`65`	`67`	`if !LoginFields().restrictToWPCom && selfHostedTapped != nil {`
`66`	`68`	`let selfHostedLoginButton = WPStyleGuide.selfHostedLoginButton(alignment: .center)`
`@@ -69,7 +71,9 @@ class LoginPrologueLoginMethodViewController: NUXViewController {`
`69`	`71`	`}`
`70`	`72`
`71`	`73`	`if WordPressAuthenticator.shared.configuration.enableSignInWithApple {`
`72`		`- buttonViewController.setupTertiaryButtonFor(socialService: .apple, onTap: handleAppleButtonTapped)`
	`74`	`+ buttonViewController.setupTertiaryButtonFor(socialService: .apple) { [weak self] in`
	`75`	`+ self?.handleAppleButtonTapped()`
	`76`	`+ }`
`73`	`77`	`}`
`74`	`78`
`75`	`79`	`buttonViewController.backgroundColor = WordPressAuthenticator.shared.style.buttonViewBackgroundColor`