AccessibleNumberSpinner memory leak

[pixelzoom]

Related to changes made to NumberSpinner and AccessibleNumberSpinner in https://github.com/phetsims/sun/issues/804 and https://github.com/phetsims/sun/issues/869.

Those changes appear to be have introduced a memory leak, which is manifesting in graphing-lines RC testing, see https://github.com/phetsims/graphing-lines/issues/153.

Graphing Lines creates and disposes NumberPicker instances. And NumberPicker uses the AccessibleNumberSpinner trait, which does this:

      this.setPDOMAttribute( 'aria-roledescription', SunStrings.a11y.numberSpinnerRoleDescriptionStringProperty );

... and then never unlinks from SunStrings.a11y.numberSpinnerRoleDescriptionStringProperty in dispose.

@jessegreenberg How do we unlink from this LocalizedStringProperty?

[pixelzoom]

More details from Slack#dev ...

ParallelDOM is the superclass of Node, and disposeParallelDOM is supposed to remove all PDOM attributes:

    // 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 removePDOMAttributes was working correctly:

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' );
       };
     }

[jessegreenberg]

I am not seeing a leak related to AccessibleNumberSpinner. I ran graphing-lines with ?fuzz&listenerLimit=600 and I see TinyEmitter listeners max out at 427. I am also seeing the aria-roledescription listener correctly unlinked in ParallelDOM.ts.

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:


[jessegreenberg]

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.

[pixelzoom]

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.

[pixelzoom]

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.