Memory leaks in ButtonNode

[pixelzoom]

In https://github.com/phetsims/sun/commit/2ec57074ff1e4cf035cee8bbeeceaed2278f403d for https://github.com/phetsims/sun/issues/658, @zepumph added _settableBaseColorProperty: PaintColorProperty.

In https://github.com/phetsims/sun/commit/53b034e899de573d867b14726653f9aa957f3976 for https://github.com/phetsims/balancing-act/issues/123, @jonathanolson added _disabledColorProperty: PaintColorProperty.

The end result was this bit of code at ButtonNode.ts line 158:

    this._settableBaseColorProperty = new PaintColorProperty( options.baseColor );
    this._disabledColorProperty = new PaintColorProperty( options.disabledColor );

    this.baseColorProperty = new DerivedProperty( [
      this._settableBaseColorProperty,
      this.enabledProperty,
      this._disabledColorProperty
    ], ( color, enabled, disabledColor ) => {
      return enabled ? color : disabledColor;
    } );

Neither of these Properties is currently disposed of when ButtonNode dispose is called. PaintColorProperty's constructor argument is a TPaint, and if it happens to be a Property, like this example in unit-rates ShoppingQuestionNode.ts:

    const editButton = new RectangularPushButton( {
      baseColor: URColors.editButtonColorProperty,
      ...
    } );

... then the result in a memory leak, like the one in https://github.com/phetsims/unit-rates/issues/226:

[pixelzoom]

Fixed in https://github.com/phetsims/sun/commit/e15b8fa70d06e16af0995c91fcc737452a079ae0. @zepumph and/or @jonathanolson please review with high priority. Do any sims require an MR for this?

[pixelzoom]

There seems to be another leak that is showing up in https://github.com/phetsims/unit-rates/issues/226 as a large number of RadialGradients. In StartStopButton.ts, I have:

    // adjust button color based on runningProperty
    // unlink not needed, exists for sim lifetime
    runningProperty.link( running => {
      this.baseColor = ( running ? '#6D6E70' : '#85d4a6' );
    } );

In ButtonNode.ts:

  /**
   * Sets the base color, which is the main background fill color used for the button.
   */
  public setBaseColor( baseColor: TColor ): void { this._settableBaseColorProperty.paint = baseColor; }

  public set baseColor( baseColor: TColor ) { this.setBaseColor( baseColor ); }

When calling setBaseColor, is the previous baseColor and associated gradients being cleaned up?

[jonathanolson]

The fix https://github.com/phetsims/sun/commit/e15b8fa70d06e16af0995c91fcc737452a079ae0 looks good.

It looks like if someone provided a color Property as a baseColor or disabledColor of a button, it would leak memory.

@kathy-phet thoughts on MR for this? Committed in late 2020, but it seems like sims passed RC memory tests, so presumably it hasn't significantly affected anything.

[jonathanolson]

When calling setBaseColor, is the previous baseColor and associated gradients being cleaned up?

I'll look into it. It SHOULD presumably be cleaning up, but likely something is going wrong.

[kathy-phet]

@jonathanolson - It's not clear to me, can you address this memory leak in the common code?

From @pixelzoom -

There seems to be another leak that is showing up in https://github.com/phetsims/unit-rates/issues/226 as a large number of RadialGradients.

[jonathanolson]

It's not clear to me, can you address this memory leak in the common code?

Yes, it should be patchable in common code.

There seems to be another leak that is showing up in https://github.com/phetsims/unit-rates/issues/226 as a large number of RadialGradients.

Yes, will be on it.

[jonathanolson]

I think the RadialGradient leak was actually https://github.com/phetsims/unit-rates/issues/226#issuecomment-2215493617 (since the CountMaps would hold onto paints).

[pixelzoom]

Thanks @jonathanolson, that makes sense. Closing.

[pixelzoom]

Reopening.

@jonathanolson had asked:

@kathy-phet thoughts on MR for this? Committed in late 2020, but it seems like sims passed RC memory tests, so presumably it hasn't significantly affected anything.

... so I'll leave this to @jonathanolson and @kathy-phet to work out whether an MR is needed for the ButtonNode leak that I fixed in https://github.com/phetsims/sun/commit/e15b8fa70d06e16af0995c91fcc737452a079ae0.