Fix retain cycles preventing objects from being deallocated after logging out #21047

staskus · 2023-07-10T20:29:34Z

Memory is not deallocated after logging out.

It is caused by 2 main issues:

WPTabBarController held strongly by singleton RootViewCoordinator after logging out (see more details)
Dashboard, Reader, Notification, and other initial view controllers and dependencies that have retain cycles preventing them from releasing (see more details)

Solution

To address issues in RootViewCoordinator I decided to limit the changes as much as possible to avoid cascading code changes throughout the codebase. For now, I decided to manually release a reference to RootViewPresenter (WPTabBarController) once logging out. See more info in commit message.

⚠️ Other issues not addressed by this PR

I noticed that the memory footprint remains high after logout, it may be more related to stores or services, I need to investigate more
There's a significant number of memory leaks in the app. This login -> logout case helps to quickly reproduce and confirm whether the issues with initial VCs are fixed but I think we'll need to create more tasks to go through all the main flows
There looks to be a memory leak with Login VCs as well, they are allocated and not deallocated when logging in and logging out as well Memory not deallocated after logging in #21075

To test:

Login & Logout (On both Jetpack and WordPress apps)

Login
Logout
3 Check Memory Graph
WPTabBarController and other non-login related view controllers should be deallocated

Regression: WordPress without static posters

Open WordPress
Enabled all the phases
Relaunch application
Confirm that no tabs are displayed and navigation in menu works

Regression: WordPress with static posters

Open WordPress
Enable static posters phase
Relaunch application
Confirm that tabs are displayed but Reader and Notifications tab is showing static poster

Regression: Dashboard Blaze card

Open Jetpack
Log into the .com site with a post
Select "Promote your content with Blaze" card
Confirm that navigation to Blaze controller is working

Regression: Dashboard Quick Actions

Open Jetpack
Log into the account
Select "Stats, "Posts", "Pages", "Media" buttons
Confirm that navigation works

Regression: Dashboard navigation

Open Jetpack
Confirm site picker, home, and menu navigation works

Regression: Reader navigation

Open Jetpack
Select Reader tab
Confirm opening Post works
Confirm opening Search works

Regression Notes

Potential unintended areas of impact

Breaking the currently existing functionality, especially root navigation

What I did to test those areas of impact (or what existing automated tests I relied on)

Unit test suite
UI test suite
Manual testing

What automated tests I added (or what prevented me from doing so)

None for now but It may be worth considering about performance tests or tests confirming the deallocation of main VCs

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

…ardCell

…mptCardCell

…ick action cell

…shboardStatsCardCell

…MySiteViewController

…tain cycle

staskus · 2023-07-10T20:47:38Z

@guarani Just informing you that it's still in progress, although most of the classes are finally released after the logout:

The work still requires thorough testing since it involves architectural changes at RootViewCoordinator
There's a significant number of memory leaks in the app. This login -> logout case helps to quickly reproduce and confirm whether the issues are fixed but I think we'll need to create more tasks to go through all the main flows
Even when dozens of VCs, Views, and related classes are deallocated after logout, the memory footprint in the app still didn't decrease. I haven't dug deep but it feels like some blog or site-related information is cached and kept in memory after logout. Our Stores and some services are either singletons or held by singletons which doesn't allow them to be released automatically after logout

wpmobilebot · 2023-07-10T20:54:46Z

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr21047-d1358ae
	Version	22.8
	Bundle ID	org.wordpress.alpha
	Commit	`d1358ae`
	App Center Build	WPiOS - One-Offs #6341

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2023-07-10T20:56:42Z

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr21047-d1358ae
	Version	22.8
	Bundle ID	com.jetpack.alpha
	Commit	`d1358ae`
	App Center Build	jetpack-installable-builds #5367

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

…ing-out

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.

guarani

I only did a partial review since you mention this isn't ready yet. I just went through the code changes but didn't test it. Once it's ready I can dive deeper and actually test the changes, let me know if this works for you.

I left some comments with first impressions, but they're things I might be able to answer myself after spending more time review and testing.

RELEASE-NOTES.txt

WordPress/Classes/ViewRelated/Blog/Blog Dashboard/Cards/Prompts/DashboardPromptsCardCell.swift

