-
Notifications
You must be signed in to change notification settings - Fork 13.4k
fix(tab-bar): prevent keyboard controller memory leak on rapid mount/unmount #30906
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| } | ||
|
|
||
| disconnectedCallback() { | ||
| if (this.keyboardCtrlPromise) { |
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.
Maybe this doesn't need to be done on this PR, but I feel like we do this same code in so many places that we should consider making it a utility.
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 this particular pattern is only specifically done in tab bar and footer (unless I'm missing something?), so I'm not sure if that's worth extracting right now
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.
I mostly meant the code that is destroying like this:
ionic-framework/core/src/components/select/select.tsx
Lines 376 to 379 in 1c89cf0
| if (this.notchController) { | |
| this.notchController.destroy(); | |
| this.notchController = undefined; | |
| } |
We also disconnect in a lot of places:
ionic-framework/core/src/components/select/select.tsx
Lines 382 to 385 in 1c89cf0
| if (this.validationObserver) { | |
| this.validationObserver.disconnect(); | |
| this.validationObserver = undefined; | |
| } |
It seems like we could make this into a utility or something, but that's not an issue for your PR.
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.
Let's make a ticket to circle back on adding the utility.
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.
@thetaPC Done! This will be FW-7030
Issue number: resolves internal
What is the current behavior?
When
ion-tab-baris rapidly mounted and unmounted, a race condition in connectedCallback can cause the keyboard controller to be created after the component has been disconnected. This results in orphaned event listeners (keyboardWillShow,keyboardWillHide) on the window object that are never cleaned up, causing a memory leak.What is the new behavior?
The keyboard controller is now properly destroyed in all scenarios:
The promise tracking pattern ensures only the most recent async operation assigns its result
Does this introduce a breaking change?
Other information
Current dev build:
I was unable to find a way to create tests that accurately identified if this problem was occurring. Memory leaks are notoriously difficult to created automated tests for. I ultimately removed my previous attempts because I didn't want to give a false sense of security.