Accessibility lacks memory management

[pixelzoom]

Accessibility has a couple of memory management deficiencies:

(1) It also no dispose function, where (ala Property and other PhET common components) listeners should be cleared.

(2) It has addAccessibleInputListener and removeAccessibleInputListener, but no hasAccessibleInputListener. So depending on ordering of operations, a listener can be tricky to remove. Dispose code should generally look like this (e.g. Dialog disposeDialog):

if ( self.hasAccessibleInputListener( escapeListener ) ) {
  self.removeAccessibleInputListener( escapeListener );
}

[pixelzoom]

(3) Example all client code that uses addAccessibleInputListener and determine whether a dispose call is needed.

(4) Examine all client code that uses removeAccessibleInputListener and determine whether hasAccessibleInputListener check is needed.

[jessegreenberg]

Dispose code should generally look like this (e.g. Dialog disposeDialog):

I don't see checks like this in dispose code very often. Is Dialog an example of where this should be added? Why? Should hasListener checks be added around the other listener removals in Dialog's disposeDialog?

[jessegreenberg]

(3) in https://github.com/phetsims/scenery/issues/775#issuecomment-385453779 was recently done for https://github.com/phetsims/scenery/issues/775.

Back to @pixelzoom to make sure I understand, then Ill continue with (4) of https://github.com/phetsims/scenery/issues/775#issuecomment-385453779

[pixelzoom]

@jessegreenberg said:

I don't see checks like this in dispose code very often.

Most dispose functions contain an implicit ordering dependency. If a client passes your component a Property and you link to it, you can't simply unlink it in dispose. You have to verify that you're still linked to it (using hasListener) before calling unlink. The client may have called dispose on the Property before calling dispose on your component. And Property's dispose removes all listeners. If you attempt to unlink from a Property that you're no longer linked to, you'll cause an assertion failure on line 80 of Emitter.

Example:

// MyType.js
function MyType( locationProperty ) {
  var locationListener = function( location ) { ... };
  locationProperty.link( locationListener );
  ...
  this.disposeMyType = function() {
    locationProperty.unlink( locationListener );
  }
}

return inherit( ..., MyType, {
  dispose: function() {
    this.disposeMyType();
  }
} );

// client code that's OK
var locationProperty = new Property(...);
var myType = new MyType( locationProperty );
...
myType.dispose();
locationProperty.dispose();

// client code that fails
var locationProperty = new Property(...);
var myType = new MyType( locationProperty );
...
locationProperty.dispose();
myType.dispose(); // assertion failure: "tried to removeListener on something that wasn't a listener"

[pixelzoom]

@jessegreenberg asked:

Is Dialog an example of where this should be added? Why?

Yes. See example above.

Should hasListener checks be added around the other listener removals in Dialog's disposeDialog?

Yes. Planning to do this for Dialog when addressing https://github.com/phetsims/joist/issues/366 and https://github.com/phetsims/joist/issues/357.

And I'll bet that most of the dispose implementations in PhET code have one or more cases where they will fail depending on the order of disposal.

[jessegreenberg]

Got it, thanks for clarifying.