Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CT Assertion failed: there should be at least as many PDOMDisplays as Displays #46

Closed
KatieWoe opened this issue Jun 15, 2021 · 22 comments

Comments

@KatieWoe
Copy link
Contributor

greenhouse-effect : phet-io-state-fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623792299218/phet-io-wrappers/state/?sim=greenhouse-effect&phetioDebug&fuzz&wrapperContinuousTest=%7B%22test%22%3A%5B%22greenhouse-effect%22%2C%22phet-io-state-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1623792299218%22%2C%22timestamp%22%3A1623795556960%7D
Uncaught Error: Assertion failed
Error: Assertion failed
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623792299218/assert/js/assert.js:25:13)
at PDOMDisplaysInfo.removePDOMDisplays (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623792299218/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:284:15)
at PDOMDisplaysInfo.removePDOMDisplays (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623792299218/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:303:35)
at PDOMDisplaysInfo.removePDOMDisplays (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623792299218/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:303:35)
at PDOMDisplaysInfo.removePDOMDisplays (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623792299218/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:303:35)
at PDOMDisplaysInfo.removePDOMDisplays (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623792299218/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:303:35)
at PDOMDisplaysInfo.removeAllPDOMDisplays (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623792299218/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:236:10)
at PDOMDisplaysInfo.onVisibilityChange (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623792299218/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:119:14)
at LayerModelScreenView.onVisiblePropertyChange (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623792299218/scenery/js/nodes/Node.js:3736:28)
at TinyForwardingProperty.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623792299218/axon/js/TinyEmitter.js:68:48)
id: Bayes Chrome
Snapshot from 6/15/2021, 3:24:59 PM
@jbphet
Copy link
Contributor

jbphet commented Jun 16, 2021

This looks PDOM-related. I'm assigning to @jessegreenberg for now, and he and I will likely take a look in our meeting today.

@jbphet jbphet assigned jessegreenberg and unassigned jbphet Jun 16, 2021
@KatieWoe
Copy link
Contributor Author

This may be related:

greenhouse-effect : phet-io-state-fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1624462178393/phet-io-wrappers/state/?sim=greenhouse-effect&phetioDebug&fuzz&wrapperContinuousTest=%7B%22test%22%3A%5B%22greenhouse-effect%22%2C%22phet-io-state-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1624462178393%22%2C%22timestamp%22%3A1624464085611%7D
Uncaught Error: Assertion failed: there should be at least as many PDOMDisplays as Displays
Error: Assertion failed: there should be at least as many PDOMDisplays as Displays
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1624462178393/assert/js/assert.js:25:13)
at PDOMDisplaysInfo.removePDOMDisplays (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1624462178393/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:284:15)
at PDOMDisplaysInfo.removePDOMDisplays (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1624462178393/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:303:35)
at PDOMDisplaysInfo.removePDOMDisplays (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1624462178393/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:303:35)
at PDOMDisplaysInfo.removePDOMDisplays (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1624462178393/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:303:35)
at PDOMDisplaysInfo.removePDOMDisplays (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1624462178393/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:303:35)
at PDOMDisplaysInfo.removeAllPDOMDisplays (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1624462178393/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:236:10)
at PDOMDisplaysInfo.onVisibilityChange (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1624462178393/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:119:14)
at WavesScreenView.onVisiblePropertyChange (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1624462178393/scenery/js/nodes/Node.js:3736:28)
at TinyForwardingProperty.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1624462178393/axon/js/TinyEmitter.js:68:48)
id: Bayes Chrome
Snapshot from 6/23/2021, 9:29:38 AM

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Jun 28, 2021

By placing a debugger in PDOMDisplaysInfo, it looks like this happens when the WavesScreenView becomes invisible (switching to the PhotonsScreenView). But I canot produce the bug manually in this way, so something else is important in this state to cause the bug.

@jessegreenberg
Copy link
Contributor

