phetsims / calculus-grapher

"Calculus Grapher" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
4 stars 4 forks source link

Memory leak in CalculusGrapherSimulationPreferencesNode #289

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

Identified in https://github.com/phetsims/calculus-grapher/issues/265#issuecomment-1468580273 by @samreid.

Text/RichText in various Preferences controls need to be disposed, because they link to Properties in CalculusGrapherStrings and/or CalculusGrapherSymbols.

pixelzoom commented 1 year ago

In the above commits, I fixed a bunch of omissions in various dispose methods.

A (big) remaining problem is how to properly implement dispose for GraphTypeLabelNode, which is used in NotationControl. The current implementation of GraphTypeLabelNode makes disposing of it similar to the problem of disposing of keyboard help dialogs -- it uses composition instead of inheritance to create its subcomponents.

pixelzoom commented 1 year ago

I handled disposal of GraphTypeLabelNode by using the composition dispose pattern that I previously suggested in the issue about Disposable. The general pattern is:

function blah(): Node {

  // needs to be disposed
   const node1 = new SomeNode(...);

  // needs to be disposed
   const node2 = new SomeOtherNode(...);

   const returnNode = new Node( {
     children: [ node1, node2 ]
   } );

  // When returnNode is disposed, dispose of anything else that
  // this function created that needs to be disposed.
  returnNode.disposeEmitter.addListener( () => {
    node1.dispose();
    node2.dispose();
  } );

  return returnNode;
}

Tested in phet and phet-io brands, and looks OK to me. So I'm going to close.