search

phetsims / build-an-atom

"Build an Atom" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/build-an-atom
GNU General Public License v3.0
11 stars 10 forks source link

Assertion failed: tried to removeListener on something that wasn't a listener #176

Closed phet-steele closed 6 years ago

phet-steele commented 6 years ago

In levels 2 - 4, try to complete challenges that look like this (a SchematicToSymbolChallenge):

screen shot 2017-10-16 at 4 13 35 pm 
Error: Assertion failed: tried to removeListener on something that wasn't a listener
    at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/snapshot-1508189960283/assert/js/assert.js:22:13)
    at Emitter.removeListener (https://bayes.colorado.edu/continuous-testing/snapshot-1508189960283/axon/js/Emitter.js?bust=1508191424338:103:17)
    at DerivedProperty.unlink (https://bayes.colorado.edu/continuous-testing/snapshot-1508189960283/axon/js/Property.js?bust=1508191424338:251:29)
    at DerivedProperty.dispose (https://bayes.colorado.edu/continuous-testing/snapshot-1508189960283/axon/js/DerivedProperty.js?bust=1508191424338:96:20)
    at ParticleAtom.dispose (https://bayes.colorado.edu/continuous-testing/snapshot-1508189960283/shred/js/model/ParticleAtom.js?bust=1508191424338:265:27)
    at NonInteractiveSchematicAtomNode.disposeNonInteractiveSchematicAtomNode (https://bayes.colorado.edu/continuous-testing/snapshot-1508189960283/build-an-atom/js/game/view/NonInteractiveSchematicAtomNode.js?bust=1508191424338:92:20)
    at NonInteractiveSchematicAtomNode.dispose (https://bayes.colorado.edu/continuous-testing/snapshot-1508189960283/build-an-atom/js/game/view/NonInteractiveSchematicAtomNode.js?bust=1508191424338:103:12)
    at NonInteractiveSchematicAtomNode.Node.dispose (https://bayes.colorado.edu/continuous-testing/snapshot-1508189960283/scenery/js/nodes/Node.js?bust=1508191424338:518:22)
    at SchematicToSymbolChallengeView.disposeSchematicToSymbolChallengeView (https://bayes.colorado.edu/continuous-testing/snapshot-1508189960283/build-an-atom/js/game/view/SchematicToSymbolChallengeView.js?bust=1508191424338:64:25)
    at SchematicToSymbolChallengeView.dispose (https://bayes.colorado.edu/continuous-testing/snapshot-1508189960283/build-an-atom/js/game/view/SchematicToSymbolChallengeView.js?bust=1508191424338:92:12)
samreid commented 6 years ago

Somehow rearranging the derived property disposals seems to help.......

samreid commented 6 years ago

I'm puzzled, but reordering the derived property disposal changes whether there is an assertion or not. I also cleaned up DerivedProperty.dispose in https://github.com/phetsims/axon/issues/148

@zepumph and/or @jbphet can you please take a look around?

jbphet commented 6 years ago

I'll take a look.

jbphet commented 6 years ago

I tried for a bit to investigate this, and I did it by manually reverting the change to the order of disposal of properties in PropertyAtom.dispose(). This didn't cause any problems in operation of the non-phet-io version as far as I could tell with quick testing, nor with the phet-io state wrapper or instance-proxies wrapper. However, after thinking about it, I suspect none of these invoke the PropertyAtom.dispose() method, so I tried the state wrapper, but ran into other problems there (the message I hit was mismatched messageIndex, possible start/end mismatch). @samreid and I have a collaboration session scheduled for the near future, perhaps we should take a look together then.

jbphet commented 6 years ago

@samreid, @zepumph, and I dug into this, and it now makes sense to us why it is necessary to dispose derived properties before their dependencies. The reason is that if a dependency is disposed before a derived property, the listener function that is linked to the dependency by the derived property (to allow the derived property to update when the dependency changes) will be removed, then when the derived property is disposed, it will attempt to remove that same listener function, which will cause an assert due to recent changes to axon that flags attempts to remove listeners that aren't present (see https://github.com/phetsims/axon/issues/129 and https://github.com/phetsims/axon/issues/132).

We were wondering why this problem didn't show up as soon as the mods to axon were made, and it is probably because there are a lot of derived properties that weren't disposed because they were unlikely to cause memory leaks. However, in the process of instrumenting the game for Build an Atom and getting state save/restore to work, there is a lot of additional disposal that is needing to be done. This is causing this case to be hit a lot.

Marking for developer meeting to discuss so that developers are aware of this constraint in the way the disposal occurs.

zepumph commented 6 years ago

Here is the example that exposed this problem from ParticleAtom.js:


    dispose: function() {

     /* DerivedProperties that use the lower countProperties */
      this.particleCountProperty.dispose();
      this.massNumberProperty.dispose();
      this.chargeProperty.dispose();

      // These should be disposed after because they are dependencies to the above DerivedProperties
      this.protonCountProperty.dispose();
      this.neutronCountProperty.dispose();
      this.electronCountProperty.dispose();

     . . . .
    }
jbphet commented 6 years ago

This has been fixed - closing.