phetsims / john-travoltage

"John Travoltage" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/john-travoltage
GNU General Public License v3.0
4 stars 8 forks source link

CT tried to removeListener on something that wasn't a listener #445

Closed KatieWoe closed 3 years ago

KatieWoe commented 3 years ago
john-travoltage : multitouch-fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1628759848901/john-travoltage/john-travoltage_en.html?continuousTest=%7B%22test%22%3A%5B%22john-travoltage%22%2C%22multitouch-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1628759848901%22%2C%22timestamp%22%3A1628763588058%7D&brand=phet&ea&fuzz&fuzzPointers=2&memoryLimit=1000&supportsPanAndZoom=false
Query: brand=phet&ea&fuzz&fuzzPointers=2&memoryLimit=1000&supportsPanAndZoom=false
Uncaught Error: Assertion failed: tried to removeListener on something that wasn't a listener
Error: Assertion failed: tried to removeListener on something that wasn't a listener
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1628759848901/assert/js/assert.js:25:13)
at Timer.removeListener (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1628759848901/axon/js/TinyEmitter.js:135:7)
at callback (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1628759848901/axon/js/Timer.js:33:16)
at Timer.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1628759848901/axon/js/TinyEmitter.js:86:9)
at https://bayes.colorado.edu/continuous-testing/ct-snapshots/1628759848901/joist/js/Sim.js:303:17
at Action.execute (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1628759848901/axon/js/Action.js:227:18)
at Sim.stepSimulation (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1628759848901/joist/js/Sim.js:1083:31)
at Sim.stepOneFrame (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1628759848901/joist/js/Sim.js:1073:12)
at Sim.runAnimationLoop (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1628759848901/joist/js/Sim.js:1051:12)
id: Bayes Chrome
Snapshot from 8/12/2021, 3:17:28 AM
jessegreenberg commented 3 years ago

This seems pretty rare, and I am not sure what is causing it yet. Fuzzing on my local machine with the same query parameters isn't showing it, and it isn't in the last 10 CT collumns.

But I have seen this once before as well on CT before the snapshot @KatieWoe posted so I don't think it is gone.

jessegreenberg commented 3 years ago

The case that we are running into is this in Timer.setTimeout

        // make sure that this callback hasn't already been removed by another listener while emit() is in progress
        if ( this.hasListener( callback ) ) {
          listener();
          this.removeListener( callback );
        }

The listener must be removing itself so this.removeListener errors out.

jessegreenberg commented 3 years ago

I am seeing this in GFL:B as well in the last 10 CT cycles, must not be specific to this sim. Going to try to get it to happen there.

jessegreenberg commented 3 years ago

Found more information about why this is happening: image

When a speech request is made, something is causing the queue to synchronously clear (and therefore remove timer listeners) from within the setTimeout listener.

jessegreenberg commented 3 years ago

I found it. What is happening is:

endSpeakingEmitter being called on the error event seems correct to me, and I don't think there is anything wrong about the client usage. I think we need to be more graceful in voicingManager by not removing the listener in cancel() if cancel was initiated immediately from a request to speak.

jessegreenberg commented 3 years ago

This was fixed in the above commit. I was able to step through the problematic case and verify that the listener was not removed twice, but was successfully removed once at the end of the callback. Closing.