Node's dispose isn't called often

[jonathanolson]

A ton of types are overriding dispose() on Node and not calling the Node.prototype.dispose(). It has always existed, but was just part of Events which had a no-op dispose() method, but now (with the Tandem handling) it actually does something.

Flagging for developer meeting for handling. I'll check to see whether I can identify where this happens by toString()ing functions automatically.

[samreid]

I added in the Node.prototype.dispose.call(this) into many of the instrumented sun and scenery-phet classes. It would be nice to check that other node subclasses address this.

[pixelzoom]

I've started using this pattern to future-proof implementations of dispose that involve a supertype:

dispose: function() {
  SuperType.prototype.dispose && SuperType.prototype.dispose.call( this );
  this.disposeSubType();
}

The advantage of the above approach is that I don't have to go hunting for dispose functions in the type hierarchy -- which are sometime not at all easy to find, due to mixins or non-intutivie hierarchies (Node extends Events being a prime example).

The disadvantage of the above approach is that I don't identify supertypes that should have a dispose function and are therefore leaking memory.

Another disadvantage is that, a future implementation of the supertype's dispose may break my code. So when you add a dispose function to a type, don't assume that you don't need to test! There may be code that starts to use your new dispose function as soon as it's added.

[samreid]

Generally, the order should be reversed:

dispose: function() {
  this.disposeSubType();
  SuperType.prototype.dispose && SuperType.prototype.dispose.call( this );
}

That is, the order an object is disassembled should be the reverse of the order the object is assembled. For instance, if subType does this.children.push(x) and subType dispose does this.children.remove(x), that must be done before the SuperType eliminates the children array.

[pixelzoom]

Self assigned to look at dispose implementations in my sims to see if any of them need to be fixed, specifically related to Node.

[pixelzoom]

@jonathanolson said:

I'll check to see whether I can identify where this happens by toString()ing functions automatically.

In 2/9/17 dev meeting, he said there's a better way to do this, and he'll do it.

[pixelzoom]

I took care of my sims, and all common-code except:

• scenery-phet/js/*.js
• scenery-phet/js/buttons/*.js
• sun/js/*.js

I intended to handle these sometime in the next week.

[pixelzoom]

I completed common-code, see above commits.
Tested using test-server with ?ea&testBuilt=false&fuzzMouse.

[pixelzoom]

All devs: Highly recommended to address this issue in your sims. You can wait until @jonathanolson implements automated checking. Or you can manually inspect dispose functions to verify that they chain to their supertype’s dispose function (search for dispose:). In either case, here's my recommendation on how to address this issue in client code.

First, identify whether the supertype has a dispose function. The easiest way to do this is to run a sim, then inspect the supertype's prototype in the console.

For example, Text does have a dispose function, while Color does not:

> phet.scenery.Text.prototype.dispose
function () { ... }
> phet.scenery.Color.prototype.dispose
undefined

If the supertype does have a dispose function, then override dispose like this:

dispose: function() {
  this.disposeSubType();
  SuperType.prototype.dispose.call( this );
}

If the supertype does not have a dispose function, then I recommend future-proofing your code by overriding dispose like this:

dispose: function() {
  this.disposeSubType();
  SuperType.prototype.dispose && SuperType.prototype.dispose.call( this );
}

[jonathanolson]

Issues created for locations that still have this problem, and since it seems to be helpful long-term, I'll leave in the assertion checks to make sure Node.dispose() is called (and that it isn't called twice).