Audit updateDisplay() for side effects that effect the Node scene graph state

[zepumph]

While working on phetsims/density-buoyancy-common#97 (comment), @jonathanolson mentioned that it is incredibly scary to be listening to Node.changedInstanceEmitter from spots that may effect the state of the scene graph. The three usages that we briefly looked at are all in scenery/js/accessibility.

The bug we ran into over in DBC was awkward because it involved a Property pointing to a Node that had been disposed, but the Property was set up to clear the disposed node correctly, just when the instances changed on updateDisplay. Fuzzing exposed this problem because of the input events occurring after the node was disposed, but before the frame ended.

Next steps to me are to understand the problem better. Most likely this means a conversation with @jonathanolson and @jessegreenberg. I'd also like to see some assertions put in place (even if under assert slow) that demonstrate when or where updateDisplay produces these side effects. Maybe not to commit, but at least to discuss. I'll mark as a high priority until I have a better understanding of the issue. Anyone feel free to change it.

[jessegreenberg]

This seems important but I don't understand the problem yet and usages in scenery/js/accessibility look similar to DisplayedProperty. Let me know if we should meet or if I can help somehow.

[zepumph]

Our investigation started by looking at usages of changedInstanceEmitter:

• FocusDisplayedController.ts: In most cases, this listener is just updated a separate display (specific to the focus overlay). focusHighlightLayerable is the biggest potential for trouble, since it may update a Node in the sim display based on the node's instance changing (added/deleted).
• InteractiveHighlighting.ts
• Voicing.ts
• Node.ts
• DisplayedProperty.js: Only usages are for the audio view. This is fine! We should just update the documentation for this.

More discussion:

• When is this a problem? Is it ok that updateDisplay() causes node state updates to another display? Potentially that is safer, but it may still be buggy. @jonathanolson was worried about potentially having the separate display create/use items that were newly freed to a pool, but may still hold references in the main display before disposal is complete (by the end of updateDisplay()).
• @jonathanolson is interested in investigating moving the removal of Instances to the end of updateDisplay(). Perhaps that would fully fix this class of problem.
• @jessegreenberg: Is there a better pattern that we could use? @jonathanolson: probably, but most likely we trade-off for performance.
• Can we just make the emitter we listen to here fire at the end of updateDisplay instead of during?

Next steps:

• @jonathanolson investigation to see about synchronous and performance-friendly ways to know if a Node is connected to a display (any display?). Perhaps via RendererSummary-style support. I will make a separate issue for this. Investigate a sync way to access if a Node is connected to a Display #1620
• FocusDisplayedController should listen for lastNode() disposal. Then we can remove the workaround added in CT should not be called if disposed density-buoyancy-common#97 (see patch below.
• We now understand better the potential fallbacks of using Node.changedInstanceEmitter. We should go crazy on some documentation to make sure that any future users of the feature are aware of the worry.
• Another issue outlining the potential problems with InteractiveHighlighting when 1 node is used as DAG or in multiple Displays. InteractiveHighlighting does not support DAG or multi-Displays #1619
• Another issue to make Voicing.voicingCanSpeakProperty. private, since it is directly changed from instance counts, and could easily creep into a space where we toggle visibility of a cue node or something. Voicing.voicingCanSpeakProperty should be private. #1618

@zepumph will take care of all items, and @jonathanolson will be assigned that separate issue.

[jessegreenberg]

Here is a patch where we add a listener to the Node's disposal to update the pointerFocusProperty, so that happens synchronously.

Subject: [PATCH] Indicate that items have been sorted, see https://github.com/phetsims/scenery-phet/issues/815
---
Index: js/accessibility/FocusDisplayedController.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/FocusDisplayedController.ts b/js/accessibility/FocusDisplayedController.ts
--- a/js/accessibility/FocusDisplayedController.ts	(revision bfee694e3542ba44fa5bbb7f4ccd556ac038d292)
+++ b/js/accessibility/FocusDisplayedController.ts	(date 1710350416893)
@@ -1,4 +1,4 @@
-// Copyright 2021-2024, University of Colorado Boulder
+// Copyright 2021-2023, University of Colorado Boulder
 
 /**
  * Responsible for setting the provided focusProperty to null when the Focused node either
@@ -22,11 +22,12 @@
   private visibilityTracker: TrailVisibilityTracker | null = null;
 
   // When there is value, we will watch and update when there are changes to the displayed state of the Focus trail.
-  private focusProperty: TProperty<Focus | null> | null;
+  private focusProperty: TProperty<Focus | null>;
 
   // Bound functions that are called when the displayed state of the Node changes.
   private readonly boundVisibilityListener: () => void;
   private readonly boundInstancesChangedListener: ( instance: Instance ) => void;
+  private readonly boundNodeDisposedListener: () => void;
 
   // Handles changes to focus, adding or removing listeners
   private readonly boundFocusListener: ( focus: Focus | null ) => void;
@@ -36,6 +37,7 @@
 
     this.boundVisibilityListener = this.handleTrailVisibilityChange.bind( this );
     this.boundInstancesChangedListener = this.handleInstancesChange.bind( this );
+    this.boundNodeDisposedListener = this.handleNodeDisposed.bind( this );
 
     this.boundFocusListener = this.handleFocusChange.bind( this );
     this.focusProperty.link( this.boundFocusListener );
@@ -58,7 +60,7 @@
    */
   private handleTrailVisibilityChange(): void {
     if ( this.visibilityTracker && !this.visibilityTracker.trailVisibleProperty.value ) {
-      this.focusProperty!.value = null;
+      this.focusProperty.value = null;
     }
   }
 
@@ -68,9 +70,16 @@
    */
   private handleInstancesChange( instance: Instance ): void {
     if ( instance.node && instance.node.instances.length === 0 ) {
-      this.focusProperty!.value = null;
+      this.focusProperty.value = null;
     }
   }
+
+  /**
+   *
+   */
+  private handleNodeDisposed(): void {
+    this.focusProperty.value = null;
+  }
 
   /**
    * Add listeners that watch when the Displayed state of the Node with Focus has changed,
@@ -85,6 +94,7 @@
 
     this.node = focus.trail.lastNode();
     this.node.changedInstanceEmitter.addListener( this.boundInstancesChangedListener );
+    this.node.disposeEmitter.addListener( this.boundNodeDisposedListener );
   }
 
   /**
@@ -99,6 +109,7 @@
     }
     if ( this.node ) {
       this.node.changedInstanceEmitter.removeListener( this.boundInstancesChangedListener );
+      this.node.disposeEmitter.removeListener( this.boundNodeDisposedListener );
       this.node = null;
     }
   }
@@ -107,11 +118,10 @@
 
     // this disposes the TrailVisibilityTracker and removes any listeners on the Node
     this.removeDisplayedListeners();
-    this.focusProperty!.unlink( this.boundFocusListener );
+    this.focusProperty.unlink( this.boundFocusListener );
 
     this.node = null;
     this.visibilityTracker = null;
-    this.focusProperty = null;
   }
 }
 

[zepumph]

Side issues have been created. All that is left here is the above patch, and documentation updates (2 and 3 bullets above).

[zepumph]

When I removed the workaround, prior to applying the patch, I wasn't able to reproduce the problem, so I'll need to come back to this.

[zepumph]

After much more fuzzing today, I could still not reproduce the bug, so I decided to commit the changes. I will monitor CT over in phetsims/density-buoyancy-common#97 to see if my new assertion (replacing the original workaround) is ever triggered.

I will update documentation from here.

[zepumph]

Alright. All remaining work here will be done in side issue. Thanks all!