I worked on this for a while but never got to the cause. Some notes though:

  • I re-learned that PDOMDisplaysInfo is a class on each Node that tracks whether that Node has some content in the PDOM displayed under a Display which has accessibility enabled. It updates whenever the Node is added/removed, has PDOM content set/removed, visibility changes, and whenever it is added/removed from a new Display. These updates happen synchronously. It is used by PDOMInstance to control whether the actual PDOM element should be hidden.
  • The assertion in this issue always happens when calling removeAllPDOMDisplays (from recursion) on the "Start Sunlight" button in the observation window.

I am not sure why this wouldn't be working in the downstream sim and will probably request some assistance from the PhET-iO team.

@jessegreenberg jessegreenberg changed the title CT Assertion failed CT Assertion failed: there should be at least as many PDOMDisplays as Displays Jul 15, 2021
@jessegreenberg
Copy link
Contributor

We have set supportsInteractiveDescription false in package.json to prevent this assertion for now to move forward with PhET-iO instrumentation.

@jessegreenberg
Copy link
Contributor

Coming back to this with @zepumph, the comment to produce the in #46 (comment) is misleading, as we understand it now we still need to fuzz the sim to see this issue.

@zepumph
Copy link
Member

zepumph commented Oct 6, 2021

We were able to reproduce this problem by checking out these shas, and then fuzzing the state wrapper.

https://phet-dev.colorado.edu/html/greenhouse-effect/1.0.0-dev.12/phet/dependencies.json

UPDATE: We can also reproduce on master, you just need to add ?supportsInteractiveDescription=true

@zepumph
Copy link
Member

zepumph commented Oct 6, 2021

@jessegreenberg and I worked on this, but didn't really make much in the way of progress. We found that it isn't specific to the waves screen, or the sunlight button. We reproduced it when setting the micro screen onto a sim currently on the photons screen. We also got the assertion on the ConcentrationSlider.

Here are two states that triggered this, but I haven't yet tried to just set this on a fresh copy of the sim. I'm not sure they will be helpful, but I want to have them just in case.

photonsScreenConcentrationSlider.txt

wavesScreenSunlightButton.txt

@zepumph zepumph self-assigned this Oct 7, 2021
@zepumph
Copy link
Member

zepumph commented Oct 7, 2021

At PhET-iO meeting today we decided this was high priority because it is blocking description on master here.

@zepumph
Copy link
Member

zepumph commented Oct 27, 2021

As a workaround, just noting here before @jessegreenberg and I start again on this, you can just exclude phet-io state testing on ct with this patch. Then you can add interactiveDescription to this sim and still make progress.

Index: data/phet-io-state-unsupported
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/data/phet-io-state-unsupported b/data/phet-io-state-unsupported
--- a/data/phet-io-state-unsupported	(revision 87a75701144fab1d5d311b164446c41b158c7733)
+++ b/data/phet-io-state-unsupported	(date 1635349914831)
@@ -4,4 +4,5 @@
 energy-skate-park
 energy-skate-park-basics
 fourier-making-waves
+greenhouse-effect
 ratio-and-proportion
\ No newline at end of file

@zepumph
Copy link
Member

zepumph commented Oct 27, 2021

@jessegreenberg, @jonathanolson and I spent some time on this today and made some real progress!

That said, it isn't really good news.. . .

Here is a wrapper that can reproduce this consistently by setting two states over 3 seconds.
html.txt

Run this in a directly inside a repo (like studio/html.html). And then run it with ?sim=greenhouse-effect

We can easily see the paused debugger which demonstrates the issue at hand. Working with @jonathanolson, we found that the issue is because we are undeferring all scenery visibility Properties before notifying any of them. @jonathanolson asserted that scenery is decidedly not set up to support this. It has many assumptions about how the state of the scene graph should be updated after every single operation.

In the particular case that the above html file shows, The screenProperty is being notified, which triggers all children to remove their PDOMDisplays, but the ConcentrationSlider is actually supported to be hidden. The runtime thinks that it is visible (not hidden) because the notification step has not occurred for ConcentrationSlider.visibleProperty.targetProperty.

