Skip to content
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 retain cycles preventing objects from being deallocated after logging out #21047

Merged
merged 19 commits into from
Jul 14, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
4791b6f
Set weak reference for BlogDashboardViewController in DashboardBlazeC…
staskus Jul 10, 2023
692c523
Weakly capture action closure in DashbordPagesListCardCell
staskus Jul 10, 2023
9ec904c
Pass weak closure instead of strongly captured method in DashboardPro…
staskus Jul 10, 2023
a7f12dd
Break circular dependency cycle for BlogDashboardViewController in qu…
staskus Jul 10, 2023
db45915
Break circular dependency cycle for BlogDashboardViewController in Da…
staskus Jul 10, 2023
857430c
Capture BlogDashboardViewController weekly in BlogDashboardViewModel
staskus Jul 10, 2023
c90bfdf
Break retain cycle by capturing presented view controllers weakly in …
staskus Jul 10, 2023
a30d50c
Weak self in NotificationsViewController
staskus Jul 10, 2023
7cff19a
Capture ReaderTabViewModel weakly in a closure
staskus Jul 10, 2023
03f3598
Use weak closures instead of passing methods to a closure to avoid re…
staskus Jul 10, 2023
d71ac66
Merge branch 'trunk' into fix/21010-memory-not-deallocated-after-logg…
staskus Jul 11, 2023
12f7efd
Resolve memory leaks in RootViewCoordinator
staskus Jul 11, 2023
daf8e08
Update RELEASE-NOTES.txt
staskus Jul 11, 2023
7d48ab1
Merge branch 'trunk' into fix/21010-memory-not-deallocated-after-logg…
staskus Jul 12, 2023
8d048fe
Update RELEASE-NOTES.txt
staskus Jul 12, 2023
3ce5076
Added unit tests confirming deallocation of rootViewController after …
staskus Jul 12, 2023
b53d04f
Merge branch 'trunk' into fix/21010-memory-not-deallocated-after-logg…
staskus Jul 12, 2023
de729ae
Update RELEASE-NOTES.txt
staskus Jul 13, 2023
d1358ae
Merge branch 'trunk' into fix/21010-memory-not-deallocated-after-logg…
staskus Jul 13, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions RELEASE-NOTES.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
22.9
-----
* [**] [internal] Fix multiple memory leaks after logging in and logging out. [#21047]
staskus marked this conversation as resolved.
Show resolved Hide resolved

22.8
-----
* [*] Blogging Reminders: Disabled prompt for self-hosted sites not connected to Jetpack. [#20970]
Expand Down
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
guarani marked this conversation as resolved.
Show resolved Hide resolved
}

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 @@ -3,7 +3,7 @@ import WordPressKit

final class DashboardBlazeCardCell: DashboardCollectionViewCell {
private var blog: Blog?
private var viewController: BlogDashboardViewController?
private weak var viewController: BlogDashboardViewController?
private var viewModel: DashboardBlazeCardCellViewModel?

func configure(blog: Blog, viewController: BlogDashboardViewController?, apiResponse: BlogDashboardRemoteEntity?) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,9 @@ extension DashboardPagesListCardCell {
private func makeAllPagesAction() -> UIMenuElement {
let allPagesAction = UIAction(title: Strings.allPages,
image: Style.allPagesImage,
handler: { _ in self.showPagesList(source: .contextMenu) })
handler: { [weak self] _ in
self?.showPagesList(source: .contextMenu)
})

// Wrap the pages action in a menu to display a divider between the pages action and hide this action.
// https://developer.apple.com/documentation/uikit/uimenu/options/3261455-displayinline
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,26 @@ class DashboardPromptsCardCell: UICollectionViewCell, Reusable {

// Defines the structure of the contextual menu items.
private var contextMenuItems: [[MenuItem]] {
let viewMoreMenuTapped = { [weak self] in
guard let self else { return }
self.viewMoreMenuTapped()
}

let skipMenuTapped = { [weak self] in
guard let self else { return }
self.skipMenuTapped()
}

let learnMoreTapped = { [weak self] in
guard let self else { return }
self.learnMoreTapped()
}

let removeMenuTapped = { [weak self] in
guard let self else { return }
self.removeMenuTapped()
}

guarani marked this conversation as resolved.
Show resolved Hide resolved
let defaultItems: [MenuItem] = [
.viewMore(viewMoreMenuTapped),
.skip(skipMenuTapped)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,20 +74,24 @@ extension DashboardQuickActionsCardCell {
}

private func configureTapEvents(for blog: Blog, with sourceController: UIViewController) {
statsButton.onTap = { [weak self] in
self?.showStats(for: blog, from: sourceController)
statsButton.onTap = { [weak self, weak sourceController] in
guard let self, let sourceController else { return }
self.showStats(for: blog, from: sourceController)
}

postsButton.onTap = { [weak self] in
self?.showPostList(for: blog, from: sourceController)
postsButton.onTap = { [weak self, weak sourceController] in
guard let self, let sourceController else { return }
self.showPostList(for: blog, from: sourceController)
}

mediaButton.onTap = { [weak self] in
self?.showMediaLibrary(for: blog, from: sourceController)
mediaButton.onTap = { [weak self, weak sourceController] in
guard let self, let sourceController else { return }
self.showMediaLibrary(for: blog, from: sourceController)
}

pagesButton.onTap = { [weak self] in
self?.showPageList(for: blog, from: sourceController)
pagesButton.onTap = { [weak self, weak sourceController] in
guard let self, let sourceController else { return }
self.showPageList(for: blog, from: sourceController)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,10 @@ extension DashboardStatsCardCell: BlogDashboardCardConfigurable {
}

private func configureCard(for blog: Blog, in viewController: UIViewController) {
frameView.onViewTap = { [weak self] in
self?.showStats(for: blog, from: viewController)
frameView.onViewTap = { [weak self, weak viewController] in
guard let self, let viewController else { return }

self.showStats(for: blog, from: viewController)
}

if FeatureFlag.personalizeHomeTab.enabled {
Expand All @@ -89,8 +91,10 @@ extension DashboardStatsCardCell: BlogDashboardCardConfigurable {
statsStackView?.visitors = viewModel?.todaysVisitors
statsStackView?.likes = viewModel?.todaysLikes

nudgeView?.onTap = { [weak self] in
self?.showNudgeHint(for: blog, from: viewController)
nudgeView?.onTap = { [weak self, weak viewController] in
guard let self, let viewController else { return }

self.showNudgeHint(for: blog, from: viewController)
}

nudgeView?.isHidden = !(viewModel?.shouldDisplayNudge ?? false)
Expand All @@ -101,8 +105,10 @@ extension DashboardStatsCardCell: BlogDashboardCardConfigurable {
}

private func makeShowStatsMenuAction(for blog: Blog, in viewController: UIViewController) -> UIAction {
UIAction(title: Strings.viewStats, image: UIImage(systemName: "chart.bar.xaxis")) { [weak self] _ in
self?.showStats(for: blog, from: viewController)
UIAction(title: Strings.viewStats, image: UIImage(systemName: "chart.bar.xaxis")) { [weak self, weak viewController] _ in
guard let self, let viewController else { return }

self.showStats(for: blog, from: viewController)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ final class BlogDashboardViewModel {
return nil
}

return DashboardDataSource(collectionView: viewController.collectionView) { [unowned self] collectionView, indexPath, item in
return DashboardDataSource(collectionView: viewController.collectionView) { [unowned self, unowned viewController] collectionView, indexPath, item in
guarani marked this conversation as resolved.
Show resolved Hide resolved

var cellType: DashboardCollectionViewCell.Type
var cardType: DashboardCard
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
Original file line number Diff line number Diff line change
Expand Up @@ -139,13 +139,13 @@ class MySiteViewController: UIViewController, NoResultsViewHost {
}
}

private(set) var sitePickerViewController: SitePickerViewController?
private(set) var blogDetailsViewController: BlogDetailsViewController? {
private(set) weak var sitePickerViewController: SitePickerViewController?
private(set) weak var blogDetailsViewController: BlogDetailsViewController? {
didSet {
blogDetailsViewController?.presentationDelegate = self
}
}
private var blogDashboardViewController: BlogDashboardViewController?
private weak var blogDashboardViewController: BlogDashboardViewController?

/// When we display a no results view, we'll do so in a scrollview so that
/// we can allow pull to refresh to sync the user's list of sites.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -513,10 +513,14 @@ class NotificationsViewController: UIViewController, UIViewControllerRestoration
detailsViewController.dataSource = self
detailsViewController.notificationCommentDetailCoordinator = notificationCommentDetailCoordinator
detailsViewController.note = note
detailsViewController.onDeletionRequestCallback = { request in
detailsViewController.onDeletionRequestCallback = { [weak self] request in
guard let self else { return }

self.showUndeleteForNoteWithID(note.objectID, request: request)
}
detailsViewController.onSelectedNoteChange = { note in
detailsViewController.onSelectedNoteChange = { [weak self] note in
guard let self else { return }

self.selectRow(for: note)
}
}
Expand Down Expand Up @@ -752,7 +756,9 @@ extension NotificationsViewController {
return
}

syncNotification(with: noteId, timeout: Syncing.pushMaxWait) { note in
syncNotification(with: noteId, timeout: Syncing.pushMaxWait) { [weak self] note in
guard let self else { return }

self.showDetails(for: note)
}
}
Expand Down Expand Up @@ -941,7 +947,9 @@ private extension NotificationsViewController {
reloadResultsController()

// Hit the Deletion Action
request.action { success in
request.action { [weak self] success in
guard let self else { return }

self.notificationDeletionRequests.removeValue(forKey: noteObjectID)
self.notificationIdsBeingDeleted.remove(noteObjectID)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ class ReaderTabViewController: UIViewController {

private let makeReaderTabView: (ReaderTabViewModel) -> ReaderTabView

private lazy var readerTabView: ReaderTabView = {
private lazy var readerTabView: ReaderTabView = { [unowned viewModel] in
return makeReaderTabView(viewModel)
}()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,22 @@ extension WPTabBarController {
}

@objc func makeReaderTabViewController() -> ReaderTabViewController {
return ReaderTabViewController(viewModel: self.readerTabViewModel, readerTabViewFactory: makeReaderTabView(_:))
return ReaderTabViewController(viewModel: readerTabViewModel) { [unowned self] viewModel in
return self.makeReaderTabView(viewModel)
}
}

@objc func makeReaderTabViewModel() -> ReaderTabViewModel {
let viewModel = ReaderTabViewModel(readerContentFactory: makeReaderContentViewController(with:),
searchNavigationFactory: navigateToReaderSearch,
tabItemsStore: ReaderTabItemsStore(),
settingsPresenter: ReaderManageScenePresenter())
let viewModel = ReaderTabViewModel(
readerContentFactory: { [unowned self] in
self.makeReaderContentViewController(with: $0)
},
searchNavigationFactory: { [unowned self] in
self.navigateToReaderSearch()
},
tabItemsStore: ReaderTabItemsStore(),
settingsPresenter: ReaderManageScenePresenter()
)
return viewModel
}

Expand Down