Random actions cause a suspended AudioContext to resume.

NEW
Assigned to

Status

()

Core
Web Audio
P2
normal
Rank:
18
10 months ago
7 months ago

People

(Reporter: Jason Ellis, Assigned: padenot)

Tracking

54 Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

10 months ago
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

Updated

10 months ago
Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
Component: Audio/Video: Playback → Web Audio
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?
Flags: needinfo?(padenot)
I get the same on OSX.
(Assignee)

Comment 3

9 months ago
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
(Assignee)

Updated

9 months ago
Flags: needinfo?(padenot)
(Assignee)

Updated

9 months ago
Status: UNCONFIRMED → NEW
Rank: 18
Ever confirmed: true
OS: Unspecified → All
Priority: -- → P1
Hardware: Unspecified → All
(Reporter)

Comment 4

9 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Assignee: nobody → padenot
(Assignee)

Comment 7

9 months ago
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 ?
Flags: needinfo?(jlaster)

Comment 8

9 months ago
mozreview-review
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.
Attachment #8892463 - Flags: review?(karlt)

Comment 9

9 months ago
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.
Flags: needinfo?(jlaster)
(Assignee)

Comment 10

8 months ago
> 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

8 months ago
mozreview-review
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
Attachment #8892465 - Flags: review?(jlaster) → review+
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
You need to log in before you can comment on or make changes to this bug.