Property listeners should be removed before Property.dispose is called

[samreid]

See https://github.com/phetsims/axon/issues/77

[samreid]

This issue can be easily seen during the fuzz tester:

Error: Assertion failed: during disposal, expected 0 observers, actual = 2
    at window.assertions.assertFunction (http://localhost/assert/js/assert.js:22:13)
    at DerivedProperty.Property.disposeProperty (http://localhost/axon/js/Property.js?bust=1447479142602:56:17)
    at DerivedProperty.inherit.dispose (http://localhost/axon/js/Property.js?bust=1447479142602:312:14)
    at DerivedProperty.inherit.dispose (http://localhost/axon/js/DerivedProperty.js?bust=1447479142602:60:34)
    at NumberPicker.inherit.dispose (http://localhost/scenery-phet/js/NumberPicker.js?bust=1447479142602:333:30)
    at TermNode.inherit.dispose (http://localhost/balancing-chemical-equations/js/common/view/TermNode.js?bust=1447479142602:63:28)
    at http://localhost/balancing-chemical-equations/js/common/view/EquationNode.js?bust=1447479142602:72:20
    at Array.forEach (native)
    at EquationNode.inherit.updateNode (http://localhost/balancing-chemical-equations/js/common/view/EquationNode.js?bust=1447479142602:71:18)
    at Array. (http://localhost/balancing-chemical-equations/js/common/view/EquationNode.js?bust=1447479142602:52:12)

[samreid]

I'm not too familiar with this sim at all. @pixelzoom are you available to take a look?

[samreid]

Hang on, I think I've figured this out....

[samreid]

The issue is that NumberPicker is adding its own internal listeners but not removing them before dispose. We can fix this if we change the upEnabledProperty.link and downEnabledProperty.link section to read like so:

    // enable/disable listeners: unlink unnecessary, properties are owned by this instance
    this.upEnabledListener = function( enabled ) { upListener.enabled = enabled; }; // @private
    this.upEnabledProperty.link( this.upEnabledListener );
    this.downEnabledListener = function( enabled ) { downListener.enabled = enabled; }; // @private
    this.downEnabledProperty.link( this.downEnabledListener );

store references to the multilinks like so;

    // @private update colors for 'up' components
    this.updateColorsForUpComponentsMultilink = Property.multilink( [ upStateProperty, this.upEnabledProperty ],
      function( state, enabled ) {
        updateColors( state, enabled, upBackground, thisNode.upArrow, backgroundColors, arrowColors );
      }
    );

    // @private update colors for 'down' components
    this.updateColorsForDownComponentsMultilink = Property.multilink( [ downStateProperty, this.downEnabledProperty ], function( state, enabled ) {
      updateColors( state, enabled, downBackground, thisNode.downArrow, backgroundColors, arrowColors );
    } );

and remove them in the dispose like so:

    // @public Ensures that this node is eligible for GC.
    dispose: function() {
      this.upEnabledProperty.unlink( this.upEnabledListener );
      Property.unmultilink( this.updateColorsForUpComponentsMultilink );
      this.upEnabledProperty.dispose();

      this.downEnabledProperty.unlink( this.downEnabledListener );
      Property.unmultilink( this.updateColorsForDownComponentsMultilink );
      this.downEnabledProperty.dispose();

      this.valueProperty.unlink( this.valueObserver );
    },

With those changes, the fuzzer runs nicely for BCE and I did not note any issues with the number spinners (though I am not too familiar with them and it would be good for @pixelzoom to check.)

Here is a snapshot of NumberPicker.js with the changes described above for your convenience (wasn't sure if an issue comment or branch would be more convenient for this case):

// Copyright 2014-2015, University of Colorado Boulder

/**
 * User-interface component for picking a number value from a range.
 * This is essentially a value with integrated up/down spinners.
 * But PhET has been calling it a 'picker', so that's what this type is named.
 *
 * @author Chris Malley (PixelZoom, Inc.)
 */
define( function( require ) {
  'use strict';

  // modules
  var ButtonListener = require( 'SCENERY/input/ButtonListener' );
  var Color = require( 'SCENERY/util/Color' );
  var DerivedProperty = require( 'AXON/DerivedProperty' );
  var Dimension2 = require( 'DOT/Dimension2' );
  var FireOnHoldInputListener = require( 'SCENERY_PHET/buttons/FireOnHoldInputListener' );
  var inherit = require( 'PHET_CORE/inherit' );
  var LinearGradient = require( 'SCENERY/util/LinearGradient' );
  var Node = require( 'SCENERY/nodes/Node' );
  var Path = require( 'SCENERY/nodes/Path' );
  var PhetFont = require( 'SCENERY_PHET/PhetFont' );
  var Property = require( 'AXON/Property' );
  var Rectangle = require( 'SCENERY/nodes/Rectangle' );
  var Shape = require( 'KITE/Shape' );
  var Text = require( 'SCENERY/nodes/Text' );
  var Util = require( 'DOT/Util' );

  /**
   * @param {Property.<number>} valueProperty
   * @param {Property.<Range>} rangeProperty
   * @param {Object} [options]
   * @constructor
   */
  function NumberPicker( valueProperty, rangeProperty, options ) {

    options = _.extend( {
      cursor: 'pointer',
      color: new Color( 0, 0, 255 ), // {Color|string} color of arrows, and top/bottom gradient on pointer over
      backgroundColor: 'white', // {Color|string} color of the background when pointer is not over it
      cornerRadius: 6,
      xMargin: 3,
      yMargin: 3,
      decimalPlaces: 0,
      font: new PhetFont( 24 ),
      upFunction: function() { return valueProperty.get() + 1; },
      downFunction: function() { return valueProperty.get() - 1; },
      timerDelay: 400, // start to fire continuously after pressing for this long (milliseconds)
      timerInterval: 100, // fire continuously at this frequency (milliseconds),
      noValueString: '-', // string to display if valueProperty.get is null or undefined
      align: 'center', // horizontal alignment of the value, 'center'|'right'|'left'
      touchAreaExpandX: 10,
      touchAreaExpandY: 10,
      mouseAreaExpandX: 0,
      mouseAreaExpandY: 5,
      backgroundStroke: 'gray',
      backgroundLineWidth: 0.5,
      arrowHeight: 6,
      arrowYSpacing: 3
    }, options );
    // {Color|string} color of arrows and top/bottom gradient when pressed
    options.pressedColor = options.pressedColor || Color.toColor( options.color ).darkerColor();

    var thisNode = this;
    Node.call( this );

    //------------------------------------------------------------
    // Properties

    this.valueProperty = valueProperty; // @private must be unlinked in dispose

    var upStateProperty = new Property( 'up' ); // up|down|over|out
    var downStateProperty = new Property( 'up' ); // up|down|over|out

    // @private must be detached in dispose
    this.upEnabledProperty = new DerivedProperty( [ valueProperty, rangeProperty ], function( value, range ) {
      return ( value !== null && value !== undefined && value < range.max );
    } );

    // @private must be detached in dispose
    this.downEnabledProperty = new DerivedProperty( [ valueProperty, rangeProperty ], function( value, range ) {
      return ( value !== null && value !== undefined && value > range.min );
    } );

    //------------------------------------------------------------
    // Nodes

    // displays the value
    var valueNode = new Text( '', { font: options.font, pickable: false } );

    // compute max width of text based on value range
    valueNode.text = Util.toFixed( rangeProperty.get().min, options.decimalPlaces );
    var maxWidth = valueNode.width;
    valueNode.text = Util.toFixed( rangeProperty.get().max, options.decimalPlaces );
    maxWidth = Math.max( maxWidth, valueNode.width );

    // compute shape of the background behind the numeric value
    var backgroundWidth = maxWidth + ( 2 * options.xMargin );
    var backgroundHeight = valueNode.height + ( 2 * options.yMargin );
    var backgroundOverlap = 1;
    var backgroundCornerRadius = options.cornerRadius;

    // top half of the background, for 'up'. Shape computed starting at upper-left, going clockwise.
    var upBackground = new Path( new Shape()
      .arc( backgroundCornerRadius, backgroundCornerRadius, backgroundCornerRadius, Math.PI, Math.PI * 3 / 2, false )
      .arc( backgroundWidth - backgroundCornerRadius, backgroundCornerRadius, backgroundCornerRadius, -Math.PI / 2, 0, false )
      .lineTo( backgroundWidth, ( backgroundHeight / 2 ) + backgroundOverlap )
      .lineTo( 0, ( backgroundHeight / 2 ) + backgroundOverlap )
      .close(), { pickable: false } );

    // bottom half of the background, for 'down'. Shape computed starting at bottom-right, going clockwise.
    var downBackground = new Path( new Shape()
      .arc( backgroundWidth - backgroundCornerRadius, backgroundHeight - backgroundCornerRadius, backgroundCornerRadius, 0, Math.PI / 2, false )
      .arc( backgroundCornerRadius, backgroundHeight - backgroundCornerRadius, backgroundCornerRadius, Math.PI / 2, Math.PI, false )
      .lineTo( 0, backgroundHeight / 2 )
      .lineTo( backgroundWidth, backgroundHeight / 2 )
      .close(), { pickable: false } );

    // separate rectangle for stroke around value background
    var strokedBackground = new Rectangle( 0, 0, backgroundWidth, backgroundHeight, backgroundCornerRadius, backgroundCornerRadius, {
      pickable: false,
      stroke: options.backgroundStroke,
      lineWidth: options.backgroundLineWidth
    } );

    // compute size of arrows
    var arrowButtonSize = new Dimension2( 0.5 * backgroundWidth, options.arrowHeight );

    // 'up' arrow
    var arrowOptions = { stroke: 'black', lineWidth: 0.25, pickable: false };
    var upArrowShape = new Shape()
      .moveTo( arrowButtonSize.width / 2, 0 )
      .lineTo( arrowButtonSize.width, arrowButtonSize.height )
      .lineTo( 0, arrowButtonSize.height )
      .close();
    this.upArrow = new Path( upArrowShape, arrowOptions ); // @private
    this.upArrow.centerX = upBackground.centerX;
    this.upArrow.bottom = upBackground.top - options.arrowYSpacing;

    // 'down' arrow
    var downArrowShape = new Shape()
      .moveTo( arrowButtonSize.width / 2, arrowButtonSize.height )
      .lineTo( 0, 0 )
      .lineTo( arrowButtonSize.width, 0 )
      .close();
    this.downArrow = new Path( downArrowShape, arrowOptions ); // @private
    this.downArrow.centerX = downBackground.centerX;
    this.downArrow.top = downBackground.bottom + options.arrowYSpacing;

    // parents for 'up' and 'down' components
    var upParent = new Node( { children: [ upBackground, this.upArrow ] } );
    upParent.addChild( new Rectangle( upParent.localBounds ) ); // invisible overlay
    var downParent = new Node( { children: [ downBackground, this.downArrow ] } );
    downParent.addChild( new Rectangle( downParent.localBounds ) ); // invisible overlay

    // rendering order
    this.addChild( upParent );
    this.addChild( downParent );
    this.addChild( strokedBackground );
    this.addChild( valueNode );

    //------------------------------------------------------------
    // Pointer areas

    // touch area
    upParent.touchArea = Shape.rectangle(
      upParent.left - ( options.touchAreaExpandX / 2 ), upParent.top - options.touchAreaExpandY,
      upParent.width + options.touchAreaExpandX, upParent.height + options.touchAreaExpandY );
    downParent.touchArea = Shape.rectangle(
      downParent.left - ( options.touchAreaExpandX / 2 ), downParent.top,
      downParent.width + options.touchAreaExpandX, downParent.height + options.touchAreaExpandY );

    // mouse area
    upParent.mouseArea = Shape.rectangle(
      upParent.left - ( options.mouseAreaExpandX / 2 ), upParent.top - options.mouseAreaExpandY,
      upParent.width + options.mouseAreaExpandX, upParent.height + options.mouseAreaExpandY );
    downParent.mouseArea = Shape.rectangle(
      downParent.left - ( options.mouseAreaExpandX / 2 ), downParent.top,
      downParent.width + options.mouseAreaExpandX, downParent.height + options.mouseAreaExpandY );

    //------------------------------------------------------------
    // Colors

    // arrow colors
    var arrowColors = {
      up: options.color,
      over: options.color,
      down: options.pressedColor,
      out: options.color,
      disabled: 'rgb(176,176,176)'
    };

    // background colors
    var highlightGradient = createVerticalGradient( options.color, options.backgroundColor, options.color, backgroundHeight );
    var pressedGradient = createVerticalGradient( options.pressedColor, options.backgroundColor, options.pressedColor, backgroundHeight );
    var backgroundColors = {
      up: options.backgroundColor,
      over: highlightGradient,
      down: pressedGradient,
      out: pressedGradient,
      disabled: options.backgroundColor
    };

    //------------------------------------------------------------
    // Observers and InputListeners

    // up
    upParent.addInputListener( new ButtonStateListener( upStateProperty ) );
    var upListener = new FireOnHoldInputListener( {
      listener: function() {
        valueProperty.set( Math.min( options.upFunction(), rangeProperty.get().max ) );
      },
      timerDelay: options.timerDelay,
      timerInterval: options.timerInterval
    } );
    upParent.addInputListener( upListener );

    // down
    downParent.addInputListener( new ButtonStateListener( downStateProperty ) );
    var downListener = new FireOnHoldInputListener( {
      listener: function() {
        valueProperty.set( Math.max( options.downFunction(), rangeProperty.get().min ) );
      },
      timerDelay: options.timerDelay,
      timerInterval: options.timerInterval
    } );
    downParent.addInputListener( downListener );

    // enable/disable listeners: unlink unnecessary, properties are owned by this instance
    this.upEnabledListener = function( enabled ) { upListener.enabled = enabled; }; // @private
    this.upEnabledProperty.link( this.upEnabledListener );
    this.downEnabledListener = function( enabled ) { downListener.enabled = enabled; }; // @private
    this.downEnabledProperty.link( this.downEnabledListener );

    // @private Update text to match the value
    this.valueObserver = function( value ) {
      if ( value === null || value === undefined ) {
        valueNode.text = options.noValueString;
        valueNode.x = ( backgroundWidth - valueNode.width ) / 2; // horizontally centered
      }
      else {
        valueNode.text = Util.toFixed( value, options.decimalPlaces );
        if ( options.align === 'center' ) {
          valueNode.centerX = upBackground.centerX;
        }
        else if ( options.align === 'right' ) {
          valueNode.right = upBackground.right - options.xMargin;
        }
        else if ( options.align === 'left' ) {
          valueNode.left = upBackground.left + options.xMargin;
        }
        else {
          throw new Error( 'unsupported value for options.align: ' + options.align );
        }
      }
      valueNode.centerY = backgroundHeight / 2;
    };
    this.valueProperty.link( this.valueObserver ); // must be unlinked in dispose

    // @private update colors for 'up' components
    this.updateColorsForUpComponentsMultilink = Property.multilink( [ upStateProperty, this.upEnabledProperty ],
      function( state, enabled ) {
        updateColors( state, enabled, upBackground, thisNode.upArrow, backgroundColors, arrowColors );
      }
    );

    // @private update colors for 'down' components
    this.updateColorsForDownComponentsMultilink = Property.multilink( [ downStateProperty, this.downEnabledProperty ], function( state, enabled ) {
      updateColors( state, enabled, downBackground, thisNode.downArrow, backgroundColors, arrowColors );
    } );

    this.mutate( options );
  }

  /**
   * Converts ButtonListener events to state changes.
   *
   * @param {Property.<string>} stateProperty up|down|over|out
   * @param {Object} [options]
   * @constructor
   */
  function ButtonStateListener( stateProperty ) {
    ButtonListener.call( this, {
      up: function() { stateProperty.set( 'up' ); },
      over: function() { stateProperty.set( 'over' ); },
      down: function() { stateProperty.set( 'down' ); },
      out: function() { stateProperty.set( 'out' ); }
    } );
  }

  // creates a vertical gradient
  var createVerticalGradient = function( topColor, centerColor, bottomColor, height ) {
    return new LinearGradient( 0, 0, 0, height )
      .addColorStop( 0, topColor )
      .addColorStop( 0.5, centerColor )
      .addColorStop( 1, bottomColor );
  };

  // Update arrow and background colors
  var updateColors = function( state, enabled, background, arrow, backgroundColors, arrowColors ) {
    if ( enabled ) {
      arrow.stroke = 'black';
      if ( state === 'up' ) {
        background.fill = backgroundColors.up;
        arrow.fill = arrowColors.up;
      }
      else if ( state === 'over' ) {
        background.fill = backgroundColors.over;
        arrow.fill = arrowColors.over;
      }
      else if ( state === 'down' ) {
        background.fill = backgroundColors.down;
        arrow.fill = arrowColors.down;
      }
      else if ( state === 'out' ) {
        background.fill = backgroundColors.out;
        arrow.fill = arrowColors.out;
      }
      else {
        throw new Error( 'unsupported state: ' + state );
      }
    }
    else {
      background.fill = backgroundColors.disabled;
      arrow.fill = arrowColors.disabled;
      arrow.stroke = arrowColors.disabled; // stroke so that arrow size will look the same when it's enabled/disabled
    }
  };

  inherit( ButtonListener, ButtonStateListener );

  return inherit( Node, NumberPicker, {

    // @public Ensures that this node is eligible for GC.
    dispose: function() {
      this.upEnabledProperty.unlink( this.upEnabledListener );
      Property.unmultilink( this.updateColorsForUpComponentsMultilink );
      this.upEnabledProperty.dispose();

      this.downEnabledProperty.unlink( this.downEnabledListener );
      Property.unmultilink( this.updateColorsForDownComponentsMultilink );
      this.downEnabledProperty.dispose();

      this.valueProperty.unlink( this.valueObserver );
    },

    // @public
    setArrowsVisible: function( visible ) {
      this.upArrow.visible = this.downArrow.visible = visible;
    }
  } );
} );

[samreid]

@pixelzoom you are listed as author for NumberPicker and primary developer for BCE, can you take a look at this issue at your convenience?

[pixelzoom]

I'll have a look.

[pixelzoom]

In this situation, the type owns the Properties (upEnabledProperty, downEnabledProperty), and they exist for the lifetime of the type. And this is a common scenario. So how about solving this in a more general way by adding unlinkAll to Property, which unlinks all observers. Then the only change required in NumberPicker is:

    // @public Ensures that this node is eligible for GC.
    dispose: function() {

      this.upEnabledProperty.unlinkAll(); // Property is owned by this instance
      this.upEnabledProperty.dispose();

      this.downEnabledProperty.unlinkAll(); // Property is owned by this instance
      this.downEnabledProperty.dispose();

      this.valueProperty.unlink( this.valueObserver );
    },

[pixelzoom]

Btw... I also think what I've described in https://github.com/phetsims/balancing-chemical-equations/issues/109#issuecomment-156910653 is the only practical way to solve this in the case where Properties "owned" by an instance are @public. In that case, it may be impossible for all observers to remove themselves before the instance is disposed of. And in the case of @private Properties owned by the instance (like this example in BCE), it's particularly convenient to be able to unlinkAll, since the only linking was done by the instance itself. Having to handle cleanup as described in https://github.com/phetsims/balancing-chemical-equations/issues/109#issuecomment-156639321 is unnecessarily burdensome and complicated.

[samreid]

@pixelzoom do you see any problems with having Property.dispose() call Property.unlinkAll()? In that case, the NumberPicker example would read like so:

    // @public Ensures that this node is eligible for GC.
    dispose: function() {

      this.upEnabledProperty.dispose();
      this.downEnabledProperty.dispose();

      this.valueProperty.unlink( this.valueObserver );
    },

[pixelzoom]

If the client has to explicitly call unlinkAll, then we can be reasonably certain that the client is OK with the fact that there may be observers. If we call unlinkAll in Property.disposeProperty, then we may be masking a programming error. As the comment at line 52 of Property says:

// Make sure there were no remaining observers.  If there are observers at disposal time, there may be a latent
// memory leak, see #77

[samreid]

Thanks for reiterating this, I'm ok for you to proceed as described in https://github.com/phetsims/balancing-chemical-equations/issues/109#issuecomment-156910653 Please let me know which parts of this you would like me to work on, if any.

[pixelzoom]

I'll take it from here. Thanks for illuminating the problem and discussing the solution.

[pixelzoom]

Resolved, tested, closing.