Memory leak in Carousel?

[zepumph]

Over in https://github.com/phetsims/sun/issues/797, I see that Carousel.carouselItemNodes is created with getGroupItemNodes, but not disposed. I am confident enough that this is a leak that I'll commit and then assign to review.

[zepumph]

@samreid, does this seem right to you? I'm checking other usages of getGroupItemNodes in that other issue, so just a spot check should be good here.

[zepumph]

Ahh shoot, looks like it is done through the AlignBox, I reverted this commit.

This seems really confusing to me, could we instead dispose the alignboxes and then dispose the nodes as two different calls?

https://github.com/phetsims/sun/blob/1e157d8bcac350f304471499d474b4f569d9329e/js/Carousel.ts#L406

[samreid]

I am confident enough that this is a leak that I'll commit and then assign to review.

From this thread and other comments like https://github.com/phetsims/scenery-phet/issues/769#issuecomment-1495060141, I'm not sure the team has a shared consensus about what constitutes a memory leak. Neglecting to dispose a component does not in itself imply a memory leak. There must also be a path from one of the heap roots.

I'm also seeing

      this.alignBoxes.forEach( alignBox => {
        alignBox.children.forEach( child => child.dispose() );
        alignBox.dispose();
      } );

Which was added in:

https://github.com/phetsims/sun/commit/e2074a56bc4a4eedf10e9af3d118c013abcd4747

[zepumph]

Right, that makes sense, but in common code, you don't know if a dependency injected node-creator will create a node that is a memory leak or not, so we need to have an abundance of caution (generally).

Will you please respond to https://github.com/phetsims/sun/issues/841#issuecomment-1501947073?

[samreid]

Right, that makes sense, but in common code, you don't know if a dependency injected node-creator will create a node that is a memory leak or not, so we need to have an abundance of caution (generally).

I agree, and in this case clients can get access to the AlignBox instances via getVisibleAlignBoxes, clients may add listeners that need cleanup.

Will you please respond to https://github.com/phetsims/sun/issues/841#issuecomment-1501947073?

I feel confused since that comment said: "This seems really confusing to me, could we instead dispose the alignboxes and then dispose the nodes as two different calls?" and showed alignBox.children.forEach( child => child.dispose() ); And I already replied that alignBox instances are being disposed in a separate call, and I showed the commit where that was introduced. So I feel I'm misunderstanding something.

[zepumph]

Do you think the code would look better like this? Do you think we need an assertion that the AlignBox only has one child, and that it is one of our Nodes from createNode?

Subject: [PATCH] use new bad-text to keep joist global out of common code (unprotected), https://github.com/phetsims/chipper/issues/1004
---
Index: js/Carousel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/Carousel.ts b/js/Carousel.ts
--- a/js/Carousel.ts    (revision ae7f27c20ca3f465dbf58a5ed32779ed2da2d439)
+++ b/js/Carousel.ts    (date 1681152240732)
@@ -403,11 +403,13 @@
       this.visibleAlignBoxesProperty.dispose();
       this.pageNumberProperty.dispose();
       this.alignBoxes.forEach( alignBox => {
-        alignBox.children.forEach( child => child.dispose() );
+        assert && assert( alignBox.children.length === 1, 'just one child please' );
+        assert && assert( this.carouselItemNodes.includes( alignBox.children[ 0 ] ), 'alignBox should wrap a content node' );
         alignBox.dispose();
       } );
       this.scrollingNode.dispose();
       this.carouselConstraint.dispose();
+      this.carouselItemNodes.forEach( node => node.dispose() );
     };

     this.mutate( options );
@@ -604,7 +606,6 @@
       const lastBox = visibleAlignBoxes[ this.itemsPerPage - 1 ] || visibleAlignBoxes[ visibleAlignBoxes.length - 1 ];

       const horizontalSize = new Dimension2(
-
         // Measure from the beginning of the first item to the end of the last item on the 1st page
         lastBox[ orientation.maxSide ] - visibleAlignBoxes[ 0 ][ orientation.minSide ] + ( 2 * this.margin ),

[samreid]

Looks good, thanks! I also tested by disposing a carousel and seeing that nothing unexpected happened. Anything else for this issue @zepumph ?

[zepumph]

Thanks! I like it.