There is an optimization that tried to synchronously resolve the "resume" [0]promise without sending it down the MSG, but it means that if the AudioContext has been suspended in the same event loop turn (which means the state has not been propagated back), then we return early where we should not. We now have a main thread flag that lets us know which one has been called last, so we can use it. Also there is the opposite case in the opposite direction. https://dxr.mozilla.org/mozilla-central/rev/77cead2cd20300623eea2416bc9bce4d5021df09/dom/media/webaudio/AudioContext.cpp#916

Review commit: https://reviewboard.mozilla.org/r/50419/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50419/

Review commit: https://reviewboard.mozilla.org/r/50417/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50417/

Paul - made a guess as to priority

Sorry about the slow response here. I think I'm going to need to have another look tomorrow to understand how the threads are interacting here.

https://reviewboard.mozilla.org/r/50417/#review57184 ::: dom/media/webaudio/AudioContext.cpp:869 (Diff revision 1) > - if (mAudioContextState == AudioContextState::Suspended) { > + if (mAudioContextState == AudioContextState::Suspended && > + mSuspendCalled) { It seems that the problem here is caused by bypassing the graph communication. The proposed solution is to change the conditions tested when deciding whether to bypass the graph communication. The new conditions select out one situation where this would cause problems, but leaves other inconsistencies remaining. I think it would be better to never bypass the graph communication. Even if there is a situation where we know it won't cause any problems, I doubt the optimization is needed. Can these shortcuts just be removed? Consider the following scenarios with this patch: Scenario I: 1. While the graph is running with state == running, suspend() is called and returns promise A. 2. suspend() is called again and returns promise B. 3. state transitions to suspended. 4. promise A is fulfilled. 5. suspend() is called again and returns promise C. Expected: 6. promise B is fulfilled. 7. promise C is fulfilled. Actual: 6. promise C is fulfilled. 7. promise B is fulfilled. Scenario II: 1. While the graph is running with state == running, suspend() is called and returns promise A. 2. resume() is called and returns promise B. 3. resume() is called and returns promise C. Expected 4. state transitions to suspended. 5. promise A is fulfilled. 6. state transitions to running. 7. promise B is fulfilled. 8. promise C is fulfilled. Actual 4. promise C is fulfilled. 5. state transitions to suspended. 6. promise A is fulfilled. 7. state transitions to running. 8. promise B is fulfilled.

Comment on attachment 8748613 [details] MozReview Request: Bug 1269741 - Allow resuming a suspended AudioContext in the same event loop run. r?karlt https://reviewboard.mozilla.org/r/50417/#review57186

Comment on attachment 8748612 [details] Bug 1269741 - Test. https://reviewboard.mozilla.org/r/50419/#review57188 ::: dom/media/webaudio/test/test_audioContextSuspendResumeClose.html:371 (Diff revision 1) > } > > +function testSuspendResumeEventLoop() { > + var ac = new AudioContext(); > + var lastCurrentTime = ac.currentTime; > + var intervalHandle; I guess intervalHandle is not required. ::: dom/media/webaudio/test/test_audioContextSuspendResumeClose.html:372 (Diff revision 1) > + var tries = 20; > + var equal = 10; > + // Check the time still increases > + function check() { > + var currentCurrentTime = ac.currentTime; > + tries--; > + if (lastCurrentTime == currentCurrentTime) { > + equal--; > + if (!equal) { > + ok(false, "The AudioContext did not resume."); > + finish(); > + return; > + } > + } > + if (!tries) { > + ok(true, "The AudioContext did resume."); > + finish(); > + return; > + } IIUC this is checking that time advances 20 times before there are 10 iterations where it doesn't advance. The timeouts are chosen so that the expected advance before the second onstatechange does not trip the 20 count, and so that currentTime is likely to usually advance in one iteration. What do you think about instead starting and stopping an OscillatorNode quickly after suspend() and looking for the ended event? That would remove all question about whether the advance happened before the suspend was effected and whether currentTime will be updated frequently enough. If the ended event is not dispatched then the test would fail due to the default test timeout. ::: dom/media/webaudio/test/test_audioContextSuspendResumeClose.html:400 (Diff revision 1) > + ac.onstatechange = null; > + > + ok(ac.state == "running", "initial state is running"); > + ac.suspend(); > + ac.resume(); > + intervalHandle = setTimeout(check, 100); intervalHandle set here but not used.

https://reviewboard.mozilla.org/r/50419/#review57188 > IIUC this is checking that time advances 20 times before there are 10 iterations where it doesn't advance. > > The timeouts are chosen so that the expected advance before the second onstatechange does not trip the 20 count, and so that currentTime is likely to usually advance in one iteration. > > What do you think about instead starting and stopping an OscillatorNode quickly after suspend() and looking for the ended event? > > That would remove all question about whether the advance happened before the suspend was effected and whether currentTime will be updated frequently enough. > > If the ended event is not dispatched then the test would fail due to the default test timeout. Or perhaps better than stop() on an OscillatorNode would be an AudioBufferSourceNode with a short buffer. That would avoid any problems picking a stop() time if 0 doesn't work. Or perhaps simplest is to create a ScriptProcessorNode after suspend.

Comment on attachment 8748612 [details] Bug 1269741 - Test. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50419/diff/1-2/

Comment on attachment 8748612 [details] Bug 1269741 - Test. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50419/diff/2-3/

Review commit: https://reviewboard.mozilla.org/r/60790/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60790/

You suggestion works fine indeed (both in the test and in the implementation), thanks !

Comment on attachment 8748612 [details] Bug 1269741 - Test. https://reviewboard.mozilla.org/r/50419/#review57614 ::: dom/media/webaudio/test/test_audioContextSuspendResumeClose.html:368 (Diff revision 3) > + var lastCurrentTime = ac.currentTime; > + var tries = 20; > + var equal = 10; These variables can be removed now. ::: dom/media/webaudio/test/test_audioContextSuspendResumeClose.html:385 (Diff revision 3) > + > + ok(ac.state == "running", "initial state is running"); > + ac.suspend(); > + source.start(); > + ac.resume(); > + lastCurrentTime = ac.currentTime; And this line.

Comment on attachment 8765373 [details] Bug 1269741 - Allow resuming a suspended AudioContext in the same event loop run. https://reviewboard.mozilla.org/r/60790/#review57616 Thank you, Paul!

Pushed by paul@paul.cx: https://hg.mozilla.org/integration/mozilla-inbound/rev/cc90d9d43123 Test. r=karlt https://hg.mozilla.org/integration/mozilla-inbound/rev/7b0517746549 Allow resuming a suspended AudioContext in the same event loop run. r=karlt

https://hg.mozilla.org/mozilla-central/rev/cc90d9d43123 https://hg.mozilla.org/mozilla-central/rev/7b0517746549