Phase 1: remove the guard from Property.unlink

samreid commented 7 years ago

From https://github.com/phetsims/axon/issues/129 we would like Property.unlink to throw an error you try to unlink something that wasn't linked. Emitter supports this but we need to remove the guard in Property.unlink. When I do so in my working copy, I see these errors:

build-an-atom Uncaught TypeError: Cannot read property 'x' of undefined TypeError: Cannot read property 'x' of undefined at Vector2.minus (http://localhost/dot/js/Vector2.js?bust=1502561957304:390:37) at MovableDragHandler.movableDragHandlerDrag (http://localhost/scenery-phet/js/input/MovableDragHandler.js?bust=1502561957304:77:33) at Object.move (http://localhost/scenery/js/input/SimpleDragHandler.js?bust=1502561957304:150:29) at Input.dispatchToListeners (http://localhost/scenery/js/input/Input.js?bust=1502561957304:790:72) at Input.dispatchEvent (http://localhost/scenery/js/input/Input.js?bust=1502561957304:760:12) at Input.branchChangeEvents (http://localhost/scenery/js/input/Input.js?bust=1502561957304:688:14) at Input.moveEvent (http://localhost/scenery/js/input/Input.js?bust=1502561957304:653:26) at Input.mouseMove (http://localhost/scenery/js/input/Input.js?bust=1502561957304:353:12) at Display.fuzzMouseEvents (http://localhost/scenery/js/display/Display.js?bust=1502561957304:1245:23) at Sim.stepSimulation (http://localhost/joist/js/Sim.js?bust=1502561957304:808:22)

circuit-construction-kit-dc-virtual-lab Uncaught Error: Assertion failed: tried to removeListener on something that wasnt a listener Error: Assertion failed: tried to removeListener on something that wasnt a listener at window.assertions.assertFunction (http://localhost/assert/js/assert.js:22:13) at Emitter.removeListener (http://localhost/axon/js/Emitter.js?bust=1502561980557:109:17) at NumberProperty.unlink (http://localhost/axon/js/Property.js?bust=1502561980557:267:29) at ResistorNode.disposeResistorNode (http://localhost/circuit-construction-kit-common/js/view/ResistorNode.js?bust=1502561980557:235:35) at ResistorNode.dispose (http://localhost/circuit-construction-kit-common/js/view/ResistorNode.js?bust=1502561980557:262:12) at ResistorNode.Node.dispose (http://localhost/scenery/js/nodes/Node.js?bust=1502561980557:526:22) at Array. (http://localhost/circuit-construction-kit-common/js/view/CircuitLayerNode.js?bust=1502561980557:271:30) at ObservableArray._fireItemRemoved (http://localhost/axon/js/ObservableArray.js?bust=1502561980557:160:18) at ObservableArray.remove (http://localhost/axon/js/ObservableArray.js?bust=1502561980557:200:14) at http://localhost/circuit-construction-kit-common/js/view/CircuitElementNode.js?bust=1502561980557:182:40

energy-forms-and-changes Uncaught Error: Assertion failed: tried to removeListener on something that wasnt a listener Error: Assertion failed: tried to removeListener on something that wasnt a listener at window.assertions.assertFunction (http://localhost/assert/js/assert.js:22:13) at Emitter.removeListener (http://localhost/axon/js/Emitter.js?bust=1502561999261:109:17) at Property.unlink (http://localhost/axon/js/Property.js?bust=1502561999261:267:29) at Beaker.reset (http://localhost/energy-forms-and-changes/js/intro/model/UserMovableModelElement.js?bust=1502561999261:58:40) at Beaker.reset (http://localhost/energy-forms-and-changes/js/intro/model/RectangularThermalMovableModelElement.js?bust=1502561999261:112:47) at Beaker.reset (http://localhost/energy-forms-and-changes/js/common/model/Beaker.js?bust=1502561999261:385:61) at BeakerHeater.deactivate (http://localhost/energy-forms-and-changes/js/energy-systems/model/BeakerHeater.js?bust=1502561999261:310:19) at http://localhost/energy-forms-and-changes/js/energy-systems/model/EnergySystemElementCarousel.js?bust=1502561999261:85:19 at Array.forEach () at Array. (http://localhost/energy-forms-and-changes/js/energy-systems/model/EnergySystemElementCarousel.js?bust=1502561999261:84:30)

gravity-force-lab Uncaught TypeError: tandem.createTandem is not a function TypeError: tandem.createTandem is not a function at Mass.InverseSquareLawObject [as constructor] (http://localhost/inverse-square-law-common/js/model/InverseSquareLawObject.js?bust=1502562094259:47:22) at new Mass (http://localhost/inverse-square-law-common/js/model/Mass.js?bust=1502562094259:42:28) at new GravityForceLabModel (http://localhost/gravity-force-lab/js/gravity-force-lab/model/GravityForceLabModel.js?bust=1502562094259:61:17) at Screen.createModel (http://localhost/gravity-force-lab/js/gravity-force-lab-main.js?bust=1502562094259:43:18) at Screen.initializeModel (http://localhost/joist/js/Screen.js?bust=1502562094259:210:26) at Array. (http://localhost/joist/js/Sim.js?bust=1502562094259:666:18) at http://localhost/joist/js/Sim.js?bust=1502562094259:677:27

gravity-force-lab-basics Uncaught TypeError: tandem.createTandem is not a function TypeError: tandem.createTandem is not a function at Mass.InverseSquareLawObject [as constructor] (http://localhost/inverse-square-law-common/js/model/InverseSquareLawObject.js?bust=1502562095283:47:22) at new Mass (http://localhost/inverse-square-law-common/js/model/Mass.js?bust=1502562095283:42:28) at new GravityForceLabBasicsModel (http://localhost/gravity-force-lab-basics/js/gravity-force-lab-basics/model/GravityForceLabBasicsModel.js?bust=1502562095283:59:17) at Screen.createModel (http://localhost/gravity-force-lab-basics/js/gravity-force-lab-basics-main.js?bust=1502562095283:45:20) at Screen.initializeModel (http://localhost/joist/js/Screen.js?bust=1502562095283:210:26) at Array. (http://localhost/joist/js/Sim.js?bust=1502562095283:666:18) at http://localhost/joist/js/Sim.js?bust=1502562095283:677:27

isotopes-and-atomic-mass Uncaught TypeError: Cannot read property 'x' of undefined TypeError: Cannot read property 'x' of undefined at Vector2.minus (http://localhost/dot/js/Vector2.js?bust=1502562105846:390:37) at MovableDragHandler.movableDragHandlerDrag (http://localhost/scenery-phet/js/input/MovableDragHandler.js?bust=1502562105846:77:33) at Object.move (http://localhost/scenery/js/input/SimpleDragHandler.js?bust=1502562105846:150:29) at Input.dispatchToListeners (http://localhost/scenery/js/input/Input.js?bust=1502562105846:790:72) at Input.dispatchEvent (http://localhost/scenery/js/input/Input.js?bust=1502562105846:760:12) at Input.branchChangeEvents (http://localhost/scenery/js/input/Input.js?bust=1502562105846:688:14) at Input.moveEvent (http://localhost/scenery/js/input/Input.js?bust=1502562105846:653:26) at Input.mouseMove (http://localhost/scenery/js/input/Input.js?bust=1502562105846:353:12) at Display.fuzzMouseEvents (http://localhost/scenery/js/display/Display.js?bust=1502562105846:1245:23) at Sim.stepSimulation (http://localhost/joist/js/Sim.js?bust=1502562105846:808:22)

**least-squares-regression Uncaught Error: Assertion failed: tried to removeListener on something that wasnt a listener Error: Assertion failed: tried to removeListener on something that wasnt a listener at window.assertions.assertFunction (http://localhost/assert/js/assert.js:22:13) at Emitter.removeListener (http://localhost/axon/js/Emitter.js?bust=1502562117006:109:17) at Property.unlink (http://localhost/axon/js/Property.js?bust=1502562117006:267:29) at DataPoint.dataPoint.returnedToOriginListener (http://localhost/least-squares-regression/js/least-squares-regression/model/LeastSquaresRegressionModel.js?bust=1502562117006:217:36) at Array. (http://localhost/least-squares-regression/js/least-squares-regression/model/LeastSquaresRegressionModel.js?bust=1502562117006:222:19) at Emitter.emit (http://localhost/axon/js/Emitter.js?bust=1502562117006:163:49) at DataPoint.animate (http://localhost/least-squares-regression/js/least-squares-regression/model/DataPoint.js?bust=1502562117006:91:38) at Array.dataPoint.userControlledListener (http://localhost/least-squares-regression/js/least-squares-regression/model/LeastSquaresRegressionModel.js?bust=1502562117006:204:21) at Emitter.emit2 (http://localhost/axon/js/Emitter.js?bust=1502562117006:200:49) at BooleanProperty._notifyListeners (http://localhost/axon/js/Property.js?bust=1502562117006:204:29)

I also saw red in trig tour and unit rates, but somehow it was truncated from this report.

samreid commented 7 years ago

Maybe it would be best to chat about this strategy in developer meeting before "going for it".

samreid commented 7 years ago

@jonathanolson recommends an axon branch that removes the guard, then we can start chipping away at these and see how it goes.

Each sim developer can tackle their own sim.

samreid commented 7 years ago

I'll create a branch and put a chip-away tag.

samreid commented 7 years ago

Here's a more checklist friendly list. Note that not all of these failures were due to axon issues (some sims were failing for other reasons, but should still be fixed):

[x] balancing-act @jbphet
[x] build-a-molecule @jonathanolson
[x] build-an-atom @jbphet
[x] charges-and-fields @jonathanolson
[x] circuit-construction-kit-black-box-study @samreid
[x] circuit-construction-kit-dc @samreid
[x] circuit-construction-kit-dc-virtual-lab @samreid
[x] curve-fitting @pixelzoom
[x] energy-forms-and-changes @jbphet (fixed by @pixelzoom)
[x] fractions-intro @jonathanolson
[x] friction @jbphet (fixed by @pixelzoom)
[x] gravity-force-lab @jessegreenberg
[x] gravity-force-lab-basics @jessegreenberg
[x] isotopes-and-atomic-mass @jbphet
[x] least-squares-regression @jessegreenberg
[x] optics-lab @samreid

samreid commented 7 years ago

I put names next to the checkboxes above based on recent commits, please change it if I've got something wrong.

samreid commented 7 years ago

I assigned people with checkboxes, please unassign yourself after you are checked off.

pixelzoom commented 7 years ago

@samreid said:

I'll create a branch and put a chip-away tag.

Please clarify how we're supposed to proceed with fixing things. I thought the idea was to remove the unlink guard in a branch, then use that branch. I see no such branch.

pixelzoom commented 7 years ago

Btw... Even if the change for Phase 1 is just this:

      unlink: function( listener ) {
//        if ( this.changedEmitter.hasListener( listener ) ) {
          this.changedEmitter.removeListener( listener );
//        }
      },

I think that we should still have a branch so that (1) we're all fixing based on the same changes, and (2) we can add additional changes to the branch as we address additional phases.

samreid commented 7 years ago

Right, I still need to create a branch.

samreid commented 7 years ago

I created the branch "unlink-noguard"

samreid commented 7 years ago

we can add additional changes to the branch as we address additional phases.

Oops, I named the branch just for phase 1. Perhaps another branch for phase 2.

samreid commented 7 years ago

I didn't understand the logic in Optics Lab, so I used quick hackish workaround for now until this sim gets some attention (possibly from @veillette) in the future.

samreid commented 7 years ago

After 15 minutes looking into Charges and Fields, and seeing how the dispose method is called twice and trying to unlink listeners from a disposed property, I decided to unassign myself and @zepumph until we can have a group discussion about who will become the responsible dev for that sim. This will not be a quick fix and it would be good to choose a developer who will become more familiar with that sim.

pixelzoom commented 7 years ago

@samreid said:

Oops, I named the branch just for phase 1. Perhaps another branch for phase 2.

Rename the branch. We should use the same branch for all phases, and progressively make changes in that branch.

samreid commented 7 years ago

I renamed the branch to "listeners"

jessegreenberg commented 7 years ago

Not seeing this for gravity-force-lab and gravity-force-lab-basics, those were probably in the category of "some sims were failing for other reasons, but should still be fixed".

jonathanolson commented 7 years ago

I think https://github.com/phetsims/phetcommon/commit/57d297ada8eab6afb99fa7acc73836f6e0211dda broke some built sims, seeing fuzz failures in BAM/BAA/IAAM (or something around that similar time).

pixelzoom commented 7 years ago

@jonathanolson said:

I think phetsims/phetcommon@57d297a broke some built sims ...

Perhaps it was tested with the branch, but not with master? @jbphet

pixelzoom commented 7 years ago

https://github.com/phetsims/axon/issues/132#issuecomment-323404090 shows:

[ ] charges-and-fields Pending decision in phetsims/phet-info#57

Does anyone know why this is pending the "responsible dev" issue? charges-and-fields has a responsible developer.

jbphet commented 7 years ago

I fixed the issue caused by https://github.com/phetsims/phetcommon/commit/57d297a - I'd added a bad assert statement. I just tested Build an Atom and Isotopes and Atomic Mass with the branch (targeted and fuzz testing) and I believe those are new fixed with respect to this issue, so they've been checked off this list in https://github.com/phetsims/axon/issues/132#issuecomment-323404090.

samreid commented 7 years ago

Does anyone know why [charges-and-fields] is pending the "responsible dev" issue? charges-and-fields has a responsible developer.

Probably a mistake on my part, I reassigned that checkbox to @jonathanolson

pixelzoom commented 7 years ago

I fixed friction, relatively straightforward. @jbphet feel free to review the above commit.

pixelzoom commented 7 years ago

I fixed energy-forms-and-changes. @jbphet should review, looks like the unlink errors might be symptomatic of other faulty logic (as noted in https://github.com/phetsims/energy-forms-and-changes/issues/36).

jonathanolson commented 7 years ago

First case I'm looking into looks like the assertion pattern prevents the pattern:

someProperty.dispose();
// ...
someProperty.unlink( listener );

since the unlink fails (the dispose removed the listener).

It's not super trivial to reverse the ordering of the calls, due to having to worry about listener order now. It seems super hacky/unclean to just add hasListener checks everywhere (particularly disconcerting since it just gets linked and unlinked once in order).

What have others been doing to remedy this situation?

jonathanolson commented 7 years ago

For the above situation, I'd presumably need to add an "about-to-be-disposed" emitter to a model element, so that views could then dispose themselves first. Seems a bit unfortunate.

pixelzoom commented 7 years ago

What have others been doing to remedy this situation?

Could you just remove the someProperty.unlink, since it's handled by someProperty.dispose?

jbphet commented 7 years ago

@pixelzoom said,

I fixed energy-forms-and-changes. @jbphet should review, looks like the unlink errors might be symptomatic of other faulty logic (as noted in phetsims/energy-forms-and-changes#36).

energy-forms-and-changes is apparently going to be turned over to me after I finish gene-expression-essentials, and I'll review all of the logic in the sim at that time.

jonathanolson commented 7 years ago

What have others been doing to remedy this situation?

Could you just remove the someProperty.unlink, since it's handled by someProperty.dispose?

In general, it wouldn't be safe since you wouldn't be able to dispose the view without disposing the model. In practice, it would work for this particular case. Should I do that?

pixelzoom commented 7 years ago

@jonathanolson said:

In general, it wouldn't be safe since you wouldn't be able to dispose the view without disposing the model.

I'm not clear on what's model vs view in your someProperty example. Can you provide an example of where it wouldn't be safe (in general) to simply omit the unlink call?

pixelzoom commented 7 years ago

9/14/17 dev meeting notes:

@jonathanolson will refactor to address situations like https://github.com/phetsims/axon/issues/132#issuecomment-329059521.

Remaining 3 sims are responsibility of @jonathanolson. Phase 2 patch has now been applied to 'listeners' branch (see #140) so he'll address Phase 1 and Phase 2 at the same time for the remaining sims.

Denz1994 commented 7 years ago

Charges-and-fields has been updated to handle the removal of the guard. Thanks, JO for the help.

jonathanolson commented 7 years ago

build-a-molecule and fractions-intro are now fuzzing nicely with the axon listeners branch. All sims above are checked off.

samreid commented 7 years ago

Since this is complete, I think we can close it and continue in #140 and #129, closing.

phetsims / axon

Phase 1: remove the guard from Property.unlink #132