Memory leaks in ButtonNode

[pixelzoom]

In 2ec5707 for #658, @zepumph added _settableBaseColorProperty: PaintColorProperty.

In 53b034e for phetsims/balancing-act#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 phetsims/unit-rates#226:

[pixelzoom]

Fixed in e15b8fa. @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 phetsims/unit-rates#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 e15b8fa 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 phetsims/unit-rates#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 phetsims/unit-rates#226 as a large number of RadialGradients.

Yes, will be on it.

[jonathanolson]

I think the RadialGradient leak was actually phetsims/unit-rates#226 (comment) (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 e15b8fa.