From here we are sorta stuck. @jonathanolson will create a scenery issue to investigate this, and perhaps add more assertions about the assumptions that scenery has about single operations occurring. From there we will either have to decide if all scenery-related Properties will need to notify immediately when undeferring, perhaps by having the PhET-iO state engine support an opt-out for these Properties, or if scenery needs to be more flexible (@jonathanolson was very nervous about this idea, reasonably so).

We will proceed in that new issue with some more investigation.

Please note that for this particular issue in the above HTML, this patch does fix the immediate problem, but we should not commit this because it is incomplete and just a workaround.

Index: js/accessibility/pdom/PDOMDisplaysInfo.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/pdom/PDOMDisplaysInfo.js b/js/accessibility/pdom/PDOMDisplaysInfo.js
--- a/js/accessibility/pdom/PDOMDisplaysInfo.js	(revision 89b51f05e4bb4c7988912ca2f6e21273a4c6a030)
+++ b/js/accessibility/pdom/PDOMDisplaysInfo.js	(date 1635362343461)
@@ -293,13 +293,21 @@
         this.pdomDisplays.splice( i, 1 );
       }
 
+      // TODO: we shouldn't check on "effective" children, because of the documentation noted in this constructor
+      const children = this.node._children;
+
       // Propagate the change to our children