WordPress/Classes/ViewRelated/Blog/Blog Dashboard/ViewModel/BlogDashboardViewModel.swift

…ing-out

…logout - Made WordPressAuthenticator injectable - Checking if rootViewController is deinitialized after presenting signInUI

…ing-out

guarani

Login & Logout (On both Jetpack and WordPress apps)

Login

Logout
3 Check Memory Graph

WPTabBarController and other non-login related view controllers should be deallocated

Here the app's starting memory was 155 MB, went to 313 MB after logging in, and stayed high (323 MB) after logging out (the increase is explained by navigating to the log out screen). I saw you mention how there's many other retain cycles that are out of scope of this PR and that could explain this. I checked the Memory Graph and indeed WPTabBarController is no longer there (this was while testing the Jetpack app), which is a good sign.

This PR does appear to fix a retain cycle, but it might not be reducing the app's memory consumption overall. If so, putting this in the release notes as I suggested earlier might more sense once we've addressed some of the other retain cycles. What do you think?

It's not easy to grok how the window management works (especially across the two apps + the migration flow). However, I think the approach is promising. I'm happy to approve this once I can spend more time on the regression testing tomorrow and can get a bit more confidence in the fix. On a side note, splitting this into multiple PRs could be an option so that if for example a change needs to be reverted, we won't need to revert all these separate fixes.

RELEASE-NOTES.txt

WordPress/Classes/System/RootViewCoordinator.swift

WordPress/WordPressTest/RootViewCoordinatorTests.swift

staskus · 2023-07-13T06:53:45Z

Thanks for the review, @guarani !

This PR does appear to fix a retain cycle, but it might not be reducing the app's memory consumption overall. If so, putting this in the release notes as I suggested earlier might more sense once we've addressed some of the other retain cycles. What do you think?

Yes, you're right. I had the expectation of fixing these retain cycles to clean the memory but there're more issues that I will address with future PRs. I didn't want to make this one too big. I will update the release notes.

On a side note, splitting this into multiple PRs could be an option so that if for example a change needs to be reverted, we won't need to revert all these separate fixes.

A lot of the small retain cycle improvements include retain cycles with BlogDashboardViewController, so only fixing all of them change anything fundamentally. However, we could split RootViewCoordinator fixes and all the retain cycle fixes in 2 PRs if needed.

…ing-out

guarani

Finished manual testing and everything is working as before.

Tested on iPhone:

Login & Logout (On both Jetpack and WordPress apps). Tested:
- Tested WP app
- Tested WP to JP app migration
- Tested JP app
Regression test: WordPress without static posters
Regression test: WordPress with static posters
Regression test: Dashboard Blaze card

Tested on iPad:

Regression test: Dashboard Quick Actions
Regression test: Dashboard navigation
Regression test: Reader navigation

The code changes look good and I think this is ready to merge 👍

staskus added 10 commits July 10, 2023 23:14

Set weak reference for BlogDashboardViewController in DashboardBlazeC…

4791b6f

…ardCell

Weakly capture action closure in DashbordPagesListCardCell

692c523

Pass weak closure instead of strongly captured method in DashboardPro…

9ec904c

…mptCardCell

Break circular dependency cycle for BlogDashboardViewController in qu…

a7f12dd

…ick action cell

Break circular dependency cycle for BlogDashboardViewController in Da…

db45915

…shboardStatsCardCell

Capture BlogDashboardViewController weekly in BlogDashboardViewModel

857430c

Break retain cycle by capturing presented view controllers weakly in …

c90bfdf

…MySiteViewController

Weak self in NotificationsViewController

a30d50c

Capture ReaderTabViewModel weakly in a closure

7cff19a

Use weak closures instead of passing methods to a closure to avoid re…

03f3598

…tain cycle

staskus added [Type] Bug Login Logged-out labels Jul 10, 2023

staskus added 3 commits July 11, 2023 12:55

Merge branch 'trunk' into fix/21010-memory-not-deallocated-after-logg…

d71ac66

…ing-out

Update RELEASE-NOTES.txt

daf8e08

staskus force-pushed the fix/21010-memory-not-deallocated-after-logging-out branch from 880e34c to daf8e08 Compare July 11, 2023 10:28

staskus added this to the 22.9 milestone Jul 11, 2023

