Launched sim freezes on splash screen with Mac + Safari

[Nancy-Salpepi]

Test device
MacBook Air (m1 chip)

Operating System
12.3.1

Browser
Safari

Problem description
For phetsims/qa#799, when making changes in the studio tree and launching sim, the launched sim often freezes on the splash screen. When I tab back to the Studio wrapper, there is an error message. This also happens in master.

This seems to be specific to Mac + Safari. I quickly tried a few other platforms (Mac + Chrome, Win10 + Chrome/Firefox) and didn't see this issue.

Steps to reproduce
When the error message occurs varies. General steps:

1. In the studio wrapper, set any visibleProperty to false.
2. Preview sim
3. Back in studio, set that property back to true.
4. Set a different visibleProperty to false
5. Preview sim
6. Repeat as many times as needed to get the error.

@KatieWoe was able to reproduce following the above steps.

Visuals

studioerrorGOB.mp4

[pixelzoom]

@Nancy-Salpepi or @KatieWoe Could I bother you to investigate a couple of more things?

• Is this reproducible in geometric-optics, or is it specific to geometric-optics-basics?

• Is it reproducible in master?

• If it's reproducible in master, is it reproducible with other PhET-iO sims? (e.g. ph-scale)

[KatieWoe]

To number 1, I was able to reproduce it in Geometric-Optics so it is not specific to basics.

[KatieWoe]

To number 2, it does happen on master.

[KatieWoe]

I didn't see when trying on Molarity in Master. If there are specific sims you want us to try, let me know.

[Nancy-Salpepi]

Is this reproducible in geometric-optics, or is it specific to geometric-optics-basics?

Yes--we were able to reproduce in geometric optics

Is it reproducible in master?

Yes

If it's reproducible in master, is it reproducible with other PhET-iO sims? (e.g. ph-scale)

Yes--I saw it in CCK-DC. I didn't see it in pH-scale (but will test a bit more).

[Nancy-Salpepi]

I also had issues when we were testing Friction with PhET-iO so perhaps it has to do with sound? Friction, GO and CCK all have sound.
phetsims/friction#278

[samreid]

@jonathanolson and I investigated. We could easily reproduce the problem on Safari following the instructions above. It seems like Safari is double-adding listeners. We added this guard:

Index: js/soundManager.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/soundManager.ts b/js/soundManager.ts
--- a/js/soundManager.ts	(revision 550be121be8dbc8b14e7fca9e7380f256f8d6689)
+++ b/js/soundManager.ts	(date 1650386311730)
@@ -307,7 +307,12 @@
         window.addEventListener( 'touchstart', resumeAudioContext, false );
 
         // listen for other user gesture events too
-        Display.userGestureEmitter.addListener( resumeAudioContext );
+        if ( Display.userGestureEmitter.hasListener( resumeAudioContext ) ) {
+          console.log( 'already has listener' );
+        }
+        else {
+          Display.userGestureEmitter.addListener( resumeAudioContext );
+        }
       }
       else {
         console.log( 'AudioContext is now running.' );

And it seemed to get past the problem OK. But note we aren't guarding the window.addEventListener above. So we wanted to reach out to @jbphet about next steps.

[jbphet]

On Slack I said (to @samreid and @jonathanolson):

Okay, in general the fix looks reasonable. Safari's audio context behavior has always been a little different from Chrome and FF, and perhaps something else has now changed. Please make the console.log into a console.warn, add a TODO about there being some pending further investigation, and assign the issue to me, and I'll see what I can figure out. I think it would be fine to fix it this way for the upcoming release.

[samreid]

@jonathanolson and @pixelzoom and I discussed this, and we agree it would be best for @jbphet to collaborate on the long-term fix. We also noted that with fix proposed above, there is still a memory leak since window.addEventListener( 'touchstart', resumeAudioContext, false ); is not guarded.

@pixelzoom said this doesn't prevent progress on the Geometric Optics dev test, so we can wait for a good long-term solution. But it does block the PhET-iO batch release, so I marked it as blocks-publication.

I'll commit the short-term fix and reassign to @jbphet for longer-term investigation.

[samreid]

OK fix pushed. Over to @jbphet for next steps.

[pixelzoom]

@Nancy-Salpepi @KatieWoe note that we transferred this issue from geometric-optics-basics to tambo, since it is a general problem that needs to be addressed in soundManager.ts. Please consider this non-blocking for GO and GO:B dev tests (phetsims/qa#798, phetsims/qa#799).

[KatieWoe]

Belatedly linking to phetsims/qa#798

[jbphet]

I tried for about an hour to get the conditions that were causing this problem to occur on a MacBook pro running MacOS 12.3.1 and Safari 15.4, but was not able to reproduce it. I suspect that there was something going on similar to what is described in #90 where the audio context is going through a different state other than 'suspended' or 'running'. I've seen things like this before, but only on Apple devices.

I think just having a simple guard that doesn't re-add the listener if it's already there is reasonable, which is what @samreid implemented. I just cleaned it up a bit.

@KatieWoe - Can you please have the QA team test this on master and make sure that the problem can't be reproduced anymore?

[jbphet]

I've also assigned @Nancy-Salpepi, since I believe @KatieWoe mentioned something about having a lot going on next week.

[KatieWoe]

I repeated steps that caused the bug in the dev test in master and the bug didn't occur, so I think we're good.

[samreid]

@jbphet did you intend to leave the console.log statements? Do you want to work with me since the problem was replicable on my Safari? And won't there be a memory leak on the window.addEventListener, since it has no guard?

        // Add a listener that will resume the audio context on the next touchstart.
        window.addEventListener( 'touchstart', resumeAudioContext, false );

        // Listen also for other user gesture events that can be used to resume the audio context.
        if ( !Display.userGestureEmitter.hasListener( resumeAudioContext ) ) {
          Display.userGestureEmitter.addListener( resumeAudioContext );
        }

[Nancy-Salpepi]

Both Geometric Optics and CCK-DC look good on Master.

[jbphet]

@samreid - Here are my answers to your questions above:

@jbphet did you intend to leave the console.log statements?

I meant to leave in the one that says "AudioContext is now running". I didn't mean to leave the other one, and have removed it. Thanks for pointing that out.

And won't there be a memory leak on the window.addEventListener, since it has no guard?

I don't think so. I found multiple sources online that said, "If multiple identical EventListener s are registered on the same EventTarget with the same parameters, the duplicate instances are discarded." Also, the listener is removed as soon as the removeAudioContext listener runs.

Do you want to work with me since the problem was replicable on my Safari?

It doesn't seem worth the time in my opinion, since QA has verified that the problem no longer exists.

@samreid - If you're reasonably satisfied with these answers, I think the issue can be closed.

[samreid]

Sounds good, thanks! Closing.