Closed KatieWoe closed 3 months ago
Seeing this again
buoyancy : fuzz : unbuilt
http://127.0.0.1/continuous-testing/ct-snapshots/1683626850194/buoyancy/buoyancy_en.html?continuousTest=%7B%22test%22%3A%5B%22buoyancy%22%2C%22fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1683626850194%22%2C%22timestamp%22%3A1683627329248%7D&brand=phet&ea&fuzz
Query: brand=phet&ea&fuzz
Error: Assertion failed: tried to removeListener on something that wasn't a listener
window.assertions.assertFunction<@http://127.0.0.1/continuous-testing/ct-snapshots/1683626850194/assert/js/assert.js:28:13
at window.assertions.assertFunction< (http://127.0.0.1/continuous-testing/ct-snapshots/1683626850194/assert/js/assert.js:28:13)
at (TinyEmitter.ts:170:14)
at removeListener (TinyProperty.ts:150:9)
at unlink (ReadOnlyProperty.ts:429:22)
at unlink (DynamicProperty.ts:246:38)
at (TinyEmitter.ts:119:18)
at emit (ReadOnlyProperty.ts:315:22)
at _notifyListeners (ReadOnlyProperty.ts:266:13)
at unguardedSet (ReadOnlyProperty.ts:250:11)
at set (Property.ts:54:10)
at inverseMap (DynamicProperty.ts:273:68)
at (TinyEmitter.ts:119:18)
at emit (ReadOnlyProperty.ts:315:22)
at _notifyListeners (ReadOnlyProperty.ts:266:13)
at unguardedSet (ReadOnlyProperty.ts:250:11)
at set (DynamicProperty.ts:311:10)
at set (DynamicProperty.ts:329:9)
at property (ComboBoxListBox.ts:104:6)
at apply (PhetioAction.ts:158:16)
at execute (ComboBoxListBox.ts:126:19)
at inputEvent (Input.ts:1899:69)
at dispatchToListeners (Input.ts:1939:11)
at dispatchToTargets (Input.ts:1851:9)
at dispatchEvent (Input.ts:1653:9)
at upEvent (Input.ts:518:13)
at apply (PhetioAction.ts:158:16)
at execute (Input.ts:1291:24)
at touchEnd (InputFuzzer.js:242:24)
at touchEnd (InputFuzzer.js:55:11)
at action (InputFuzzer.js:107:6)
at fuzzEvents (SimDisplay.ts:211:23)
at fuzzInputEvents (Sim.ts:998:44)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
id: Sparky Playwright Firefox
Snapshot from 5/9/2023, 4:07:30 AM
----------------------------------
buoyancy : multitouch-fuzz : unbuilt
http://127.0.0.1/continuous-testing/ct-snapshots/1683626850194/buoyancy/buoyancy_en.html?continuousTest=%7B%22test%22%3A%5B%22buoyancy%22%2C%22multitouch-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1683626850194%22%2C%22timestamp%22%3A1683628425725%7D&brand=phet&ea&fuzz&fuzzPointers=2&supportsPanAndZoom=false
Query: brand=phet&ea&fuzz&fuzzPointers=2&supportsPanAndZoom=false
Error: Assertion failed: tried to removeListener on something that wasn't a listener
window.assertions.assertFunction<@http://127.0.0.1/continuous-testing/ct-snapshots/1683626850194/assert/js/assert.js:28:13
at window.assertions.assertFunction< (http://127.0.0.1/continuous-testing/ct-snapshots/1683626850194/assert/js/assert.js:28:13)
at (TinyEmitter.ts:170:14)
at removeListener (TinyProperty.ts:150:9)
at unlink (ReadOnlyProperty.ts:429:22)
at unlink (DynamicProperty.ts:246:38)
at (TinyEmitter.ts:119:18)
at emit (ReadOnlyProperty.ts:315:22)
at _notifyListeners (ReadOnlyProperty.ts:266:13)
at unguardedSet (ReadOnlyProperty.ts:250:11)
at set (Property.ts:54:10)
at createCustomSolidMaterial (MaterialMassVolumeControlNode.ts:208:44)
at (TinyEmitter.ts:119:18)
at emit (ReadOnlyProperty.ts:315:22)
at _notifyListeners (ReadOnlyProperty.ts:266:13)
at unguardedSet (ReadOnlyProperty.ts:250:11)
at set (Property.ts:65:10)
at set (Slider.ts:463:24)
at (TinyEmitter.ts:119:18)
at emit (ReadOnlyProperty.ts:315:22)
at _notifyListeners (ReadOnlyProperty.ts:266:13)
at unguardedSet (ReadOnlyProperty.ts:250:11)
at set (DerivedProperty.ts:158:12)
at (TinyEmitter.ts:119:18)
at emit (ReadOnlyProperty.ts:315:22)
at _notifyListeners (ReadOnlyProperty.ts:266:13)
at unguardedSet (ReadOnlyProperty.ts:250:11)
at set (Property.ts:54:10)
at inverseMap (DynamicProperty.ts:273:68)
at (TinyEmitter.ts:119:18)
at emit (ReadOnlyProperty.ts:315:22)
at _notifyListeners (ReadOnlyProperty.ts:266:13)
at unguardedSet (ReadOnlyProperty.ts:250:11)
at set (DynamicProperty.ts:311:10)
at set (DynamicProperty.ts:329:9)
at property (ComboBoxListBox.ts:104:6)
at apply (PhetioAction.ts:158:16)
at execute (ComboBoxListBox.ts:126:19)
at inputEvent (Input.ts:1899:69)
at dispatchToListeners (Input.ts:1939:11)
at dispatchToTargets (Input.ts:1851:9)
at dispatchEvent (Input.ts:1653:9)
at upEvent (Input.ts:401:11)
at apply (PhetioAction.ts:158:16)
at execute (Input.ts:1219:23)
at mouseUp (InputFuzzer.js:275:26)
at mouseToggle (InputFuzzer.js:40:11)
at action (InputFuzzer.js:107:6)
at fuzzEvents (SimDisplay.ts:211:23)
at fuzzInputEvents (Sim.ts:998:44)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
at requestAnimationFrame (Sim.ts:990:11)
id: Sparky Playwright Firefox
Snapshot from 5/9/2023, 4:07:30 AM
May be related:
buoyancy : multitouch-fuzz : unbuilt
http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/buoyancy/buoyancy_en.html?continuousTest=%7B%22test%22%3A%5B%22buoyancy%22%2C%22multitouch-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1684762546721%22%2C%22timestamp%22%3A1684763878935%7D&brand=phet&ea&fuzz&fuzzPointers=2&supportsPanAndZoom=false
Query: brand=phet&ea&fuzz&fuzzPointers=2&supportsPanAndZoom=false
Uncaught Error: Assertion failed
Error: Assertion failed
at window.assertions.assertFunction (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/assert/js/assert.js:28:13)
at assert (Material.ts:101:14)
at (Material.ts:118:11)
at createCustomMaterial (Material.ts:138:20)
at createCustomSolidMaterial (MaterialMassVolumeControlNode.ts:208:44)
at listener (TinyEmitter.ts:119:8)
at emit (ReadOnlyProperty.ts:315:22)
at _notifyListeners (ReadOnlyProperty.ts:266:13)
at unguardedSet (ReadOnlyProperty.ts:250:11)
at set (Property.ts:65:10)
[URL] http://127.0.0.1/continuous-testing/aqua/html/sim-test.html?url=..%2F..%2Fct-snapshots%2F1684762546721%2Fbuoyancy%2Fbuoyancy_en.html&simQueryParameters=brand%3Dphet%26ea%26fuzz%26fuzzPointers%3D2%26supportsPanAndZoom%3Dfalse&testInfo=%7B%22test%22%3A%5B%22buoyancy%22%2C%22multitouch-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1684762546721%22%2C%22timestamp%22%3A1684763878935%7D
[NAVIGATED] http://127.0.0.1/continuous-testing/aqua/html/sim-test.html?url=..%2F..%2Fct-snapshots%2F1684762546721%2Fbuoyancy%2Fbuoyancy_en.html&simQueryParameters=brand%3Dphet%26ea%26fuzz%26fuzzPointers%3D2%26supportsPanAndZoom%3Dfalse&testInfo=%7B%22test%22%3A%5B%22buoyancy%22%2C%22multitouch-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1684762546721%22%2C%22timestamp%22%3A1684763878935%7D
[NAVIGATED] about:blank
[NAVIGATED] http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/buoyancy/buoyancy_en.html?continuousTest=%7B%22test%22%3A%5B%22buoyancy%22%2C%22multitouch-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1684762546721%22%2C%22timestamp%22%3A1684763878935%7D&brand=phet&ea&fuzz&fuzzPointers=2&supportsPanAndZoom=false
[CONSOLE] enabling assert
[CONSOLE] continuous-test-load
[CONSOLE] Assertion failed
[PAGE ERROR] Error: Error: Assertion failed
at window.assertions.assertFunction (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/assert/js/assert.js:28:13)
at new Material (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/density-buoyancy-common/js/common/model/Material.js:40:15)
at Material.createCustomMaterial (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/density-buoyancy-common/js/common/model/Material.js:56:12)
at Material.createCustomSolidMaterial (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/density-buoyancy-common/js/common/model/Material.js:76:21)
at http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/density-buoyancy-common/js/common/view/MaterialMassVolumeControlNode.js:166:45
at TinyProperty.emit (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/TinyEmitter.js:95:9)
at NumberProperty._notifyListeners (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/ReadOnlyProperty.js:237:23)
at NumberProperty.unguardedSet (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/ReadOnlyProperty.js:189:14)
at NumberProperty.set (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/ReadOnlyProperty.js:174:12)
at NumberProperty.set (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/Property.js:58:11)
[CONSOLE] continuous-test-error
id: "Sparky Node Puppeteer"
Snapshot from 5/22/2023, 7:35:46 AM
----------------------------------
buoyancy : pan-and-zoom-fuzz : unbuilt
http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/buoyancy/buoyancy_en.html?continuousTest=%7B%22test%22%3A%5B%22buoyancy%22%2C%22pan-and-zoom-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1684762546721%22%2C%22timestamp%22%3A1684767650400%7D&brand=phet&ea&fuzz&fuzzPointers=2&supportsPanAndZoom=true
Query: brand=phet&ea&fuzz&fuzzPointers=2&supportsPanAndZoom=true
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 (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/assert/js/assert.js:28:13)
at assert (TinyEmitter.ts:170:6)
at removeListener (TinyProperty.ts:150:9)
at unlink (ReadOnlyProperty.ts:429:22)
at unlink (DynamicProperty.ts:246:38)
at listener (TinyEmitter.ts:119:8)
at emit (ReadOnlyProperty.ts:315:22)
at _notifyListeners (ReadOnlyProperty.ts:266:13)
at unguardedSet (ReadOnlyProperty.ts:250:11)
at set (Property.ts:54:10)
[URL] http://127.0.0.1/continuous-testing/aqua/html/sim-test.html?url=..%2F..%2Fct-snapshots%2F1684762546721%2Fbuoyancy%2Fbuoyancy_en.html&simQueryParameters=brand%3Dphet%26ea%26fuzz%26fuzzPointers%3D2%26supportsPanAndZoom%3Dtrue&testInfo=%7B%22test%22%3A%5B%22buoyancy%22%2C%22pan-and-zoom-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1684762546721%22%2C%22timestamp%22%3A1684767650400%7D
[NAVIGATED] http://127.0.0.1/continuous-testing/aqua/html/sim-test.html?url=..%2F..%2Fct-snapshots%2F1684762546721%2Fbuoyancy%2Fbuoyancy_en.html&simQueryParameters=brand%3Dphet%26ea%26fuzz%26fuzzPointers%3D2%26supportsPanAndZoom%3Dtrue&testInfo=%7B%22test%22%3A%5B%22buoyancy%22%2C%22pan-and-zoom-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1684762546721%22%2C%22timestamp%22%3A1684767650400%7D
[NAVIGATED] about:blank
[NAVIGATED] http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/buoyancy/buoyancy_en.html?continuousTest=%7B%22test%22%3A%5B%22buoyancy%22%2C%22pan-and-zoom-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1684762546721%22%2C%22timestamp%22%3A1684767650400%7D&brand=phet&ea&fuzz&fuzzPointers=2&supportsPanAndZoom=true
[CONSOLE] enabling assert
[CONSOLE] continuous-test-load
[CONSOLE] Assertion failed: tried to removeListener on something that wasn't a listener
[PAGE ERROR] Error: Error: Assertion failed: tried to removeListener on something that wasn't a listener
at window.assertions.assertFunction (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/assert/js/assert.js:28:13)
at TinyProperty.removeListener (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/TinyEmitter.js:143:7)
at TinyProperty.unlink (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/TinyProperty.js:133:10)
at LocalizedStringProperty.unlink (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/ReadOnlyProperty.js:344:23)
at DynamicProperty.onPropertyChange (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/DynamicProperty.js:194:37)
at TinyProperty.emit (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/TinyEmitter.js:95:9)
at Property._notifyListeners (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/ReadOnlyProperty.js:237:23)
at Property.unguardedSet (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/ReadOnlyProperty.js:189:14)
at Property.set (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/ReadOnlyProperty.js:174:12)
at set value [as value] (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/Property.js:48:11)
[CONSOLE] continuous-test-error
[CONSOLE] Assertion failed: tried to removeListener on something that wasn't a listener
[PAGE ERROR] Error: Error: Assertion failed: tried to removeListener on something that wasn't a listener
at window.assertions.assertFunction (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/assert/js/assert.js:28:13)
at TinyProperty.removeListener (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/TinyEmitter.js:143:7)
at TinyProperty.unlink (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/TinyProperty.js:133:10)
at LocalizedStringProperty.unlink (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/ReadOnlyProperty.js:344:23)
at DynamicProperty.onPropertyChange (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/DynamicProperty.js:194:37)
at TinyProperty.emit (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/TinyEmitter.js:95:9)
at Property._notifyListeners (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/ReadOnlyProperty.js:237:23)
at Property.unguardedSet (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/ReadOnlyProperty.js:189:14)
at Property.set (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/ReadOnlyProperty.js:174:12)
at set value [as value] (http://127.0.0.1/continuous-testing/ct-snapshots/1684762546721/chipper/dist/js/axon/js/Property.js:48:11)
[CONSOLE] continuous-test-error
id: "Sparky Node Puppeteer"
Snapshot from 5/22/2023, 7:35:46 AM
@jonathanolson @zepumph and I reproduced this problem by:
Buoyancy => Application screen => Cement => set volume and mass to 0 => select "custom"
@jonathanolson and @zepumph and I looked into this, and it seems the problem is:
The Material derives a name Property in this code: https://github.com/phetsims/density-buoyancy-common/blob/d0336128a3743915cee7e99a4e57471f5b475f9e/js/buoyancy/view/DensityReadoutListNode.ts#L29-L38
@jonathanolson hypothesizes that when a re-entrant Property changes from A=>B then B=>C we do not have the correct "oldValue" or miss it somehow.
@jonathanolson was reminded of a scenery case where there was something similar in rendering SVG gradients, where we had to track the oldValue ourselves because it was showing incorrectly in the axon notifications.
@jonathanolson and @zepumph agree we need a unit test to exercise this case and see if there is an incorrect or missing oldValue
in this re-entrant case.
Patch from discoveries with @jbphet and @marlitas :
Indeed we saw the incorrect order in a listener test:
QUnit.test( 'TEST HELLO', assert => {
const myProperty = new Property( 'a',{
reentrant: true
} );
myProperty.link( value => {
if ( value === 'b' ) {
myProperty.value = 'c';
}
} );
myProperty.link( ( value, oldValue ) => console.log( `link result: ${oldValue} => ${value}` ) );
myProperty.value = 'b';
assert.ok( true, 'first test' );
// We hope:
// null => a
// a => b
// b => c
// We think since it is buggy we will instead get:
// null=>a
// b=>c
// a=>b
} );
We proposed a fix for that problem above that has the correct behavior and doesn't use setTimeout. That is working well. However, when we tried to generalize to N levels of re-entrancy, it had very broken behavior:
We feel it overlaps with the intermediate states listed in https://github.com/phetsims/axon/issues/303
We saw that queueMicrotask for all callbacks did have the correct listener order. But that is a scary change.
Here is the one that uses queueMicrotask at every level:
@zepumph and I worked out a pattern in TinyEmitter so that re-entrant emit
s are scheduled in order. It is working really well and resolves this problem. We are wondering if it should be the default behavior and if we can implement it in a way that has low risk for performance or memory problems in the non-re-entrant cases.
I'd like to add that ?stringTest=dynamic
causes an infinite loop in the patch. I'm nervous about what it would take to go "all in" on this, and would probably prefer to just have an option for cases we need it to (even a mutable option for a DynamicProperty to set/assert onto its provided parameter propertyProperty. Still more to think about. I think bringing @jonathanolson into the loop would be helpful from here.
I also added unit tests to Property to show how that fixed things up.
We met with @jonathanolson today and things seemed really promising. We found an infinite loop in Buoyancy with these changes when switching the "Volume units" simulation preference, this patch comments out the maxWidth setter in NumberDisplay to show that that is the bug. @jonathanolson wanted to take a look at that. There is also still that infinite loop to investigate with dynamic stringTest.
Since fixing #73, this issue is breaking CT just about every snapshot.
Just catching up on this issue right now, it seems like there are two problems, and both feel like they are worth @jonathanolson's time in investigating. Co-assigning.
Lots of great investigation here today. @jonathanolson and @AgustinVallejo and I were able to reproduce with steps in https://github.com/phetsims/buoyancy/issues/67#issuecomment-1735930418. This is far from the first time that reentrancy has meant that we can't trust the "oldValue" of the Property. For example, it was worked around in https://github.com/phetsims/scenery/commit/3f1cd688. So above @jonathanolson added another workaround that doesn't solve the underlying problem, but does solve the assertion we were getting by keeping track of the actually last set Property (old value) instead of trusting the value that comes from the Property listener callback parameter.
@jonathanolson and I were still worried about how this is quite buggy for DynamicProperty still in cases where the reentrant cases need to take precedent as the listened-to Property, since it will be swapped out for a previous propertyProperty
. Basically this won't be solved until we have a general solution in https://github.com/phetsims/axon/issues/447.
BUT, it doesn't effect this case in buoyancy. This is because all the reentrant cases just HAPPEN to be mapping to the same value (all new instance of a custom Material have the same value for the nameProperty and the density as the combobox settles on the new value). This is totally happen stance, but at least we can close this issue. CT is clear, and we know how to proceed generally. Closing