staskus requested a review from guarani July 11, 2023 11:24

staskus marked this pull request as ready for review July 11, 2023 11:26

staskus added the Memory Management label Jul 11, 2023

guarani reviewed Jul 11, 2023

View reviewed changes

RELEASE-NOTES.txt Outdated Show resolved Hide resolved

WordPress/Classes/ViewRelated/Blog/Blog Dashboard/Cards/Prompts/DashboardPromptsCardCell.swift Show resolved Hide resolved

WordPress/Classes/ViewRelated/Blog/Blog Dashboard/ViewModel/BlogDashboardViewModel.swift Show resolved Hide resolved

staskus added 4 commits July 12, 2023 11:52

Merge branch 'trunk' into fix/21010-memory-not-deallocated-after-logg…

7d48ab1

…ing-out

Update RELEASE-NOTES.txt

8d048fe

Added unit tests confirming deallocation of rootViewController after …

3ce5076

…logout - Made WordPressAuthenticator injectable - Checking if rootViewController is deinitialized after presenting signInUI

Merge branch 'trunk' into fix/21010-memory-not-deallocated-after-logg…

b53d04f

…ing-out

staskus requested a review from guarani July 12, 2023 12:51

guarani reviewed Jul 13, 2023

View reviewed changes

RELEASE-NOTES.txt Show resolved Hide resolved

WordPress/Classes/System/RootViewCoordinator.swift Show resolved Hide resolved

WordPress/WordPressTest/RootViewCoordinatorTests.swift Show resolved Hide resolved

staskus changed the title ~~Fix memory not deallocated after logging out~~ Fix retain cycles preventing classes from being deallocated after logging out Jul 13, 2023

staskus changed the title ~~Fix retain cycles preventing classes from being deallocated after logging out~~ Fix retain cycles preventing objects from being deallocated after logging out Jul 13, 2023

staskus added 2 commits July 13, 2023 10:30

Update RELEASE-NOTES.txt

de729ae

Merge branch 'trunk' into fix/21010-memory-not-deallocated-after-logg…

d1358ae

…ing-out

staskus requested a review from guarani July 13, 2023 07:33

guarani approved these changes Jul 13, 2023

View reviewed changes

staskus merged commit cb5ccda into trunk Jul 14, 2023

staskus deleted the fix/21010-memory-not-deallocated-after-logging-out branch July 14, 2023 08:55

staskus mentioned this pull request Jul 17, 2023

Break retain cycle between MySiteViewController and CompliancePopoverCoordinator #21111

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix retain cycles preventing objects from being deallocated after logging out #21047

Fix retain cycles preventing objects from being deallocated after logging out #21047

staskus commented Jul 10, 2023 •

edited

Loading

staskus commented Jul 10, 2023

wpmobilebot commented Jul 10, 2023 •

edited

Loading

wpmobilebot commented Jul 10, 2023 •

edited

Loading

guarani left a comment

guarani left a comment

staskus commented Jul 13, 2023 •

edited

Loading

guarani left a comment

Fix retain cycles preventing objects from being deallocated after logging out #21047

Fix retain cycles preventing objects from being deallocated after logging out #21047

Conversation

staskus commented Jul 10, 2023 • edited Loading

Solution

⚠️ Other issues not addressed by this PR

To test:

Login & Logout (On both Jetpack and WordPress apps)

Regression: WordPress without static posters

Regression: WordPress with static posters

Regression: Dashboard Blaze card

Regression: Dashboard Quick Actions

Regression: Dashboard navigation

Regression: Reader navigation

Regression Notes

staskus commented Jul 10, 2023

wpmobilebot commented Jul 10, 2023 • edited Loading

wpmobilebot commented Jul 10, 2023 • edited Loading

guarani left a comment

Choose a reason for hiding this comment

guarani left a comment

Choose a reason for hiding this comment

Login & Logout (On both Jetpack and WordPress apps)

staskus commented Jul 13, 2023 • edited Loading

guarani left a comment

Choose a reason for hiding this comment

staskus commented Jul 10, 2023 •

edited

Loading

wpmobilebot commented Jul 10, 2023 •

edited

Loading

wpmobilebot commented Jul 10, 2023 •

edited

Loading

staskus commented Jul 13, 2023 •

edited

Loading