search

phetsims / charges-and-fields

"Charges And Fields" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
9 stars 7 forks source link

CT currentNode should have a parent #187

Open KatieWoe opened 3 years ago

KatieWoe commented 3 years ago 
charges-and-fields : phet-io-studio-fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1596408425822/studio/?sim=charges-and-fields&phetioDebug&fuzz&wrapperContinuousTest=%7B%22test%22%3A%5B%22charges-and-fields%22%2C%22phet-io-studio-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1596408425822%22%2C%22timestamp%22%3A1596434554940%7D
Uncaught Error: currentNode should have a parent
Error: currentNode should have a parent
at window.assert (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1596408425822/phet-io-wrappers/common/js/assert.js:32:13)
at Select.expandAncestorsForTreeNode (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1596408425822/studio/js/Select.js:151:17)
at Select.selectInTreeAndRerender (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1596408425822/studio/js/Select.js:127:10)
at HTMLParagraphElement.<anonymous> (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1596408425822/studio/js/TreeNode.js:324:21)
at HTMLTree.fuzzOnce (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1596408425822/studio/js/studioFuzz.js:96:34)
at https://bayes.colorado.edu/continuous-testing/ct-snapshots/1596408425822/studio/js/studioFuzz.js:32:21
id: Bayes Chrome
Snapshot from 8/2/2020, 4:47:05 PM
zepumph commented 3 years ago

This is only when fuzzing studio, assigning the PhET-iO team.

samreid commented 3 years ago

This looks like a rare problem, and has occurred for the following phet-io ids:

That is, group members.

I'll label high priority but only because it is affecting CT, please reprioritize if you disagree.

samreid commented 3 years ago

@chrisklus and I worked on this today and saw that the fuzzer is calling click on a tree element corresponding to a TreeNode with isDisposed=true. We will be adding an assertion to assert that selectInTreeAndRerender arguments are not already disposed. Then we recommend putting on a "band aid" to avoid calling click on disposed elements until we can get assistance from @zepumph.

samreid commented 3 years ago

I committed the band-aid above. Leaving assigned to @chrisklus to add the assertion at selectInTreeAndRerender and leaving assigned to @zepumph for further investigation. Please promote to a Wednesday topic as necessary.

chrisklus commented 3 years ago

Assertion added above.

zepumph commented 3 years ago

The root cause seems to be a race condition between the fuzzing setInterval in studio and the debouncing of the tree being updated.

Although it is really laggy, I can't reproduce the bug with the following patch:

Index: charges-and-fields/js/charges-and-fields/view/ChargesAndFieldsScreenView.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- charges-and-fields/js/charges-and-fields/view/ChargesAndFieldsScreenView.js (revision 27fded7b941dfbf40c37a372b5d9c06a90debcc5)
+++ charges-and-fields/js/charges-and-fields/view/ChargesAndFieldsScreenView.js (date 1599076898963)
@@ -172,7 +172,7 @@
     const resetAllButton = new ResetAllButton( {

       // do not reset the availableDragBoundsProperty
-      listener: () => model.reset(),
+      // listener: () => model.reset(),
       tandem: tandem.createTandem( 'resetAllButton' )
     } );

Index: studio/js/TreeNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- studio/js/TreeNode.js   (revision 557bfa126e13e52b987778add18d1eec4ae590d8)
+++ studio/js/TreeNode.js   (date 1599076854376)
@@ -323,12 +323,12 @@

       // Band-aid to prevent fuzzing from clicking on an element with a disposed TreeNode
       // TODO: Find the root cause as part of https://github.com/phetsims/charges-and-fields/issues/187
-      if ( !this.isDisposed ) {
+      // if ( !this.isDisposed ) {
         const isExpanded = this.isExpanded;
         studio.select.selectInTreeAndRerender( this, true );
         this.isExpanded = !isExpanded;
         studio.updateTreePanel(); // don't debounce to keep things snappy
-      }
+      // }
     } );

     return p;
Index: studio/js/studio.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- studio/js/studio.js (revision 557bfa126e13e52b987778add18d1eec4ae590d8)
+++ studio/js/studio.js (date 1599077130906)
@@ -107,7 +107,7 @@
     this.debouncedUpdateControlPanel = _.debounce( this.updateControlPanel.bind( this ), 200, { leading: true } );

     // @private {function}
-    this.debouncedUpdateTreePanel = _.debounce( () => this.updateTreePanel(), 500, { leading: true } );
+    this.debouncedUpdateTreePanel = ()=>{this.updateTreePanel();};

     // Overrides specified by studio or loaded from the previous override file
     this.phetioElementsOverrides = overridesData;

Probably the best way would be to rewrite StudioFuzz to be based on the TreeNode tree, instead of the HTML tree. Let me see what I can do.

zepumph commented 3 years ago

After further investigation, I think the full solution for this would be to not use HTMLFuzzer for the studio tree. Instead we should get the clickable elements by looking at the TreeNode tree. Then we have info about isDisposed etc, and can navigate the race condition causing this problem more elegantly. This should be fixed by that issue. I will mark this issue on hold until that is solved. Once done, we should be able to remove the workaround you two added in.