CT Assertion failed: countingObjectType not found!

[chrisklus]

Making a side issue from phetsims/number-play#186 since there were multiple problems in that stack.

[chrisklus]

In phetsims/number-play#33 I added logic to grab counting objects not in ten frames before any objects are removed from ten frames (regardless of how far away things are) when the down arrow is clicked to return a counting object. For this issue I need to properly remove counting objects from ten frames when they are selected to be returned.

[chrisklus]

Fixed in the above commit, closing.

[pixelzoom]

Reopening. This occurred in CT last night:

number-play : multitouch-fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1672892325918/number-play/number-play_en.html?continuousTest=%7B%22test%22%3A%5B%22number-play%22%2C%22multitouch-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1672892325918%22%2C%22timestamp%22%3A1672896955054%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: countingObjectType not found!
Error: Assertion failed: countingObjectType not found!
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1672892325918/assert/js/assert.js:28:13)
at assert (LabScreenView.ts:292:14)
at getCountingObjectType (LabScreenView.ts:377:36)
at getCorrespondingPlayAreaNode (LabScreenView.ts:343:36)
at getCountingObjectNode (DraggableTenFrameNode.ts:57:45)
at Proxy.forEach
at forEach (DraggableTenFrameNode.ts:56:33)
at _start (DragListener.ts:365:26)
at callback (PressListener.ts:727:16)
at apply (PhetioAction.ts:157:16)
id: Bayes Puppeteer
Snapshot from 1/4/2023, 9:18:45 PM

[pixelzoom]

Migrating to number-suite-common, since this involves LabScreenView.

I'll have a look. I'm guessing that this is the same type of problem as phetsims/number-play#195, where a drag is interrupted, and we're trying to remove an object that has already been disposed.

[pixelzoom]

This seems to be specific to multitouch, and it's failing in DraggableTenFrame.ts at line 57:

    this.dragListener = new DragListener( {
      targetNode: this,
      start: () => {
        selectedTenFrameProperty.value = tenFrame;
        this.moveToFront();
        tenFrame.countingObjects.forEach( countingObject => {
57        const countingObjectNode = options.getCountingObjectNode( countingObject );
          countingObjectNode.moveToFront();
        } );
      },

I have not yet been able to reproduce manually on iPad.

I also have not been able to reproduce in master (15 minutes of multitouch fuzzing) using
http://localhost:8080/number-play/number-play_en.html?brand=phet&screens=4&ea&fuzz&fuzzPointers=2&supportsPanAndZoom=false

So this issue is likely either something that rarely occurs (a "corner case") or a case that does not have good coverage by CT.

[pixelzoom]

@chrisklus FYI... Adding blocks-sim-publication label to this issue. While its true that this issue is rare and does not need to be resolved before publication, we do need to ensure that the sim does not crash when the assertion condition is violated.

[pixelzoom]

I went down a path of trying to make the sim behave gracefully when this assertion is violated. But unlike #25, it was not easy to address in one place -- I ended up having to touch many methods. This felt too invasive for a workaround, so bailed. I think my next approach will be to take another stab at identifying the actual cause of the problem.

[pixelzoom]

It's possible that this problem may have been resolved by changes elsewhere. I haven't seen it reported in CT for awhile. And I'm no longer able to reproduce this in master with URL http://localhost:8080/number-play/number-play_en.html?brand=phet&ea&fuzz&fuzzPointers=2&memoryLimit=1000&supportsPanAndZoom=false.

I did proceed with a patch which ensures that calling getCountingObjectType will not cause runtime failure. Here it is for posterity:

patch

Subject: [PATCH] refine comment where PhET-iO instrumentation is not desired, https://github.com/phetsims/calculus-grapher/issues/197
---
Index: js/lab/view/DraggableTenFrameNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/lab/view/DraggableTenFrameNode.ts b/js/lab/view/DraggableTenFrameNode.ts
--- a/js/lab/view/DraggableTenFrameNode.ts	(revision ab839e916049a7a2aad997a993d19480e83dba4b)
+++ b/js/lab/view/DraggableTenFrameNode.ts	(date 1674851695982)
@@ -23,7 +23,7 @@
 type SelfOptions = {
   dropListener: ( tenFrameNode: DraggableTenFrameNode ) => void;
   removeCountingObjectListener: ( countingObject: CountingObject ) => void;
-  getCountingObjectNode: ( countingObject: CountingObject ) => CountingObjectNode;
+  getCountingObjectNode: ( countingObject: CountingObject ) => CountingObjectNode | null;
 };
 type DraggableTenFrameNodeOptions = SelfOptions;
 
@@ -62,7 +62,7 @@
         this.moveToFront();
         tenFrame.countingObjects.forEach( countingObject => {
           const countingObjectNode = options.getCountingObjectNode( countingObject );
-          countingObjectNode.moveToFront();
+          countingObjectNode && countingObjectNode.moveToFront();
         } );
       },
       drag: ( event: PressListenerEvent, listener: DragListener ) => {
@@ -84,16 +84,18 @@
 
     tenFrame.countingObjects.addItemAddedListener( countingObject => {
       const countingObjectNode = options.getCountingObjectNode( countingObject );
+      if ( countingObjectNode ) {
 
-      // make the countingObjectNode pickable:false instead of inputEnabled:false because we still want to be able to
-      // drag this tenFrameNode that the countingObjectNode is on top of
-      countingObjectNode.pickable = false;
+        // make the countingObjectNode pickable:false instead of inputEnabled:false because we still want to be able to
+        // drag this tenFrameNode that the countingObjectNode is on top of
+        countingObjectNode.pickable = false;
 
-      // animate the countingObject to the next available space in the tenFrame
-      countingObject.setDestination( tenFrame.getCountingObjectSpot( countingObject ), true );
+        // animate the countingObject to the next available space in the tenFrame
+        countingObject.setDestination( tenFrame.getCountingObjectSpot( countingObject ), true );
 
-      // make the tenFrame selected so the user knows they can remove the newly added countingObject
-      selectedTenFrameProperty.value = tenFrame;
+        // make the tenFrame selected so the user knows they can remove the newly added countingObject
+        selectedTenFrameProperty.value = tenFrame;
+      }
     } );
 
     tenFrame.countingObjects.addItemRemovedListener( countingObject => {
Index: js/lab/view/LabScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/lab/view/LabScreenView.ts b/js/lab/view/LabScreenView.ts
--- a/js/lab/view/LabScreenView.ts	(revision ab839e916049a7a2aad997a993d19480e83dba4b)
+++ b/js/lab/view/LabScreenView.ts	(date 1674852081141)
@@ -274,22 +274,19 @@
   /**
    * Returns the counting object type of the CountingObjectNode for the given CountingObject
    */
-  private getCountingObjectType( countingObject: CountingObject ): CountingObjectType {
-    let countingObjectType = CountingObjectType.DOG;
-    let countingObjectTypeFound = false;
+  private getCountingObjectType( countingObject: CountingObject ): CountingObjectType | null {
 
-    for ( let i = 0; i < this.playAreaNodes.length; i++ ) {
+    let countingObjectType = null;
+
+    for ( let i = 0; i < this.playAreaNodes.length && !countingObjectType; i++ ) {
       const playAreaNode = this.playAreaNodes[ i ];
-
       if ( playAreaNode.playArea.countingObjects.includes( countingObject ) ) {
         const countingObjectNode = playAreaNode.getCountingObjectNode( countingObject );
         countingObjectType = countingObjectNode.countingObjectTypeProperty.value;
-        countingObjectTypeFound = true;
-        break;
       }
     }
 
-    assert && assert( countingObjectTypeFound, 'countingObjectType not found!' );
+    assert && assert( countingObjectType, 'countingObjectType not found!' );
     return countingObjectType;
   }
 
@@ -337,11 +334,11 @@
         dropListener: dropListener,
         removeCountingObjectListener: countingObject => {
           const playAreaNode = this.getCorrespondingPlayAreaNode( countingObject );
-          playAreaNode.playArea.sendCountingObjectToCreatorNode( countingObject );
+          playAreaNode && playAreaNode.playArea.sendCountingObjectToCreatorNode( countingObject );
         },
         getCountingObjectNode: countingObject => {
           const playAreaNode = this.getCorrespondingPlayAreaNode( countingObject );
-          return playAreaNode.getCountingObjectNode( countingObject );
+          return playAreaNode ? playAreaNode.getCountingObjectNode( countingObject ) : null;
         }
       } );
 
@@ -373,11 +370,14 @@
    * Each type of counting object has its own play area, so when working with a counting object, we need to look up
    * its corresponding play area in order to do an operation on it (like sending the counting object back to its origin).
    */
-  private getCorrespondingPlayAreaNode( countingObject: CountingObject ): CountingPlayAreaNode {
+  private getCorrespondingPlayAreaNode( countingObject: CountingObject ): CountingPlayAreaNode | null {
+    let playAreaNode = null;
     const countingObjectType = this.getCountingObjectType( countingObject );
-    const playAreaNode = this.countingObjectTypeToPlayAreaNode.get( countingObjectType );
-    assert && assert( playAreaNode, 'playAreaNode not found for counting object type: ' + countingObjectType.name );
-    return playAreaNode!;
+    if ( countingObjectType ) {
+      playAreaNode = this.countingObjectTypeToPlayAreaNode.get( countingObjectType )!;
+      assert && assert( playAreaNode, 'playAreaNode not found for counting object type: ' + countingObjectType.name );
+    }
+    return playAreaNode;
   }
 }
 

I did not apply the patch because it does address the root problem. There are other places where things will go badly if a CountingObject's associated CountingObjectNode has already been disposed. For example in LabScreenView.ts getCountingObjectType:

285    const countingObjectNode = playAreaNode.getCountingObjectNode( countingObject );

While this issue could crash the sim, the fact that we can't reproduce easily suggests that it's a corner case, and unlikely to be encountered by the user. So I recommend that we publish with this as a known issue, and defer addressing the root cause in the future. The ideal way to address this would be to overhaul how CountingObjects and CountingObjectNodes are managed, which will certainly be necessary (via PhetioGroup) if/when PhET-iO support is added to these sims.

@chrisklus if that's OK with you, please change the label from "block-sim-publication" to "deferred". If not, let's discuss.

[chrisklus]

@marlitas @pixelzoom and I discussed this today and agreed that this can be deferred. Marking as such and we can reinvestigate later on if this ever pops up on CT.

[chrisklus]

For #72