fix: Mitigate Safari memory leak for input element #4250
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lighthouse scores
What is this?Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on Transfer Size
Request Count
|
Tachometer resultsChromeaction-bar permalinkbasic-test
action-menu permalinktest-basic
test-directive permalink
test-lazy permalink
test-open-close-directive permalink
test-open-close permalink
card permalinktest-basic
color-field permalinkbasic-test
combobox permalinkbasic-test
light-dom-test permalink
illustrated-message permalinktest-basic
menu permalinktest-basic
number-field permalinkbasic-test
overlay permalinkbasic-test
directive-test permalink
element-test permalink
lazy-test permalink
picker permalinkbasic-test
popover permalinktest-basic
search permalinktest-basic
slider permalinktest-basic
split-button permalinkbasic-test
textfield permalinktest-basic
tooltip permalinktest-basic
test-directive permalink
test-element permalink
test-lazy permalink
truncated permalinkbasic-test
Firefoxaction-bar permalinkbasic-test
action-menu permalinktest-basic
test-directive permalink
test-lazy permalink
test-open-close-directive permalink
test-open-close permalink
card permalinktest-basic
color-field permalinkbasic-test
combobox permalinkbasic-test
light-dom-test permalink
illustrated-message permalinktest-basic
menu permalinktest-basic
number-field permalinkbasic-test
overlay permalinkbasic-test
directive-test permalink
element-test permalink
lazy-test permalink
picker permalinkbasic-test
popover permalinktest-basic
search permalinktest-basic
slider permalinktest-basic
split-button permalinkbasic-test
textfield permalinktest-basic
tooltip permalinktest-basic
test-directive permalink
test-element permalink
test-lazy permalink
truncated permalinkbasic-test
|
Rajdeepc
requested changes
Apr 11, 2024
|
|
||
| public override disconnectedCallback(): void { | ||
| // Cleanup form event listener and remove form element from DOM | ||
| this.form.removeEventListener('submit', this.handleSubmit.bind(this)); |
There was a problem hiding this comment.
you should also check if the form element exists before trying to remove the event listener and remove the form element.
|
|
||
| protected override firstUpdateAfterConnected(): void { | ||
| super.firstUpdateAfterConnected(); | ||
| this.form.addEventListener('submit', this.handleSubmit.bind(this)); |
There was a problem hiding this comment.
Instead of binding the event listener in the firstUpdateAfterConnected method, you can bind it directly in the connectedCallback method. This simplifies the code and makes it easier to understand.
There was a problem hiding this comment.
In connectedCallback I don't have access to the form element because it hasn't been created yet. My first option was to add the listener in firstUpdated where the form element has been created but in that case it won't be called again after a disconnect
packages/search/src/Search.ts
Outdated
| @@ -98,15 +130,28 @@ export class Search extends Textfield { | |||
| ); | |||
| } | |||
|
|
|||
| private _manageClearButtonListeners(): void { | |||
There was a problem hiding this comment.
This can be better written like
private _manageClearButtonListeners(): void {
if (this.clearButton) {
if (!this.clearButtonAbortController) {
this.clearButtonAbortController = new AbortController();
const { signal } = this.clearButtonAbortController;
this.clearButton.addEventListener('keydown', stopPropagation, { signal });
}
} else {
if (this.clearButtonAbortController) {
this.clearButtonAbortController.abort();
this.clearButtonAbortController = undefined;
}
}
}There was a problem hiding this comment.
Will update, thanks for the suggestion
packages/search/src/Search.ts
Outdated
| this.formAbortController?.abort(); | ||
| this.clearButtonAbortController?.abort(); | ||
| this.clearButtonAbortController = undefined; | ||
| this.form.remove(); |
There was a problem hiding this comment.
It's generally a good practice to perform cleanup operations in the reverse order of initialisation. So, you might want to remove event listeners before aborting controllers and removing elements.
There was a problem hiding this comment.
Calling formAbortController.abort() removes all the event listeners that were created using that controller's signal. You can read more about that here https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#signal
packages/search/src/Search.ts
Outdated
| protected override firstUpdateAfterConnected(): void { | ||
| super.firstUpdateAfterConnected(); | ||
|
|
||
| this.formAbortController = new AbortController(); |
There was a problem hiding this comment.
Instead of creating a new AbortController instance every time the method is called, you can initialize it once in the constructor or connectedCallback and reuse it
There was a problem hiding this comment.
I can't do that because once the controller.abort() method has been called its signal can't be reused to add the event listeners again. So the current flow is as follows:
- on first update after
connectedI create an instance ofAbortController - I use the controller's
signalto add the form event listeners - the form event listeners are removed on
disconnectedCallbackby callingcontroller.abort()
If I try to reuse the same controller and let's say I reparent the element, after the element is moved the form event listeners won't be re-created.
packages/search/src/Search.ts
Outdated
| private _manageClearButtonListeners(): void { | ||
| // add clear button listener when button is added to the DOM | ||
| if (this.clearButton && !this.clearButtonAbortController) { | ||
| this.clearButtonAbortController = new AbortController(); |
There was a problem hiding this comment.
Same here you are recreating this controller everytime this method reinstantiates.
There was a problem hiding this comment.
As mentioned previously I can't reuse the same controller. If it seems like a significant issue I can remove the AbortController and instead use add/removeEventListener API along with a flag to check that the listeners have been added to achieve the same result
@Rajdeepc Not sure exactly what you mean with this abstraction since AbortController is a browser API. Could you further explain this or offer an example?
I will update the test env I used to reflect the latest changes from this PR and get back with an answer. Thanks for taking the time to check this out |
loredanaspataru
force-pushed
the
lspataru/safari-input-leaks
branch
from
a5e3160 to
23c4197
Compare
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is an attempt to reduce the memory leak impact caused by the Safari bug with HTML input elements that aren't properly garbage collected.
addEventListenerAPI instead of the lit declarative waydisconnectedCallbackThe PR should be treated as POC to showcase the changes we need in SWC and to confirm the fix works as expected on our side.
Related issue(s)
Motivation and context
How has this been tested?
Screenshots (if appropriate)
Here's a screenshot that compares DOM trees that use
sp-search; left one is current SWC behavior, right one is patched using the changes in this PR. After detaching child nodes from their parent element, the ones in red are garbage collected, while the remaining ones are retained.Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.