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

Memory not deallocated after logging out #21010

Closed
staskus opened this issue Jul 5, 2023 · 2 comments · Fixed by #21047
Closed

Memory not deallocated after logging out #21010

staskus opened this issue Jul 5, 2023 · 2 comments · Fixed by #21047
Assignees

Comments

@staskus
Copy link
Contributor

staskus commented Jul 5, 2023

Parent issue: #20989

There're memory leaks happening that are preventing ViewControllers, Views, Services, and other entities from being released after logging out. Moreover, the allocations and memory usage keep on increasing with each login-logout iteration. The potential causes can be multiple.

Login-logout sequence is just one of the angles to observe memory issues. However, I found it easier to reproduce and scope down.

Note, there's a possibility some of the issues could be resolved with Some notification observers are not removed but by looking at the memory graph I suspect there're many more issues to fix.

Expected behavior

Unused classes should be deallocated after logging out

Actual behavior

Classes are not deallocated after logging out, memory leaking, memory footprint keeps increasing with every login-logout sequence

Steps to reproduce the behavior

  1. Open the fresh app
  2. Observe the Memory Graph, only login-related classes are allocated
  3. Login
  4. Logout
  5. Observe the Memory Graph: Tab Bar, Dashboard, and many other classes are still allocated
  6. Login again
  7. Observe the Memory Graph: notice even more allocations. For example multiple instances of BlogDashboardViewController. Observe memory increasing
Memory Leak 1 3 - Memory skyrocketing Memory Leak 1 2 - Blog Dashboard VC keeps increasing
Tested on [device], iOS [version], Jetpack iOS / WordPress iOS 22.6
@staskus
Copy link
Contributor Author

staskus commented Jul 6, 2023

  • RootViewCoordinator is a singleton
  • RootViewCoordinator holds a strong reference to RootViewPresenter

RootViewPresenter is a protocol, that is implemented by:

  • WPTabBarController for Jetpack app
  • MySiteCoordinator for WordPress app
  • StaticScreensTabBarWrapper for a WP -> JP migration app state

Focusing on the Jetpack app case, the result is as this:
RootViewCoordinator is a singleton that holds a strong reference to RootViewPresenter that holds a strong reference to WPTabBarController. Even when WPTabBarController is dismissed during logout, the reference continues to be held.

Expected behavior

A strong reference to ViewController ideally should be held by a root view controller (or window) so that when the navigation stack is dismissed, child view controllers would be dismissed as well.

In this case, it's a problem that WPTabBarController is both a presenter and it's own root view controller:

extension WPTabBarController: RootViewPresenter {
    var rootViewController: UIViewController {
        return self
    }
}

however, in the case of MySiteCoordinator, it holds a viewController instead of being it's own root view controller:

extension MySitesCoordinator: RootViewPresenter {
    var currentViewController: UIViewController? {
        return rootViewController
    }
}

Somewhere in this cycle, we need to have a weak reference so view controllers would get deallocated when they are removed from the navigation stack.

@staskus
Copy link
Contributor Author

staskus commented Jul 10, 2023

Additional information

After breaking strong references to a WPTabBarController there're still circular references holding the dashboard, and related views.

The most usual cases are:

Assigning a function to a closure

In this case readerContentFactory parameter accepts the closure which starts to hold self strongly. We need to explicitly capture self weakly and then call the method.

 let viewModel = ReaderTabViewModel(readerContentFactory: makeReaderContentViewController(with:))

More info

Capturing self strongly in a closure

 let allPagesAction = UIAction(title: Strings.allPages,
                                      image: Style.allPagesImage,
                                      handler: { _ in self.showPagesList(source: .contextMenu) })
        })

Passing a view controller to a closure that already holds a reference to this object

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

This is a tricky example. It's not obvious that a circular dependency cycle could be created here. However, since sourceController already holds a strong reference to this object, by capturing sourceController we prevent self from being released. The solution is to capture sourceController weakly.

Holding onto the ViewController strongly in a view that is presented by the given ViewController

final class DashboardBlazeCardCell: DashboardCollectionViewCell {
    <...>
    private var viewController: BlogDashboardViewController?
    <...>
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants