-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 cycle in GutenbergViewController #22265
Conversation
📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @kean! I couldn't reproduce the crash when following the steps in #20647 (comment). I was able to reproduce the crash using when rolling back to 4fc94b6. 🚀
@@ -133,13 +133,11 @@ class PrepublishingViewController: UITableViewController { | |||
/// Toggles `keyboardShown` as the keyboard notifications come in | |||
private func configureKeyboardToggle() { | |||
NotificationCenter.default.publisher(for: UIResponder.keyboardDidShowNotification) | |||
.map { _ in return true } | |||
.assign(to: \.keyboardShown, on: self) | |||
.sink { [weak self] _ in self?.keyboardShown = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make sure I understand it correctly: The cycle was due to passing directly self
rather than using assign
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .assign(to: \.keyboardShown, on: self)
method retains self
, which is a common issue in Combine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the fix Alex!
a491f52
to
b765975
Compare
Generated by 🚫 dangerJS |
Fix some of the occurrences of #20647, namely the ones references in #20647 (comment).
RCA
PrepublishingViewController
had a retain cycle "thanks" toassign(to:)
.When "Tags" screen was presented this property was called for the first time:
I haven't followed the entire memory graph, but this property was evidently indirectly retaining the main
GutenbergViewController
. And becausePrepublishingViewController
had a retain cycle, it was also permanently retaining whatever was referenced bypresentedVC
.To test:
GutenbergViewController/deinit
Regression Testing
Regression Notes
PR submission checklist:
RELEASE-NOTES.txt
if necessary.UI Changes testing checklist: