Skip to content

Commit

Permalink
Resolve memory leaks in RootViewCoordinator
Browse files Browse the repository at this point in the history
Problem:
- RootViewCoordinator is a singleton
- RootViewCoordinator holds a strong reference to RootViewPresenter
- WPTabBarController implements RootViewPresenter and is presenter as rootViewController
- When WPTabBarController is dismissed, RootViewCoordinator continues to hold a strong reference to a WPTabBarController creating a memory leak

Solution:
- Move presentation logic from WindowsManager to RootViewCoordinator
- Only initialize RootViewPresenter when needed
- Release RootViewPresenter after logging out. Not making RootViewPresenter weak or unowned to avoid further cascading changes through the codebase to handle RootViewPresenter being optional
- RootViewPresenter should not be accessed before the root view is presented. If it happens - print a warning. All the issues should be addressed with further improvements.
  • Loading branch information
staskus committed Jul 11, 2023
1 parent d71ac66 commit 12f7efd
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 26 deletions.
58 changes: 41 additions & 17 deletions WordPress/Classes/System/RootViewCoordinator.swift
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import Foundation
import WordPressAuthenticator

extension NSNotification.Name {
static let WPAppUITypeChanged = NSNotification.Name(rawValue: "WPAppUITypeChanged")
Expand All @@ -19,7 +20,17 @@ class RootViewCoordinator {
static let shared = RootViewCoordinator(featureFlagStore: RemoteFeatureFlagStore(),
windowManager: WordPressAppDelegate.shared?.windowManager)
static var sharedPresenter: RootViewPresenter {
shared.rootViewPresenter
guard let rootViewPresenter = shared.rootViewPresenter else {
/// Accessing RootViewPresenter before root view is presented is incorrect behavior
/// It shows either inconsistent order of app dependency initialization
/// or that RootViewPresenter contains actions unrelated to presented views
DDLogWarn("RootViewPresenter is accessed before root view is presented")
let rootViewPresenter = shared.createPresenter(shared.currentAppUIType)
shared.rootViewPresenter = rootViewPresenter
return rootViewPresenter
}

return rootViewPresenter
}

// MARK: Public Variables
Expand All @@ -34,7 +45,7 @@ class RootViewCoordinator {

// MARK: Private instance variables

private(set) var rootViewPresenter: RootViewPresenter
private var rootViewPresenter: RootViewPresenter?
private var currentAppUIType: AppUIType {
didSet {
updateJetpackFeaturesRemovalCoordinatorState()
Expand All @@ -50,17 +61,39 @@ class RootViewCoordinator {
self.featureFlagStore = featureFlagStore
self.windowManager = windowManager
self.currentAppUIType = Self.appUIType(featureFlagStore: featureFlagStore)
switch self.currentAppUIType {
updateJetpackFeaturesRemovalCoordinatorState()
}

// MARK: - Root Coordination

func showAppUI(animated: Bool = true, completion: (() -> Void)? = nil) {
let rootViewPresenter = createPresenter(currentAppUIType)
windowManager?.show(rootViewPresenter.rootViewController, animated: animated, completion: completion)
self.rootViewPresenter = rootViewPresenter

updatePromptsIfNeeded()
}

func showSignInUI(completion: (() -> Void)? = nil) {
guard let loginViewController = WordPressAuthenticator.loginUI() else {
fatalError("No login UI to show to the user. There's no way to gracefully handle this error.")
}

windowManager?.show(loginViewController, completion: completion)
WordPressAuthenticator.track(.openedLogin)
self.rootViewPresenter = nil
}

private func createPresenter(_ appType: AppUIType) -> RootViewPresenter {
switch appType {
case .normal:
self.rootViewPresenter = WPTabBarController(staticScreens: false)
return WPTabBarController(staticScreens: false)
case .simplified:
let meScenePresenter = MeScenePresenter()
self.rootViewPresenter = MySitesCoordinator(meScenePresenter: meScenePresenter, onBecomeActiveTab: {})
return MySitesCoordinator(meScenePresenter: meScenePresenter, onBecomeActiveTab: {})
case .staticScreens:
self.rootViewPresenter = StaticScreensTabBarWrapper()
return StaticScreensTabBarWrapper()
}
updateJetpackFeaturesRemovalCoordinatorState()
updatePromptsIfNeeded()
}

// MARK: JP Features State
Expand Down Expand Up @@ -122,15 +155,6 @@ class RootViewCoordinator {
}

private func reloadUI(using windowManager: WindowManager) {
switch currentAppUIType {
case .normal:
self.rootViewPresenter = WPTabBarController(staticScreens: false)
case .simplified:
let meScenePresenter = MeScenePresenter()
self.rootViewPresenter = MySitesCoordinator(meScenePresenter: meScenePresenter, onBecomeActiveTab: {})
case .staticScreens:
self.rootViewPresenter = StaticScreensTabBarWrapper()
}
windowManager.showUI(animated: false)
}

Expand Down
9 changes: 2 additions & 7 deletions WordPress/Classes/System/WindowManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class WindowManager: NSObject {
///
@objc func showAppUI(for blog: Blog? = nil, animated: Bool = true, completion: Completion? = nil) {
isShowingFullscreenSignIn = false
show(RootViewCoordinator.sharedPresenter.rootViewController, animated: animated, completion: completion)
RootViewCoordinator.shared.showAppUI(animated: animated, completion: completion)

guard let blog = blog else {
return
Expand All @@ -81,12 +81,7 @@ class WindowManager: NSObject {
func showSignInUI(completion: Completion? = nil) {
isShowingFullscreenSignIn = true

guard let loginViewController = WordPressAuthenticator.loginUI() else {
fatalError("No login UI to show to the user. There's no way to gracefully handle this error.")
}

show(loginViewController, completion: completion)
WordPressAuthenticator.track(.openedLogin)
RootViewCoordinator.shared.showSignInUI(completion: completion)
}

/// Shows the specified VC as the root VC for the managed window. Takes care of animating the transition whenever the existing
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ extension RootViewCoordinator {
}

@objc func updatePromptsIfNeeded() {
guard let blog = rootViewPresenter.currentOrLastBlog() else {
guard let blog = Self.sharedPresenter.currentOrLastBlog() else {
return
}

Expand All @@ -22,7 +22,7 @@ extension RootViewCoordinator {
guard Feature.enabled(.bloggingPrompts),
let siteID = userInfo[BloggingPrompt.NotificationKeys.siteID] as? Int,
let blog = accountSites?.first(where: { $0.dotComID == NSNumber(value: siteID) }),
let viewController = rootViewPresenter.currentViewController else {
let viewController = Self.sharedPresenter.currentViewController else {
return
}

Expand Down

0 comments on commit 12f7efd

Please sign in to comment.