-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: Prevent GutenbergKit retain cycle #24936
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
base: trunk
Are you sure you want to change the base?
Conversation
The memory leak likely leads to performance degradations and crashes.
|
|
App Name | Jetpack | |
Configuration | Release-Alpha | |
Build Number | 29457 | |
Version | PR #24936 | |
Bundle ID | com.jetpack.alpha | |
Commit | d7071d7 | |
Installation URL | 5jtepeg1id7vg |
|
App Name | WordPress | |
Configuration | Release-Alpha | |
Build Number | 29457 | |
Version | PR #24936 | |
Bundle ID | org.wordpress.alpha | |
Commit | d7071d7 | |
Installation URL | 7g3jch06u6qpo |
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.
Pull Request Overview
This PR fixes a memory leak in GutenbergKit by preventing a retain cycle that was preventing proper disposal of WebViews. The fix addresses strong reference cycles in the Autosaver and improves cleanup of view controllers and tasks during deinitialization.
- Changed Autosaver closure to use weak self reference to break retain cycle
- Added proper cleanup of loading tasks and child view controllers in deinit
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
tearDownKeyboardObservers() | ||
|
||
// Cancel any pending tasks | ||
editorLoadingTask?.cancel() |
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.
This line won't do anything because editorLoadingTask
retains self
, so there is no way deinit
can get called before the task finishes.
editorLoadingTask?.cancel() | ||
|
||
// Clean up child view controller | ||
editorViewController.willMove(toParent: nil) |
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.
It looks like a no-op as it happens automatically when the parent is removed.
Description
Close CMM-834.
It appears the strong
Autosaver
references and, possibly, lack of cleaning up asset loading tasks and view elements led to a retain cycle. The retain cycle prevented proper disposal of WebViews. The memory leak likely leads to performance degradations and crashes.Testing instructions
1. Verify pending changes are persisted via autosave mechanism
2. Verify WebViews are disposed upon editor close