-      for ( i = 0; i < this.node._children.length; i++ ) {
-        const child = this.node._children[ i ];
-        // NOTE: Since this gets called many times from the RendererSummary (which happens before the actual child
-        // modification happens), we DO NOT want to traverse to the child node getting removed. Ideally a better
-        // solution than this flag should be found.
-        if ( child._pdomDisplaysInfo.canHavePDOMDisplays() && !child._isGettingRemovedFromParent ) {
+      for ( i = 0; i < children.length; i++ ) {
+        const child = children[ i ];
+        // NOTE: RE: _isGettingRemovedFromParent: Since this gets called many times from the RendererSummary (which
+        // happens before the actual child modification happens), we DO NOT want to traverse to the child node
+        // getting removed. Ideally a better solution than this flag should be found.
+        // TODO: canHavePDOMDisplays doesn't seem to cover the case where our child is allowed to have a PDOMDisplay, but doesn't have any pdomInstances at this time.
+        if ( !child.pdomParent && child._pdomDisplaysInfo.canHavePDOMDisplays() && !child._isGettingRemovedFromParent ) {
+
+          // use this.node.getEffectiveChildren()? This is not what pdomDisplays is meant for.
+          // better:  !child.pdomParent
+          //child._pdomDisplaysInfo.pdomDisplays.length > 0
           child._pdomDisplaysInfo.removePDOMDisplays( displays );
         }
       }

@zepumph
Copy link
Member

zepumph commented Oct 28, 2021

This is going to be worked on in phetsims/scenery#1312. Until this is worked on and fixes this issue, it is recommended to remove this sim from CT PhET-iO state testing when working on PDOM-related features (see https://github.com/phetsims/phet-io/blob/a62d88b335459d3c8815c1555bb73944e95a3052/doc/phet-io-instrumentation-technical-guide.md#turn-off-phet-io-state-testing).

Coordinate this change with toggling supportsInteractiveDescription in package.json or as a query parameter. Also @jbphet, please make sure that as you toggle this, make sure to keep around the a11y view.

@jbphet
Copy link
Contributor

jbphet commented Nov 1, 2021

Okay, here are the steps that I took in order to re-enable the a11y view without causing CT failures due to the issue with PCOMDisplays:

  1. I added "supportsInteractiveDescription": true back to package.json
  2. I added this sim to the phet-io-state-unsupported file
  3. I added this sim back to the interactive-description file

@zepumph and @jessegreenberg - Are those the correct steps?

@zepumph
Copy link
Member

zepumph commented Nov 2, 2021

  1. Run grunt update. To make sure the package.json is correct in the development html.

I added this sim back to the interactive-description file

While what you have listened will work. . . .
This will be done automatically each night, or via the manually run generate-data perennial script. Ideally this file is not edited manually.

@zepumph zepumph assigned jbphet and unassigned jessegreenberg and zepumph Nov 2, 2021
@jbphet
Copy link
Contributor

jbphet commented Nov 2, 2021

I ran grunt update and it didn't change package.json, so I think we're probably good.

I'm going to leave this open and deferred until phetsims/scenery#1312 is taken care of, and then will follow up with turning state testing back on.

@zepumph
Copy link
Member

zepumph commented Feb 21, 2022

A fix has been pushed over in phetsims/scenery#1312 (comment), next steps for this issue are to make sure it is being tested on CT in the state wrapper. This will ensure we got the fix correct.

While testing for the fix, I noticed some other state issues, so please test fuzzing the state wrapper locally also.

@jbphet
Copy link
Contributor

jbphet commented Mar 9, 2022

@zepumph - I need a bit of clarification to help me figure out what the next steps are on this issue. One thing you said was:

[The] next steps for this issue are to make sure it is being tested on CT in the state wrapper.

Do I need to turn on this testing? If so, how is that done? I just looked around in the code and found the file phet-io-state-unsupported that looks like it contains a list of sims for which state is unsupported, and it looks like this feeds into CT. But greenhouse-effect isn't on this list, so I'm not sure what needs to be done for this sim.

Also, you said:

I noticed some other state issues, so please test fuzzing the state wrapper locally also.

I just tried running state fuzz testing locally on my machine and, indeed, it hit an assertion fairly quickly. Is this the sort of thing you're referring to? Do I need to correct all such errors and then turn phet-io state fuzz testing on?

@jbphet jbphet assigned zepumph and unassigned jbphet Mar 9, 2022
@zepumph
Copy link
Member

zepumph commented Mar 9, 2022

But greenhouse-effect isn't on this list

I see it here: https://github.com/phetsims/perennial/blob/d972159f0e77099bebb3b4ee1ab449fe422d02cb/data/phet-io-state-unsupported#L13

So what I was noting in this issue, is that although the original assertion is in all likelihood fixed, the state wrapper has other bugs in it that prevent me from knowing 100% that this original bug report is solved. Until greenhouse is removed from phet-io-state-unsupported and tested on CT, and the CT is clear of all bugs (including unrelated ones), I would be hesitant to close this issue, since it isn't my repo and isn't my issue.

There is no rush on supporting the state wrapper, but if it is possible to be supporting state while implementing other features, it may save time in the long run.

Does that make sense?

@zepumph zepumph assigned jbphet and unassigned zepumph Mar 9, 2022
@jbphet
Copy link
Contributor

jbphet commented Apr 15, 2022

I took a little time today to see if I could resolve the errors that this sim is encountering when running the state wrapper test. Unfortunately, the first problem it hit in each of my test runs was a general problem described in https://github.com/phetsims/phet-io/issues/1858, so I think I'm out of luck for working on this at the moment.

I'm going to defer this issue for a while, and will take another look in a couple of weeks.

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Apr 16, 2022

Sorry about that. We expect https://github.com/phetsims/phet-io/issues/1858 to be fixed now. The remaining assertion

If alerting to Voicing, the alertable needs to be an Utterance and have canAnounceProperties

was still present but caused by something else. I have disabled Voicing for GasConcentrationAlerter for now.

@jbphet
Copy link
Contributor

jbphet commented May 7, 2022

After the fix the @jessegreenberg described above (thanks!) I tested and found that errors were being encountered in the "Options" dialog. I fixed these by opting a lot of the controls in this dialog out of registration, since it's temporary anyway. After this I was able to run fuzz testing on the state wrapper for 10 minutes without any issues, so I have removed the sim from the list of those that don't support phet-io state. I believe that this turns on state testing for it.

I'll leave this issue open for a while and will close it if it succeeds in the next several rounds of CT.

@KatieWoe
Copy link
Contributor Author

Closing for phetsims/qa#832 as this seems solved. If this is in error, feel free to close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants