Bug 216972 - Calling suspend() on audio context with no node does not prevent auto transition to running when first node is created
Summary: Calling suspend() on audio context with no node does not prevent auto transit...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: Safari 13
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 212611
  Show dependency treegraph
 
Reported: 2020-09-25 06:42 PDT by Francois Daoust
Modified: 2020-10-02 18:26 PDT (History)
11 users (show)

See Also:


Attachments
Test HTML page to reproduce the bug (4.72 KB, text/html)
2020-09-25 06:42 PDT, Francois Daoust
no flags Details
Patch (7.67 KB, patch)
2020-10-02 13:39 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (8.21 KB, patch)
2020-10-02 14:16 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Francois Daoust 2020-09-25 06:42:20 PDT
Created attachment 409686 [details]
Test HTML page to reproduce the bug

If one calls suspend() on an audio context before any node has been created, the call succeeds (and state right after is indeed "suspended") but the intent is ignored: the audio context still switches to "running" automatically when a first node gets created. This is counter-intuitive.

Steps to Reproduce:

Run the following code in response to some user gesture. See attached file for an HTML page that contains the test.

const AudioContext = window.AudioContext || window.webkitAudioContext;
const audioContext = new AudioContext();
await audioContext.suspend();
const node = audioContext.createOscillator();

Actual state after creation of the oscillator node is "running". Since the code explicitly called suspend(), I would rather expect the audio context to remain in a "suspended" state.


Working around this behavior is easy, one can either:

1. call suspend() after creating the first node

2. call resume() before suspend() leading to the following (weird) workaround:
const AudioContext = window.AudioContext || window.webkitAudioContext;
const audioContext = new AudioContext();
await audioContext.resume();
await audioContext.suspend();
const node = audioContext.createOscillator();

The attached HTML page renders 3 buttons. First button illustrates the bug. Second and third button illustrate the workarounds.

I note that this may warrant an update to the Web Audio API specification, as it does not explicitly state that the initial transition to running (delayed in Webkit as allowed by the spec) needs to take the [[suspended by user]] flag into account. It seems reasonable to assume that when the developer calls suspend(), they actually mean it, though!
Comment 1 Radar WebKit Bug Importer 2020-10-02 06:43:15 PDT
<rdar://problem/69878834>
Comment 2 Chris Dumez 2020-10-02 08:12:35 PDT
Seems reasonable indeed. I will look into it shortly. Thank you for the bug report.
Comment 3 Chris Dumez 2020-10-02 13:39:39 PDT
Created attachment 410359 [details]
Patch
Comment 4 Darin Adler 2020-10-02 13:49:16 PDT
Comment on attachment 410359 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410359&action=review

> Source/WebCore/Modules/webaudio/BaseAudioContext.h:467
> +    bool m_wasSuspendedByUser { false };

What is "user" here? Code in the webpage? Not sure the word "user" is quite right for that, unless it’s guarded by "in response to user event" or that sort of thing.
Comment 5 Chris Dumez 2020-10-02 13:57:26 PDT
(In reply to Darin Adler from comment #4)
> Comment on attachment 410359 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410359&action=review
> 
> > Source/WebCore/Modules/webaudio/BaseAudioContext.h:467
> > +    bool m_wasSuspendedByUser { false };
> 
> What is "user" here? Code in the webpage? Not sure the word "user" is quite
> right for that, unless it’s guarded by "in response to user event" or that
> sort of thing.

I can rename to m_wasSuspendedByScript for clarity. The reason I had used that name was to match the wording in the spec.
Comment 6 Darin Adler 2020-10-02 14:02:24 PDT
(In reply to Chris Dumez from comment #5)
> The reason I had used
> that name was to match the wording in the spec.

That seems like a reason to stick with that wording, but I must say that calling the webpage code "the user" is peculiar.
Comment 7 Chris Dumez 2020-10-02 14:13:59 PDT
(In reply to Darin Adler from comment #6)
> (In reply to Chris Dumez from comment #5)
> > The reason I had used
> > that name was to match the wording in the spec.
> 
> That seems like a reason to stick with that wording, but I must say that
> calling the webpage code "the user" is peculiar.

Yes, I agree the naming is peculiar. Thinking about this more, I think my preference would be to use a better name in our implementation. I can add a comment next to the data member explaining which flag in the specification it corresponds to.
Comment 8 Chris Dumez 2020-10-02 14:16:47 PDT
Created attachment 410371 [details]
Patch
Comment 9 EWS 2020-10-02 18:26:18 PDT
Committed r267911: <https://trac.webkit.org/changeset/267911>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410371 [details].