-
Notifications
You must be signed in to change notification settings - Fork 12
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
AccessibleNumberSpinner memory leak #879
Comments
More details from Slack#dev ... ParallelDOM is the superclass of Node, and // PDOM attributes can potentially have listeners, so we will clear those out.
this.removePDOMAttributes(); But that does not seem to be working. This uncommitted patch in AccessibleNumberSpinner resolves the memory leak, and should not be necessary if Subject: [PATCH] various memory leaks related to LocalizedStringProperty and ProfileColorProperty, https://github.com/phetsims/graphing-lines/issues/153
---
Index: js/accessibility/AccessibleNumberSpinner.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/AccessibleNumberSpinner.ts b/js/accessibility/AccessibleNumberSpinner.ts
--- a/js/accessibility/AccessibleNumberSpinner.ts (revision 75e22745c05481d1d6b3e80c8d7d2138563df27a)
+++ b/js/accessibility/AccessibleNumberSpinner.ts (date 1712621584033)
@@ -180,6 +180,7 @@
this.pdomDecrementDownEmitter.dispose();
this.removeInputListener( accessibleInputListener );
+ this.removePDOMAttribute( 'aria-roledescription' );
};
} |
I am not seeing a leak related to AccessibleNumberSpinner. I ran graphing-lines with For ~4 minutes of fuzzing, memory was stable at 87 MB. But after about 4 minutes of fuzzing, the listener count started to go up quickly. ( I don't know why, maybe I clicked into the sim to turn on sound?) The limit assertion stopped here: https://github.com/phetsims/tambo/blob/ebe910a6ea4b9403e4e554125967455a518cef12/js/sound-generators/SoundGenerator.ts#L115 Stack trace goes through RichDragListener: |
For graphing-lines, it does look like RichDragListeners can be created and not disposed for the 'reward' views. So I probably started to see the leak after a while because the fuzzer happened to win a game. Removing my assignment for now. |
Thanks @jessegreenberg. It’s highly probable that I was confused about leakage in AccessibleNumberSpinner/ParallelDOM. There were so many things leaking in Graphing Lines due to LocalizedStringProperty. Thanks for looking, and I’ll continue to investigate. |
I confirmed that AccessibleNumberSpinner's 'aria-roledescription' listener is correctly unlinked in ParallelDOM.ts. Weird, not what I was seeing yesterday. Sorry to bother you. Closing. |
Related to changes made to NumberSpinner and AccessibleNumberSpinner in #804 and #869.
Those changes appear to be have introduced a memory leak, which is manifesting in graphing-lines RC testing, see phetsims/graphing-lines#153.
Graphing Lines creates and disposes NumberPicker instances. And NumberPicker uses the AccessibleNumberSpinner trait, which does this:
... and then never unlinks from
SunStrings.a11y.numberSpinnerRoleDescriptionStringProperty
in dispose.@jessegreenberg How do we unlink from this LocalizedStringProperty?
The text was updated successfully, but these errors were encountered: