Closed Bug 1413098 Opened 7 years ago Closed 6 years ago

Should also block web audio when the pref "media.autoplay.enabled=false"

Categories

(Core :: Web Audio, defect, P2)

Product:
Core
Component:
Web Audio
Version:
Other Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: alwu, Assigned: padenot)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: dev-doc-complete, Whiteboard: [block-ap-v1])

Attachments

(5 files, 4 obsolete files)

Bug 1413098 - part1 : add policy to decide whether allow audio context to start.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1413098 - part2 : add test.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1413098 - part3 : fix returning wrong result from SpecialPowers.getPrivilegedProps.
59 bytes, text/x-review-board-request
jmaher
: review+
padenot
: review+
Details
Bug 1413098 - Part 4 - Rename variable and reorder argument to remove default argument value.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1413098 - Part 5 - Allow starting an AudioContext when gUM has been allowed.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details 
Now web audio could still be autoplay even the pref is false.

STR.
1. set the pref "media.autoplay.enabled=false"
2. open https://alastor0325.github.io/htmltests/non_mse_tests/autoplay_webaudio.html

Expect.
3. should not play web audio

Actual.
3. web audio is playing
Maybe the AudioContext is suspended by default and then users need to manually start web audio by AudioContext.resume()?

However, considering web audio doesn't have the controller interface at most time, not like that the video element or audio element. Not sure whether this proposal is correct or not...

---

Hi, Paul,
Could you give me any suggestion about this issue?
Thanks!
Flags: needinfo?(padenot)
This is in the spec [0]. Please have search around for "allowed to start" to understand how this should behave.

[0]: https://webaudio.github.io/web-audio-api/#dfn-allowed-to-start
Flags: needinfo?(padenot)
Thanks!
By spec, it seems we should initialize AudioContext with state "suspended".
Assignee: nobody → alwu
Yes, this has always been the case, but we _can_ delay the initial transition to "running", waiting, for example, for user interaction.
Making this a P2 since I understand it needs to be done by 59.
Rank: 18
Priority: -- → P2
Comment on attachment 8924865 [details]
Bug 1413098 - AudioContext shouldn't be allowed to start if we disable autoplay.

Hi, Paul,
Not sure whether it's the right way to do, could you give me some feedback?
Thanks!
Attachment #8924865 - Flags: feedback?(padenot)
Comment on attachment 8924865 [details]
Bug 1413098 - AudioContext shouldn't be allowed to start if we disable autoplay.

https://reviewboard.mozilla.org/r/196120/#review201406

This is incomplete. We need broader consensus and to discuss about design, to come up with a unified solution for all this. It isn't time to write code just yet.
Attachment #8924865 - Flags: review-
Attachment #8924865 - Flags: feedback?(padenot)
Hi, Paul,
Except keeping the AudioContext as a "suspended" sate, what are the situations we doesn't define clearly? 

For now, after reading the spec [1], the thing we need to do seems very similar with suspending the AudioContext.
Could you give me some suggestion? 

Thanks!

[1] https://webaudio.github.io/web-audio-api/#dfn-allowed-to-start
Flags: needinfo?(padenot)
`AudioContext`s initially starts with `state == "suspended"`, and then transitions to `state == "running"` quickly. If the `AudioContext` is not being "allowed to start", then there should not be this initial transition to `state == "running"`.

Additionally, this initial transition from `state == "suspended"` to `state == "running"` is supposed to happen later, when the `AudioContext` is "allowed to start", for example in response to user interaction (this is being done, in other implementations, by waiting for something like `AudioBufferSourceNode.start()` to be called from a touch handler.

We don't know what policy we want to put in place just yet however.
Flags: needinfo?(padenot)
Following is my understanding, do you think is there anything missing or incomplete?

The rough idea is we would have a flag which would indicate whether we need to block autoplay media.
If the flag is true, then we won't allow the autoplay media playing.

That means the AudioContext is not "allowed to start"
(1) we would prevent AudioContext transferring to "running" after it's created
(2) when calling AudioContext.resume(), would pend the return promise [1]
(3) should reject anything which wants to start AudioContext
    - I don't know whether there are other ways to start an AudioContext. 
      If we have, could you please tell me and discuss how should we handle those behaviors?

Thanks!
 
[1] https://webaudio.github.io/web-audio-api/#resume
Flags: needinfo?(padenot)
Flags: needinfo?(padenot)
(In reply to Paul Adenot (:padenot) from comment #11)
> We don't know what policy we want to put in place just yet however.

After looking the spec [1], it didn't describe the behavior of AudioBufferSourceNode if AudioContext is not allowed to play. Is that what you mean?

[1] https://webaudio.github.io/web-audio-api/#dom-audiobuffersourcenode-start

---

If the AudioContext is not allowed to play, should it affect other different audio nodes? 
I didn't find the related discussion in the spec.

What I know so far is that, (that is also I want to implement in this bug)

If the AudioContext is not "allowed to start", then it would never become "running" state.
If the user call ac.resume(), the promise would be pending unitil the next time user call ac.resume() and the AudioContext have been "allowed to start". [2]

[2] https://github.com/WebAudio/web-audio-api/pull/991#discussion_r80734990

---

Hi, Paul,
Could you give me any suggestion?
Thanks!
Flags: needinfo?(padenot)
Attachment #8924865 - Attachment is obsolete: true
Now I got a problem, I don't know whether we also need to postpone the returned promise from AudioContext::StartRendering().

Spec doesn't mention related behavior when the AudioContext is not allowed to start.
(In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #14)
> Now I got a problem, I don't know whether we also need to postpone the
> returned promise from AudioContext::StartRendering().
> 
> Spec doesn't mention related behavior when the AudioContext is not allowed
> to start.

Ah, "If the rendering thread state of the AudioContext is not running, return false" [1]
We should also put the returned promise from AudioContext::StartRendering() into pending promise.

[1] step3, https://webaudio.github.io/web-audio-api/#dfn-render-quantum
(In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #15)
> Ah, "If the rendering thread state of the AudioContext is not running,
> return false" [1]
> We should also put the returned promise from AudioContext::StartRendering()
> into pending promise.
> 
> [1] step3, https://webaudio.github.io/web-audio-api/#dfn-render-quantum

Hmm, no.

In "begin offline rendering", spec said that "Resolve promise with [[rendered buffer]]". [1]
It doesn't mention that we need to resolve previous pending promise.

So, the behavior is still unclear for the returned promise from AudioContext::StartRendering() when the AudioContext is not allowed to start...

[1] https://webaudio.github.io/web-audio-api/#dom-offlineaudiocontext-startrendering
`AudioContext` does not have an `startRendering` method. `OfflineAudioContext` does. `OfflineAudioContext` does not do any audio playback.

No need to do anything here.
Flags: needinfo?(padenot)
(In reply to Paul Adenot (:padenot) from comment #17)
> `AudioContext` does not have an `startRendering` method.
> `OfflineAudioContext` does. `OfflineAudioContext` does not do any audio
> playback.
> 
> No need to do anything here.

OK, I'll update the preliminary patch later.

BTW, I can't find a good way to write a robust test for this changing, because the returned promise would be pending forever, instead of being rejected immediately. It causes that the next step is on the unclear status. 

If the promise is resolved, it means tests fail and we can stop it.
If the promise is pending, it might mean the resume is stopped by "allow-to-start" or promise is just not resolved yet (and we still need to wait it).

So, we can't decide when we need to do the next testing step. If we use setTimeout(), the test might be non-stable and easily fail intermittently.
Comment on attachment 8934453 [details]
Bug 1413098 - part1 : add policy to decide whether allow audio context to start

https://reviewboard.mozilla.org/r/205392/#review210956


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: dom/media/webaudio/AudioContext.cpp:701
(Diff revision 1)
>        p->MaybeReject(NS_ERROR_DOM_INVALID_STATE_ERR);
>      }
>  
>      mPromiseGripArray.Clear();
> +
> +    for (auto p : mPendingResumePromises) {

Warning: Loop variable is copied but only used as const reference; consider making it a const reference [clang-tidy: performance-for-range-copy]

    for (auto p : mPendingResumePromises) {
              ^
         const  &

::: dom/media/webaudio/AudioContext.cpp:893
(Diff revision 1)
>  
> +  // Resolve all pending promises once the audio context has been allowed to
> +  // start.
> +  if (mAudioContextState == AudioContextState::Suspended &&
> +      aNewState == AudioContextState::Running) {
> +    for (auto p : mPendingResumePromises) {

Warning: Loop variable is copied but only used as const reference; consider making it a const reference [clang-tidy: performance-for-range-copy]

    for (auto p : mPendingResumePromises) {
              ^
         const  &
(In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #18)
> BTW, I can't find a good way to write a robust test for this changing,
> because the returned promise would be pending forever, instead of being
> rejected immediately. It causes that the next step is on the unclear status. 
> 
> If the promise is resolved, it means tests fail and we can stop it.
> If the promise is pending, it might mean the resume is stopped by
> "allow-to-start" or promise is just not resolved yet (and we still need to
> wait it).
> 
> So, we can't decide when we need to do the next testing step. If we use
> setTimeout(), the test might be non-stable and easily fail intermittently.

You want to have a chrome API, just for testing, that is able to simulate a touch or something like that, and check that the promise is resolved properly when this event happens (but not before). 

As you mention, setTimeout could maybe work, but it's not satisfactory.
Comment on attachment 8934453 [details]
Bug 1413098 - part1 : add policy to decide whether allow audio context to start

https://reviewboard.mozilla.org/r/205392/#review212386

This should simply leverage NotifyWhenGraphStarted [0], calling it only when we're "allowed to start". Also we really want tests for this.

It's also unclear what policy we want for this. Let's chat in Austin.

[0]: https://searchfox.org/mozilla-central/source/dom/media/MediaStreamGraph.cpp#3897
Attachment #8934453 - Flags: review?(padenot) → review-
Comment on attachment 8936688 [details]
Bug 1413098 - part3 : fix returning wrong result from SpecialPowers.getPrivilegedProps.

https://reviewboard.mozilla.org/r/207418/#review213348

::: testing/specialpowers/content/specialpowersAPI.js:1692
(Diff revision 1)
>    // :jdm gets credit for this.  ex: getPrivilegedProps(window, 'location.href');
>    getPrivilegedProps(obj, props) {
>      var parts = props.split(".");
>      for (var i = 0; i < parts.length; i++) {
>        var p = parts[i];
> -      if (obj[p]) {
> +      if (obj[p] != undefined) {

My .js skills are not the best, should we be doing !== instead?  This check in general is great to see and I am glad you found it.
Attachment #8936688 - Flags: review?(jmaher) → review+
Comment on attachment 8936687 [details]
Bug 1413098 - part2 : add test.

Tests would fail on try, need to fix them.
Attachment #8936687 - Flags: review?(padenot)
Now my browser test would fail on *window7 debug*, but works well on other platforms....
Still need to figure out what's happen.
Comment on attachment 8934453 [details]
Bug 1413098 - part1 : add policy to decide whether allow audio context to start

I'll wait until we have tests passing to review.
Attachment #8934453 - Flags: review?(padenot)
Still have no idea why the test case fail... (only on Windows, will pass on Linux and OSX)

The error message is "A promise chain failed to handle a rejection", and this error would also happen when I just simply call AudioContext.resume() in browser test.

I tried different ways and all getting the same result. [1] is to create new AudioContext in content task and then call resume(). [2] is to create AudioContext in advance in the another file and use content.wrappedJSObject to access it. 

And I *didn't* apply patch1 for those tests, it seems there has some problem about using AudioContext.resume in browser test? (or any methods which would return promise).

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b1882b1a26d36063dbe9a2d2f8fb9334b290f16&selectedJob=154675517
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=63b89d25698ea163451cda68d9d47dcf50d58b42&selectedJob=151851974

In addition, if I don't reject the promise in AudioContext::Shutdown() [3], then I won't get the error. However, I can't understand why this part would affect the test. The resume promise should be removed from |mPromiseGripArray| when it's resolved, so |mPromiseGripArray| should be empty at that time.

[3] https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/dom/media/webaudio/AudioContext.cpp#680

---

Paul,
Do you have any idea about this eror?
Thanks!
Flags: needinfo?(padenot)
Can you repro locally on Windows? Maybe the global that has the AudioContext is being closed, and the AudioContext is closed as well ?
Flags: needinfo?(padenot)
(In reply to Paul Adenot (:padenot) from comment #31)
> Can you repro locally on Windows? Maybe the global that has the AudioContext
> is being closed, and the AudioContext is closed as well ?

Unfortunately, it can't reproduce on local, I ran the test on Win7 for a day, and didn't see any failure.
But in this case, we don't have any pending promise when the tab is removed, I don't understand where rejected promises come from.
Whiteboard: block-ap-v1
Whiteboard: block-ap-v1 → [block-ap-v1]
Assignee: alastor0325 → nobody
Assignee: nobody → padenot
Comment on attachment 8953991 [details]
Bug 1413098 - part2 : add test.

https://reviewboard.mozilla.org/r/223154/#review229080


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/content/tests/browser/browser_autoplay_policy_user_gestures.js:121
(Diff revision 1)
> +  }
> +
> +  function checking_audio_context_running_state() {
> +    let ac = content.ac;
> +    return new Promise(resolve => {
> +      setTimeout(() => {

Error: Listen for events instead of settimeout() with arbitrary delay [eslint: mozilla/no-arbitrary-setTimeout]

::: toolkit/content/tests/browser/browser_autoplay_policy_user_gestures.js:133
(Diff revision 1)
> +  function resume_without_supported_user_gestures() {
> +    let ac = content.ac;
> +    let promise = ac.resume();
> +    ac.resumePromises.push(promise);
> +    return new Promise((resolve, reject) => {
> +      setTimeout(() => {

Error: Listen for events instead of settimeout() with arbitrary delay [eslint: mozilla/no-arbitrary-setTimeout]
Comment on attachment 8953993 [details]
Bug 1413098 - Part 4 - Rename variable and reorder argument to remove default argument value.

https://reviewboard.mozilla.org/r/223158/#review229082

::: commit-message-d7823:1
(Diff revision 1)
> +Bug 1413098 - Part 4 - Rename variable and reoder argument to remove default argument value. r?pehrsons

s/reoder/reorder/
Attachment #8953993 - Flags: review?(apehrson) → review+
Paul: Are you working on this bug? We'd like to ship block-autoplay in 62.
Flags: needinfo?(padenot)
I theory yes, but I'm doing other things at the minute. I shouldn't be too far off in the current state though.
Flags: needinfo?(padenot)
Attachment #8936688 - Attachment is obsolete: true
Attachment #8936687 - Attachment is obsolete: true
Attachment #8934453 - Attachment is obsolete: true
Comment on attachment 8971643 [details]
Bug 1413098 - Part 5 - Allow starting an AudioContext when gUM has been allowed.

https://reviewboard.mozilla.org/r/240410/#review246468


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/content/tests/browser/browser_autoplay_policy_user_gestures.js:158
(Diff revision 3)
>  
> +function callGUM(testParameters) {
> +  info("- calling gum with " + JSON.stringify(testParameters.constraints));
> +  if (testParameters.shouldAllowStartingContext) {
> +    return content.navigator.mediaDevices.getUserMedia(testParameters.constraints);
> +  } else {

Error: Unnecessary 'else' after 'return'. [eslint: no-else-return]

::: toolkit/content/tests/browser/browser_autoplay_policy_user_gestures.js:201
(Diff revision 3)
>    await simulateUserGesture(gesture, tab.linkedBrowser);
>  
>    info("- calling resume() again");
>    try {
>      let resumeFunc = gesture.isActivationGesture ?
> -      resume_with_supported_user_gestures :
> +      resume_with_expected_success:

Error: Infix operators must be spaced. [eslint: space-infix-ops]
This is green on try. I don't like the use of fake devices and non-fake devices and various prefs to make things intentionally fail, though, it's clunky.
Comment on attachment 8971643 [details]
Bug 1413098 - Part 5 - Allow starting an AudioContext when gUM has been allowed.

https://reviewboard.mozilla.org/r/240410/#review246900

Impl looks good, but test needs some clarifications and/or fixes.

::: toolkit/content/tests/browser/browser_autoplay_policy_user_gestures.js:159
(Diff revision 3)
> +    // Call gUM, without sucess: we've made it so that only fake requests
> +    // succeed without permission. Return a resolved promise so that the test
> +    // continues.
> +    content.navigator.mediaDevices.getUserMedia(testParameters.constraints);
> +    return Promise.resolve();

I'm not entirely sure what this is intending to do.
It seems to me that it wants to leave gUM in such a state that starting an AudioContext is not allowed. 

Then why call gUM at all?

Could the returned stream to one of these gUM promises get garbage collected before the test finishes? That could lead to intermittents. Do we want to test this explicitly?

At the very least clarify the intention here with a comment.

::: toolkit/content/tests/browser/browser_autoplay_policy_user_gestures.js:162
(Diff revision 3)
> +    return content.navigator.mediaDevices.getUserMedia(testParameters.constraints);
> +  } else {
> +    // Call gUM, without sucess: we've made it so that only fake requests
> +    // succeed without permission. Return a resolved promise so that the test
> +    // continues.
> +    content.navigator.mediaDevices.getUserMedia(testParameters.constraints);

Won't this start a capture and make an AudioContext resumable shortly after the return?

::: toolkit/content/tests/browser/browser_autoplay_policy_user_gestures.js:190
(Diff revision 3)
>    }
>  
>    info("- calling resume() -");
>    try {
>      await ContentTask.spawn(tab.linkedBrowser, null,
> -                            resume_without_supported_user_gestures);
> +                            resume_without_expected_success);

How did this get affected? Fix from another change?

::: toolkit/content/tests/browser/browser_autoplay_policy_user_gestures.js:218
(Diff revision 3)
> +  info("- open new tab -");
> +  let tab = await BrowserTestUtils.openNewForegroundTab(window.gBrowser,
> +                                                        "about:blank");
> +  info("- create audio context -");
> +  // We want the same audio context be used between different content
> +  // tasks, so it *must* need to be loaded by frame script.

Use "must" or "needs to", not both.
Attachment #8971643 - Flags: review?(apehrson)
There's lots of backlash against Chrome's implementation:
https://www.dailydot.com/debug/chrome-autoplay-block-games/

Paul: do we also require AudioContexts to be resumed after documents are gesture activated, or do AudioContexts automatically resume once gesture activation occurs?
Flags: needinfo?(padenot)
See also, https://bugs.chromium.org/p/chromium/issues/detail?id=840866 for the Chromium bug where people are reporting (a lot) of sites that are broken by Chrome's implementation. Most are games or audio/visual art experiences.
(In reply to Chris Pearce (:cpearce) from comment #55)
> There's lots of backlash against Chrome's implementation:
> https://www.dailydot.com/debug/chrome-autoplay-block-games/
> 
> Paul: do we also require AudioContexts to be resumed after documents are
> gesture activated, or do AudioContexts automatically resume once gesture
> activation occurs?

In the current state of this patch, a call to `resume` is required.
Flags: needinfo?(padenot)
We're going to add a permission prompt to request users' consent to autoplay for media which aren't unblocked by gesture activation. I think we should do the same for WebAudio, so that there's an escape hatch to allow sites to work without needing to change.

The HTMLMediaElement work will be happening in bug 1463919.
Comment on attachment 8971643 [details]
Bug 1413098 - Part 5 - Allow starting an AudioContext when gUM has been allowed.

https://reviewboard.mozilla.org/r/240410/#review246900

> I'm not entirely sure what this is intending to do.
> It seems to me that it wants to leave gUM in such a state that starting an AudioContext is not allowed. 
> 
> Then why call gUM at all?
> 
> Could the returned stream to one of these gUM promises get garbage collected before the test finishes? That could lead to intermittents. Do we want to test this explicitly?
> 
> At the very least clarify the intention here with a comment.

Here, we're checking whether having gUM called, but without having the user accepting is enough to allow starting an AudioContext (it should not be).

> Won't this start a capture and make an AudioContext resumable shortly after the return?

This will never succeed, we have disabled permissions for fake devices only.

> How did this get affected? Fix from another change?

I'm just renaming because it's not strictly user gesture that can allow resuming a context, now.
Comment on attachment 8971643 [details]
Bug 1413098 - Part 5 - Allow starting an AudioContext when gUM has been allowed.

https://reviewboard.mozilla.org/r/240410/#review253596

r- for callGUM putting constraint requirements on the caller. Seems like a footgun. Otherwise this looks fine.

::: dom/media/AutoplayPolicy.cpp:97
(Diff revision 4)
> +    nsCOMPtr<nsPIDOMWindowInner> window = aContext->GetParentObject();
> +    if (window && manager->IsActivelyCapturingOrHasAPermission(window->WindowID())) {
> +      return true;
> +    }

Does this work? It's redeclaring `window` from above.

Is GetOwner() different from GetParentObject() in this case?

::: dom/media/AutoplayPolicy.cpp:103
(Diff revision 4)
> +    if (window && manager->IsActivelyCapturingOrHasAPermission(window->WindowID())) {
> +      return true;
> +    }
> +  }
> +
>     nsCOMPtr<nsIPrincipal> principal = aContext->GetParentObject()->AsGlobal()->PrincipalOrNull();

Could just as well fix the indentation here now.

::: toolkit/content/tests/browser/browser_autoplay_policy_user_gestures.js:122
(Diff revision 4)
> -    return new Promise(resolve => {
> +  return new Promise(resolve => {
> -      setTimeout(() => {
> +    setTimeout(() => {
> -        ok(ac.state == "suspended", "audio context is still suspended");
> +      ok(ac.state == "suspended", "audio context is still suspended");
> -        resolve();
> +      resolve();
> -      }, 4000);
> +    }, 2000);
> -    });
> +  });

Possibly this belongs in another bug, but this would help overall.

We normally try to avoid putting logic in the Promise ctor.

Instead it could be simplified as
```
return new Promise(r => setTimeout(r, 2000))
.then(() => is(ac.state, "suspended", "audio context is still suspended"));
```

More obvious with async/await (that this test is already using):
```
await new Promise(r => setTimeout(r, 2000));
is(ac.state, "suspended", "audio context is still suspended");
```

::: toolkit/content/tests/browser/browser_autoplay_policy_user_gestures.js:157
(Diff revision 4)
> +    // Because of the prefs and constraints we've set and passed, this is going
> +    // to allow the window to stat an AudioContext synchronously.

This function doesn't know that the correct prefs and constraints are set to properly expect this.

Currently this function does one thing only, call gUM with the constraints unwrapped. In the first case we just resolve later (but for all the calls in this file we still resolve).

Instead, do the magic (to constraints or prefs or whatnot) needed to get the expected result inside this function, so at the callsite it looks like:

```
  await test_webaudio_with_gum({constraints: {video: true}, shouldAllowStartingContext: true});
  await test_webaudio_with_gum({constraints: {video: true}, shouldAllowStartingContext: false});
```

i.e., no requirement is put on the caller for which constraints must be passed for different values of `shouldAllowStartingContext`.

::: toolkit/content/tests/browser/browser_autoplay_policy_user_gestures.js:158
(Diff revision 4)
>  
> +function callGUM(testParameters) {
> +  info("- calling gum with " + JSON.stringify(testParameters.constraints));
> +  if (testParameters.shouldAllowStartingContext) {
> +    // Because of the prefs and constraints we've set and passed, this is going
> +    // to allow the window to stat an AudioContext synchronously.

s/stat/start/

::: toolkit/content/tests/browser/browser_autoplay_policy_user_gestures.js:166
(Diff revision 4)
> +
> +  // Call gUM, without sucess: we've made it so that only fake requests
> +  // succeed without permission. Return a resolved promise so that the test
> +  // continues.
> +  // We do this to check that it's not merely calling gUM that allows starting
> +  // an AudioContext, it's having it resolved successfuly.

Could this mention user consent or permission instead of "resolved"?

::: toolkit/content/tests/browser/browser_autoplay_policy_user_gestures.js:177
(Diff revision 4)
> +async function test_webaudio_with_user_gesture(gesture) {
>    info("- open new tab -");
>    let tab = await BrowserTestUtils.openNewForegroundTab(window.gBrowser,
>                                                          "about:blank");
>    info("- create audio context -");
> -  // We want the same audio context could be used between different content
> +  // We want the same audio context be used between different content

I think you want these
s/be/to be/
s/between/across/
Attachment #8971643 - Flags: review?(apehrson) → review-
(In reply to Andreas Pehrson [:pehrsons] from comment #65)
> ::: dom/media/AutoplayPolicy.cpp:97
> (Diff revision 4)
> > +    nsCOMPtr<nsPIDOMWindowInner> window = aContext->GetParentObject();
> > +    if (window && manager->IsActivelyCapturingOrHasAPermission(window->WindowID())) {
> > +      return true;
> > +    }
> 
> Does this work? It's redeclaring `window` from above.

Yes, name shadowing is allowed in C++, this is in a different block.

> Is GetOwner() different from GetParentObject() in this case?

No, just a mistake, I've removed the second one and just using the outer `window`.

All the other comments have been addressed.
Comment on attachment 8971643 [details]
Bug 1413098 - Part 5 - Allow starting an AudioContext when gUM has been allowed.

https://reviewboard.mozilla.org/r/240410/#review254326

LGTM

::: dom/media/AutoplayPolicy.cpp:97
(Diff revision 5)
>  
> +  // Pages which have been granted permission to capture WebRTC camera or
> +  // microphone are assumed to be trusted, and are allowed to autoplay.
> +  MediaManager* manager = MediaManager::GetIfExists();
> +  if (manager) {
> +    if (window && manager->IsActivelyCapturingOrHasAPermission(window->WindowID())) {

`(bool)window` is guaranteed true here, so this is a superfluous check.

::: toolkit/content/tests/browser/browser_autoplay_policy_user_gestures.js:126
(Diff revision 5)
> -  function resume_without_supported_user_gestures() {
> +function resume_without_expected_success() {
> -    let ac = content.ac;
> +  let ac = content.ac;
> -    let promise = ac.resume();
> +  let promise = ac.resume();
> -    ac.resumePromises.push(promise);
> +  ac.resumePromises.push(promise);
> -    return new Promise((resolve, reject) => {
> +  return new Promise((resolve, reject) => {
> -      setTimeout(() => {
> +    setTimeout(() => {
> -        if (ac.state == "suspended") {
> +      if (ac.state == "suspended") {
> -          ok(true, "audio context is still suspended");
> +        ok(true, "audio context is still suspended");
> -          resolve();
> +        resolve();
> -        } else {
> +      } else {
> -          reject("audio context should not be allowed to start");
> +        reject("audio context should not be allowed to start");
> -        }
> +      }
> -      }, 4000);
> +    }, 2000);
> -    });
> +  });
> -  }
> +}

The same change you did to "checking_audio_context_running_state" can be done here too.
Attachment #8971643 - Flags: review?(apehrson) → review+
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5031770d70fd
part1 : add policy to decide whether allow audio context to start.  r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/747764af7143
part2 : add test. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/49ef875bd65e
part3 : fix returning wrong result from SpecialPowers.getPrivilegedProps. r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/b12730d42016
Part 4 - Rename variable and reorder argument to remove default argument value. r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c186b3bb909
Part 5 - Allow starting an AudioContext when gUM has been allowed. r=pehrsons
Comment on attachment 8953990 [details]
Bug 1413098 - part1 : add policy to decide whether allow audio context to start.

https://reviewboard.mozilla.org/r/223152/#review255038
Attachment #8953990 - Flags: review?(padenot) → review+
Comment on attachment 8953991 [details]
Bug 1413098 - part2 : add test.

https://reviewboard.mozilla.org/r/223154/#review255042
Attachment #8953991 - Flags: review?(padenot) → review+
Comment on attachment 8953992 [details]
Bug 1413098 - part3 : fix returning wrong result from SpecialPowers.getPrivilegedProps.

https://reviewboard.mozilla.org/r/223156/#review255044
Attachment #8953992 - Flags: review+
Comment on attachment 8953992 [details]
Bug 1413098 - part3 : fix returning wrong result from SpecialPowers.getPrivilegedProps.

https://reviewboard.mozilla.org/r/223156/#review255070

thanks
Attachment #8953992 - Flags: review?(jmaher) → review+
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/autoland/rev/d2f35c824530
part1 : add policy to decide whether allow audio context to start.  r=padenot
https://hg.mozilla.org/integration/autoland/rev/f99e6152df75
part2 : add test. r=padenot
https://hg.mozilla.org/integration/autoland/rev/4f79c817377c
part3 : fix returning wrong result from SpecialPowers.getPrivilegedProps. r=jmaher
https://hg.mozilla.org/integration/autoland/rev/ca0241cca7a5
Part 4 - Rename variable and reorder argument to remove default argument value. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/129aa2ad2b7a
Part 5 - Allow starting an AudioContext when gUM has been allowed. r=pehrsons
Flags: needinfo?(padenot)
Depends on: 1468085
Updated documentation:

https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/autoplay (now includes a note that browsers may offer a preference to disable autoplay, and not to rely on autoplay actually starting media)

https://developer.mozilla.org/en-US/Firefox/Releases/62 (note that autoplay preference now controls both video and audio-only media)

Please let me know if more needs to be done.
Keywords: dev-doc-needed → dev-doc-complete
Blocks: 1489278
Blocks: block-autoplay-webaudio
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: