CT: TinyEmitter.emit should not be called if disposed

KatieWoe commented 1 year ago

projectile-motion : fuzz : unbuilt : listenerOrderRandom
http://128.138.93.172//continuous-testing/ct-snapshots/1688154842092/projectile-motion/projectile-motion_en.html?continuousTest=%7B%22test%22%3A%5B%22projectile-motion%22%2C%22fuzz%22%2C%22unbuilt%22%2C%22listenerOrderRandom%22%5D%2C%22snapshotName%22%3A%22snapshot-1688154842092%22%2C%22timestamp%22%3A1688155765303%7D&brand=phet&ea&fuzz&listenerOrder=random
Query: brand=phet&ea&fuzz&listenerOrder=random
Uncaught Error: Assertion failed: should not be called if disposed
Error: Assertion failed: should not be called if disposed
at window.assertions.assertFunction (http://128.138.93.172//continuous-testing/ct-snapshots/1688154842092/assert/js/assert.js:28:13)
at assert (TinyEmitter.ts:98:14)
at emit (TinyProperty.ts:125:9)
at notifyListeners (TinyProperty.ts:73:11)
at set (TinyForwardingProperty.ts:175:12)
at set (Node.ts:3884:25)
at setVisible (Node.ts:3892:9)
at (FreeBodyDiagram.ts:153:29)
at (TinyEmitter.ts:136:42)
at emit (ReadOnlyProperty.ts:315:22)
[URL] http://128.138.93.172//continuous-testing/aqua/html/sim-test.html?url=..%2F..%2Fct-snapshots%2F1688154842092%2Fprojectile-motion%2Fprojectile-motion_en.html&simQueryParameters=brand%3Dphet%26ea%26fuzz%26listenerOrder%3Drandom&testInfo=%7B%22test%22%3A%5B%22projectile-motion%22%2C%22fuzz%22%2C%22unbuilt%22%2C%22listenerOrderRandom%22%5D%2C%22snapshotName%22%3A%22snapshot-1688154842092%22%2C%22timestamp%22%3A1688155765303%7D
[NAVIGATED] http://128.138.93.172//continuous-testing/aqua/html/sim-test.html?url=..%2F..%2Fct-snapshots%2F1688154842092%2Fprojectile-motion%2Fprojectile-motion_en.html&simQueryParameters=brand%3Dphet%26ea%26fuzz%26listenerOrder%3Drandom&testInfo=%7B%22test%22%3A%5B%22projectile-motion%22%2C%22fuzz%22%2C%22unbuilt%22%2C%22listenerOrderRandom%22%5D%2C%22snapshotName%22%3A%22snapshot-1688154842092%22%2C%22timestamp%22%3A1688155765303%7D
[NAVIGATED] about:blank
[NAVIGATED] http://128.138.93.172//continuous-testing/ct-snapshots/1688154842092/projectile-motion/projectile-motion_en.html?continuousTest=%7B%22test%22%3A%5B%22projectile-motion%22%2C%22fuzz%22%2C%22unbuilt%22%2C%22listenerOrderRandom%22%5D%2C%22snapshotName%22%3A%22snapshot-1688154842092%22%2C%22timestamp%22%3A1688155765303%7D&brand=phet&ea&fuzz&listenerOrder=random
[CONSOLE] enabling assert
[CONSOLE] listenerOrder random seed: 906065
[CONSOLE] continuous-test-load
[CONSOLE] Assertion failed: should not be called if disposed
[PAGE ERROR] Error: Error: Assertion failed: should not be called if disposed
at window.assertions.assertFunction (http://128.138.93.172//continuous-testing/ct-snapshots/1688154842092/assert/js/assert.js:28:13)
at TinyForwardingProperty.emit (http://128.138.93.172//continuous-testing/ct-snapshots/1688154842092/chipper/dist/js/axon/js/TinyEmitter.js:72:15)
at TinyForwardingProperty.notifyListeners (http://128.138.93.172//continuous-testing/ct-snapshots/1688154842092/chipper/dist/js/axon/js/TinyProperty.js:109:10)
at TinyForwardingProperty.set (http://128.138.93.172//continuous-testing/ct-snapshots/1688154842092/chipper/dist/js/axon/js/TinyProperty.js:58:12)
at TinyForwardingProperty.set (http://128.138.93.172//continuous-testing/ct-snapshots/1688154842092/chipper/dist/js/axon/js/TinyForwardingProperty.js:140:13)
at Node.setVisible (http://128.138.93.172//continuous-testing/ct-snapshots/1688154842092/chipper/dist/js/scenery/js/nodes/Node.js:3542:26)
at set visible [as visible] (http://128.138.93.172//continuous-testing/ct-snapshots/1688154842092/chipper/dist/js/scenery/js/nodes/Node.js:3550:10)
at Array.viewPointListener (http://128.138.93.172//continuous-testing/ct-snapshots/1688154842092/chipper/dist/js/projectile-motion/js/common/view/FreeBodyDiagram.js:113:31)
at TinyProperty.emit (http://128.138.93.172//continuous-testing/ct-snapshots/1688154842092/chipper/dist/js/axon/js/TinyEmitter.js:108:39)
at DerivedProperty._notifyListeners (http://128.138.93.172//continuous-testing/ct-snapshots/1688154842092/chipper/dist/js/axon/js/ReadOnlyProperty.js:239:23)
[CONSOLE] continuous-test-error

id: "Sparky Node Puppeteer"
Snapshot from 6/30/2023, 1:54:02 PM

pixelzoom commented 10 months ago

This error is still occurring occassionally (about once per day) as of 10/10/23.

pixelzoom commented 10 months ago

The problem originates at this line in FreeBodyDiagram:

153      xDragForceLabel.visible = dragForceExists;

xDragForceLabel has apparently been disposed, which in turn has disposed its default visibleProperty, a TinyProperty. So trying to set xDragForceLabel.visible fails this assertion in TinyEmitter (the superclass of TinyProperty):

98  public emit( ...args: T ): void {
99    assert && assert( !this.isDisposed, 'should not be called if disposed' );

xDragForceLabel is a local const in FreeBodyDiagram's constructor, so it can only have been disposed by calling dispose on the FreeBodyDiagram.

jessegreenberg commented 10 months ago

Here is how to reproduce this manually (only found after looking at the code)

Load the Vectors screen with ?listenerOrder=random(906065)
Make all Vectors visible.
Fire a cannonball. Pause the sim when it gets close to the ground. In the final step before the ball hits the ground toggle the Air Resistance. Then step forward one more time. That is the only way for the visibility to change in the viewPointListener and the assertion mentioned above only happens when there is a change in value.

(Image showing the last step before ball hits the ground. Toggle Air Resistance here).

jessegreenberg commented 10 months ago

This happens when the listeners on ProjectileNode.viewPointProperty are shuffled. One listener in ProjectileNode calls dispose and another updates visibility (the viewPointListener). If the dispose happens first this assertion will happen. It happens even though the dispose unlinks the viewPointListener because of this: https://github.com/phetsims/axon/blob/4f571cbc738bdcba5f9f49050c3f2a1eddde9110/js/TinyEmitter.ts#L195-L199.

Knowing that, it seems OK to me to only update visibility if not disposed. But Ill leave to @matthew-blackman to review in case you want to handle a different way!

matthew-blackman commented 3 months ago

The commit above looks good. Tested force diagram behavior and no issues. Nice work @jessegreenberg! CT is also looking good. Closing.

phetsims / projectile-motion

CT: TinyEmitter.emit should not be called if disposed #323