1375562 - Random actions cause a suspended AudioContext to resume.

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P3)

Description

[Jason Ellis]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

Steps to reproduce:

Go to https://webaudiodemos.appspot.com/wubwubwub/ (not my site but an easy way to reproduce)
Open dev console and run audioCtx.suspend()
Add a breakpoint in any js file (also caused by opening the dev tools, or downloading a file)


Actual results:

Audio playback resumed


Expected results:

Audio Playback should have stayed paused

Comment 1

[Alex Chronopoulos [:achronop]]

I confirm the behavior described here (that audio playback resumes when you add a breakpoint). Is that expected? Is it a webaudio bug or devtools are responsible for that?

Comment 2

[Alex Chronopoulos [:achronop]]

I get the same on OSX.

Comment 3

[Paul Adenot (:padenot)]

It's expected because of the way the devtool implement breakpoints. We can fix it, though.

In short, when a breakpoint is hit, nsGlobalWindow::Suspend and nsGlobalWindow::Resume [0] are called. Those call suspend and resume on the AudioContext. We could imagine passing in a flag that would tell the AudioContext that those suspend/resume call are not coming from content, and that we should treat them separately. 

[0]: http://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#12614

Comment 4

[Jason Ellis]

I will add that I only came across the other methods of triggering the bug (with the devtools) while debugging the original bug I was investigating; resuming being triggered by downloading a file in another tab.  For some reason triggering it by downloading file stopped happening after testing this for a while.  Some of my coworkers were still able to trigger the audio context to resume by downloading a file but haven't been able to, even after reinstalling Firefox.

Comment 5

[Paul Adenot (:padenot)]

Attachment: Bug 1375562 - Don't use AudiContext::Suspend when freezing a page, as it has a bunch of side effects.

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

Comment 6

[Paul Adenot (:padenot)]

Attachment: Bug 1375562 - Test the interaction of AudioContext::Suspend and breakpoints.

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

Comment 7

[Paul Adenot (:padenot)]

Jason, I have mostly no idea what I'm doing here, so I'm going to explain what I want to achieve. I think it somehow works, but I don't know why.

When a breakpoint is set, the document is freezed and then unfreezed (that's what I observe in gdb, I don't know the details). This calls AudioContext::Suspend on all AudioContexts of the document. This is a method that is supposed to do a bunch of stuff (return promise, fire events, change state), usually called from javascript (AudioContext.suspend()).

This is not ideal because when your AudioContext is suspended, then unfreezing a document makes it start again, confusing the user.

I've introduced new methods that effectively do what `AudioContext.suspend` does, but without touching the state, firing events, etc.

Anyways, I wrote a test, that (I think) does the following:
- open the debugger
- create an audiocontext, wait for it to change to "running" state
- suspends it
- set a breakpoint
- check that it's still in "suspended" state
- close the page

does that sound good ?

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8892463 [details]
Bug 1375562 - Don't use AudiContext::Suspend when freezing a page, as it has a bunch of side effects.

https://reviewboard.mozilla.org/r/163442/#review169104

I wondered about just calling Suspend() and Resume() on all streams, but there may be many streams, which makes the approach of this patch to use ApplyAudioContextOperation() more efficient.

::: dom/media/GraphDriver.h:351
(Diff revision 1)
> -                               dom::AudioContextOperation aOperation);
> +                               dom::AudioContextOperation aOperation,
> +                               dom::AudioContextStateCallSource aSource);
>    RefPtr<MediaStream> mStream;
>    void* mPromise;
>    dom::AudioContextOperation mOperation;
> +  dom::AudioContextStateCallSource mSource;

EnqueueStreamAndPromiseForOperation() can create the StreamAndPromiseForOperation only when this is a content operation, making this member, and the whole instance, unnecessary.

::: dom/media/MediaStreamGraph.h:1345
(Diff revision 1)
> -                                  void* aPromise);
> +                                  void* aPromise,
> +                                  dom::AudioContextStateCallSource aSource);

The extra parameter doesn't actually add any new information because aPromise is null iff aSource is chrome.

I think I'd leave out the new parameter and just check aPromise, but I understand if you think this is overloading too much.

AudioCallbackDriver is assuming that there is only a promise if source is content.  If you'd like to keep the extra parameter, then can you assert that, please?

If you'd like to keep the extra parameter, then would you be happy to name it to indicate the difference in behavior rather than the source of operation please?  Something like AudioContextOperationFlags::None/SendStateChange perhaps.

::: dom/media/MediaStreamGraph.cpp:3998
(Diff revision 1)
>        driver->EnqueueStreamAndPromiseForOperation(aDestinationStream,
> -          aPromise, aOperation);
> +          aPromise, aOperation, aSource);
>      } else {
>        // We are resuming a context, but we are already using an
>        // AudioCallbackDriver, we can resolve the promise now.
>        AudioContextOperationCompleted(aDestinationStream, aPromise, aOperation);

The EnqueueStreamAndPromiseForOperation() path sends the state change only when source is content, but the AudioContextOperationCompleted() path always sends the state change.

This feels precarious to me.  Is there a reason why it would always be balanced?

::: dom/media/webaudio/AudioContext.h:195
(Diff revision 1)
>  
> +

Accidental extra new line, maybe?
I don't often see double blank lines in Gecko.

::: dom/media/webaudio/AudioContext.h:357
(Diff revision 1)
> +  // The state of this AudioContext, as seen and set by content. This is
> +  // modified by calling suspend/resume/close on an AudioContext.

This is a bit misleading.  From this comment, I would infer that mAudioContextState is set during suspend/resume/close.  However, it is not set until OnStateChange() runs after the graph has reported completion of the request.

::: dom/media/webaudio/AudioContext.h:360
(Diff revision 1)
> +  // The state of this AudioContext, from content. This is set by Gecko itself
> +  // when it want to suspend/resume a page. This takes precedence over the
> +  // "content" state above.
> +  AudioContextState mChromeAudioContextState;

This is not set or used.

::: dom/media/webaudio/AudioContext.cpp:872
(Diff revision 1)
> +AudioContext::SuspendFromChrome()
> +{
> +  // Already suspended, don't bother suspending again.
> +  if (mAudioContextState == AudioContextState::Suspended ||

If there is a resume in progress, then I assume the AudioContextOperation::Suspend is still required?

Would it be OK to send another AudioContextOperation::Suspend while still suspended?

Perhaps testing mSuspendCalled would be better than testing AudioContextOperation::Suspend, but I don't know that we can be sure that absolutely no events will run in content while suspended by chrome.  If some content events may run, possibly modifying the AudioContext, then it is safer to just always increment the suspend count.  That feels simpler to me too.

Comment 9

[Jason Laster [:jlast]]

Hi :padenot, thanks for the test. You're going int he right direction. I think you might want to try adding a breakpoint to a javascript source, which is where the debugger will pause execution and resume.

Take a look at some of the other tests in the directory for examples of selecting a source in the debugger and adding a breakpoint.

Comment 10

[Paul Adenot (:padenot)]

> The extra parameter doesn't actually add any new information because aPromise is null iff aSource is chrome.

`aPromise` is also `nullptr` when initially creating the `AudioContext`, because we need to do the initial "suspended" -> "running" state transition, and we don't have a promise at this point. 

>The EnqueueStreamAndPromiseForOperation() path sends the state change only when source is content, but the 
> AudioContextOperationCompleted() path always sends the state change.
> This feels precarious to me.  Is there a reason why it would always be balanced?

This is an oversight.

> If there is a resume in progress, then I assume the AudioContextOperation::Suspend is still required?

At first, I wanted to prevent initiating a resume/suspend/close call when stopped on a breakpoint (hence the `mChromeAudioContextState` member that I had added and forgot to remove), and then I went against it, but I can't remember why. I still think that's a good idea.

Comment 11

[Jason Laster [:jlast]]

Comment on attachment 8892465 [details]
Bug 1375562 - Test the interaction of AudioContext::Suspend and breakpoints.

https://reviewboard.mozilla.org/r/163444/#review175942

This looks good. I'd prefer that the test were added to debugger/new/test/mochitest. but this is okay too

Comment 12

[Bulk Bug Changes for mreavy's org]

Mass change P1->P2 to align with new Mozilla triage process

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 15

[Paul Adenot (:padenot)]

Attachment: Bug 1375562 - Allow suspending, resuming and closing an AudioContext without triggering the statechange event and dealing with Promises. r?karlt,alwu

Comment 16

[Paul Adenot (:padenot)]

Attachment: Bug 1375562 - Test the interaction of AudioContext::Suspend and breakpoints. r?jlast

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/dbd18752120b
Allow suspending, resuming and closing an AudioContext without triggering the statechange event and dealing with Promises. r=karlt
https://hg.mozilla.org/mozilla-central/rev/b4fc9c4112a8
Test the interaction of AudioContext::Suspend and breakpoints.  r=jlast

Comment 19

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/dbd18752120b
https://hg.mozilla.org/mozilla-central/rev/b4fc9c4112a8