Change a few delegate properties in Signup to be weak references by crazytonyli · Pull Request #21064 · wordpress-mobile/WordPress-iOS

crazytonyli · 2023-07-11T23:01:02Z

Relates to #21050.

This PR changed a few delegate properties' declaration. I'll post PR comments in detail regarding those individual changes.

Regression Notes

Potential unintended areas of impact
None.
What I did to test those areas of impact (or what existing automated tests I relied on)
None.
What automated tests I added (or what prevented me from doing so)
None.

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.

UI Changes testing checklist: N/A

crazytonyli · 2023-07-11T23:02:17Z

WordPress/Classes/ViewRelated/NUX/SignupEpilogueCell.swift


    private var cellType: EpilogueCellType?
-    open var delegate: SignupEpilogueCellDelegate?
+    open weak var delegate: SignupEpilogueCellDelegate?


There is a retain cycle between the SignupEpilogueTableViewController and this SignupEpilogueCell.

WordPress-iOS/WordPress/Classes/ViewRelated/NUX/SignupEpilogueTableViewController.swift

Line 214 in 48b8a39

cell.delegate = self

crazytonyli · 2023-07-11T23:04:59Z

WordPress/Classes/ViewRelated/NUX/SignupEpilogueTableViewController.swift

-    open var dataSource: SignupEpilogueTableViewControllerDataSource?
-    open var delegate: SignupEpilogueTableViewControllerDelegate?
+    open weak var dataSource: SignupEpilogueTableViewControllerDataSource?
+    open weak var delegate: SignupEpilogueTableViewControllerDelegate?


WordPress-iOS/WordPress/Classes/ViewRelated/NUX/SignupEpilogueViewController.swift

Lines 71 to 79 in 48b8a39

override func prepare(for segue: UIStoryboardSegue, sender: Any?) {

super.prepare(for: segue, sender: sender)

if let vc = segue.destination as? SignupEpilogueTableViewController {

vc.credentials = credentials

vc.socialService = socialService

vc.dataSource = self

vc.delegate = self

}

I'm not sure if a view controller would hold strong references to segue instances, which would cause a retain cycle in our case here. But considering how this delegate and dataSource are used, they should be weak references anyways.

Yes, I jumped on this memory leak in other investigations.

crazytonyli · 2023-07-11T23:08:21Z

WordPress/Classes/ViewRelated/NUX/SignupUsernameViewController.swift

    open var currentUsername: String?
    open var displayName: String?
-    open var delegate: SignupUsernameViewControllerDelegate?
+    open weak var delegate: SignupUsernameViewControllerDelegate?


This delegate is used in a similar way as #21064 (comment)

WordPress-iOS/WordPress/Classes/ViewRelated/NUX/SignupEpilogueViewController.swift

Lines 80 to 88 in 48b8a39

if let vc = segue.destination as? SignupUsernameViewController {

vc.currentUsername = updatedUsername ?? epilogueUserInfo?.username

vc.displayName = updatedDisplayName ?? epilogueUserInfo?.fullName

vc.delegate = self

// Empty Back Button

navigationItem.backBarButtonItem = UIBarButtonItem(title: String(), style: .plain, target: nil, action: nil)

}

}

crazytonyli · 2023-07-11T23:09:26Z

WordPress/Classes/ViewRelated/NUX/SignupUsernameTableViewController.swift

    open var currentUsername: String?
    open var displayName: String?
-    open var delegate: SignupUsernameViewControllerDelegate?
+    open weak var delegate: SignupUsernameViewControllerDelegate?


There is a retain cycle between SignupUsernameViewController and SignupUsernameTableViewController.

WordPress-iOS/WordPress/Classes/ViewRelated/NUX/SignupUsernameViewController.swift

Lines 31 to 36 in 48b8a39

if let vc = segue.destination as? SignupUsernameTableViewController {

usernamesTableViewController = vc

vc.delegate = self

vc.displayName = displayName

vc.currentUsername = currentUsername

}

wpmobilebot · 2023-07-11T23:26:47Z

📲 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	pr21064-a530793
	Version	22.7
	Bundle ID	org.wordpress.alpha
	Commit	`a530793`
	App Center Build	WPiOS - One-Offs #6308

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

wpmobilebot · 2023-07-11T23:27:26Z

📲 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	pr21064-a530793
	Version	22.7
	Bundle ID	com.jetpack.alpha
	Commit	`a530793`
	App Center Build	jetpack-installable-builds #5331

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

staskus

👍 LGTM

Change a few delegate properties in Signup to be weak references

a530793

crazytonyli added the Memory Management label Jul 11, 2023

crazytonyli added this to the 22.9 milestone Jul 11, 2023

crazytonyli requested review from jkmassel and mokagio July 11, 2023 23:01

crazytonyli self-assigned this Jul 11, 2023

crazytonyli commented Jul 11, 2023

View reviewed changes

staskus mentioned this pull request Jul 14, 2023

WatchdogTermination: The OS watchdog terminated your app, possibly because it overused RAM. #20989

Open

staskus approved these changes Jul 14, 2023

View reviewed changes

staskus mentioned this pull request Jul 14, 2023

Fix retain cycles in Login-related classes #21092

Merged

4 tasks

crazytonyli merged commit 16c3626 into trunk Jul 18, 2023

crazytonyli deleted the weak-delegate-violation-fixes-6 branch July 18, 2023 01:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change a few delegate properties in Signup to be weak references#21064

Change a few delegate properties in Signup to be weak references#21064
crazytonyli merged 1 commit intotrunkfrom
weak-delegate-violation-fixes-6

crazytonyli commented Jul 11, 2023

Uh oh!

crazytonyli Jul 11, 2023

Uh oh!

crazytonyli Jul 11, 2023

Uh oh!

staskus Jul 14, 2023

Uh oh!

crazytonyli Jul 11, 2023

Uh oh!

crazytonyli Jul 11, 2023

Uh oh!

wpmobilebot commented Jul 11, 2023

Uh oh!

wpmobilebot commented Jul 11, 2023

Uh oh!

staskus left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	override func prepare(for segue: UIStoryboardSegue, sender: Any?) {
	super.prepare(for: segue, sender: sender)
	if let vc = segue.destination as? SignupEpilogueTableViewController {
	vc.credentials = credentials
	vc.socialService = socialService
	vc.dataSource = self
	vc.delegate = self
	}

	if let vc = segue.destination as? SignupUsernameViewController {
	vc.currentUsername = updatedUsername ?? epilogueUserInfo?.username
	vc.displayName = updatedDisplayName ?? epilogueUserInfo?.fullName
	vc.delegate = self

	// Empty Back Button
	navigationItem.backBarButtonItem = UIBarButtonItem(title: String(), style: .plain, target: nil, action: nil)
	}
	}

	if let vc = segue.destination as? SignupUsernameTableViewController {
	usernamesTableViewController = vc
	vc.delegate = self
	vc.displayName = displayName
	vc.currentUsername = currentUsername
	}

Conversation

crazytonyli commented Jul 11, 2023

Regression Notes

Uh oh!

crazytonyli Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

crazytonyli Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

staskus Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

crazytonyli Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

crazytonyli Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

wpmobilebot commented Jul 11, 2023

Uh oh!

wpmobilebot commented Jul 11, 2023

Uh oh!

staskus left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants