Closed Bug 1463919 Opened Last year Closed Last year

Request users' approval to play audible media on tabs which are not allowed to play media with sound

Categories

(Core :: Audio/Video: Playback, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 2 obsolete files)

We've decided to prompt the user to approve/deny playing audible media on tab which have not had user interaction.

This bug covers landing the code send the request from HTMLMediaElement to front end.
Comment on attachment 8988064 [details]
Bug 1463919 - Have HTMLMediaElement ask for autoplay permission when playback otherwise blocked.

https://reviewboard.mozilla.org/r/253304/#review259930

::: dom/html/HTMLMediaElement.h:59
(Diff revision 1)
>  class MediaInputPort;
>  class MediaStream;
>  class MediaStreamGraph;
>  class VideoFrameContainer;
>  namespace dom {
> +class AutoplayRequest;

Should that be in the DOM namespace?

This isn't a DOM object.

::: dom/html/HTMLMediaElement.cpp:4058
(Diff revision 1)
> +    // will call Play() again if the tab is brought to the foreground, or the
> +    // audio tab indicator is clicked, which will resolve the promise if we end
> +    // up playing.
> +    LOG(LogLevel::Debug, ("%p Play() call delayed by AudioChannelAgent", this));
> +    MaybeDoLoad();
> -  mPendingPlayPromises.AppendElement(promise);
> +    mPendingPlayPromises.AppendElement(promise);

Do we need to use multiple play promises?

why not re-use an existing promise if one is pending?

::: dom/html/HTMLMediaElement.cpp:4104
(Diff revision 1)
> +    AutoplayPolicy::RequestFor(WrapNotNull(OwnerDoc()));
> +  if (!request) {
> +    AsyncRejectPendingPlayPromises(NS_ERROR_DOM_INVALID_STATE_ERR);
> +    return;
> +  }
> +  RefPtr<HTMLMediaElement> self(this);

nit: I prefer RefPtr<HTLMMediaElement> self = this 
as this is one it's mostly used style-wise in the dom/media code.

I see one lonely existing self(this) form in this file...

::: dom/html/HTMLMediaElement.cpp:4111
(Diff revision 1)
> +    ->Then(mAbstractMainThread,
> +           __func__,
> +           [ self, handlingUserInput = aHandlingUserInput, request ](
> +             bool aApproved) {
> +             self->mAutoplayPermissionRequest.Complete();
> +             MOZ_RELEASE_ASSERT(!self->mAutoplayPermissionRequest.Exists());

unecessary, as you've called Complete() just above.

With the promise semantics, the assert is redundant, you can't be there if the promise request holder has been disconnected.

And I don't see the point in asserting the line of code above worked as documented.

::: dom/html/HTMLMediaElement.cpp:4121
(Diff revision 1)
> +             self->PlayInternal(handlingUserInput);
> +             self->UpdateCustomPolicyAfterPlayed();
> +           },
> +           [self, request](nsresult aError) {
> +             self->mAutoplayPermissionRequest.Complete();
> +             MOZ_RELEASE_ASSERT(!self->mAutoplayPermissionRequest.Exists());

same as above

::: dom/html/HTMLMediaElement.cpp:7895
(Diff revision 1)
>  }
>  
>  void
>  HTMLMediaElement::AsyncRejectPendingPlayPromises(nsresult aError)
>  {
> +  mAutoplayPermissionRequest.DisconnectIfExists();

So we disconnect AsyncRejectPendingPlayPromises (it will already be disconnected except when called from Pause())

Does that mean the prompt stays displayed even though we no longer care for it?
Attachment #8988064 - Flags: review?(jyavenard) → review+
Comment on attachment 8988065 [details]
Bug 1463919 - Move ask autoplay permission check into AutoplayPolicy.

https://reviewboard.mozilla.org/r/253312/#review259938

::: dom/html/HTMLMediaElement.cpp:6237
(Diff revision 1)
>    if (!CanActivateAutoplay()) {
>      return;
>    }
>  
> +  if (!AutoplayPolicy::IsMediaElementAllowedToPlay(WrapNotNull(this))) {
> +    if (Preferences::GetBool("media.autoplay.ask-permission", false)) {

shouldn't the handling on this pref done within the Autoplay policy?

such that HTMLMediaElement doesn't need to know about specific exception to such autoplay policy.
Attachment #8988065 - Flags: review?(jyavenard) → review+
Comment on attachment 8988066 [details]
Bug 1463919 - Adjust mochitests to pass after changes.

https://reviewboard.mozilla.org/r/253314/#review259940

::: commit-message-eb6c2:17
(Diff revision 1)
> +permission request, this no longer effectively tests whether the cross origin
> +child can autoplay, as the cross origin child just uses the top level window's
> +origin for requests. So we can instead load the helper window cross orgin
> +instead, and remove one helper layer. Also an issue here is the way I was
> +waiting for a new window to load was racy, so now we wait for loading windows
> +to send us a "ready" message.

tldr.

can you break in paragraphs?
Attachment #8988066 - Flags: review?(jyavenard) → review+
Comment on attachment 8988069 [details]
Bug 1463919 - Tests for prompting for permission to autoplay.

https://reviewboard.mozilla.org/r/253310/#review259942

::: dom/html/HTMLMediaElement.cpp:4076
(Diff revision 1)
>    // ask for and are granted permission by the user.
>  
>    if (!Preferences::GetBool("media.autoplay.ask-permission", false)) {
>      LOG(LogLevel::Debug, ("%p play not allowed and prompting disabled.", this));
>      promise->MaybeReject(NS_ERROR_DOM_MEDIA_NOT_ALLOWED_ERR);
> +    if (Preferences::GetBool("media.autoplay.block-event.enabled", false)) {

I'd prefer a StaticPref access, makes it easier if there's any desire one day to change the pref name
Attachment #8988069 - Flags: review?(jyavenard) → review+
Comment on attachment 8988063 [details]
Bug 1463919 - Add AutoplayRequest to encapsulate asking for autoplay permission.

https://reviewboard.mozilla.org/r/253302/#review259944

::: dom/html/AutoplayRequest.h:18
(Diff revision 1)
> +
> +class nsPIDOMWindowInner;
> +class nsIEventTarget;
> +
> +namespace mozilla {
> +namespace dom {

I don't think this object should be in the DOM namespace.
Comment on attachment 8988064 [details]
Bug 1463919 - Have HTMLMediaElement ask for autoplay permission when playback otherwise blocked.

https://reviewboard.mozilla.org/r/253304/#review259930

> Do we need to use multiple play promises?
> 
> why not re-use an existing promise if one is pending?

The spec says to allocate a new one... https://html.spec.whatwg.org/multipage/media.html#playing-the-media-resource:media-element-28

> So we disconnect AsyncRejectPendingPlayPromises (it will already be disconnected except when called from Pause())
> 
> Does that mean the prompt stays displayed even though we no longer care for it?

Yes, Bug 1471485 will help with that somewhat... I think leaving the prompt showing is OK here, as if we hid it on promise reject, we could end up getting into a show/hide/show/hide loop on pages which were constantly playing/pausing/playing/pausing, which would be hard on the eyes.
Comment on attachment 8988065 [details]
Bug 1463919 - Move ask autoplay permission check into AutoplayPolicy.

https://reviewboard.mozilla.org/r/253312/#review260326

::: dom/html/HTMLMediaElement.cpp:6237
(Diff revision 1)
>    if (!CanActivateAutoplay()) {
>      return;
>    }
>  
> +  if (!AutoplayPolicy::IsMediaElementAllowedToPlay(WrapNotNull(this))) {
> +    if (Preferences::GetBool("media.autoplay.ask-permission", false)) {

We could have AutoplayPolicy::IsMediaElementAllowedToPlay return an enum, {Allowed,Blocked,AskPermission}, would that suffice?
Comment on attachment 8988065 [details]
Bug 1463919 - Move ask autoplay permission check into AutoplayPolicy.

https://reviewboard.mozilla.org/r/253312/#review260326

> We could have AutoplayPolicy::IsMediaElementAllowedToPlay return an enum, {Allowed,Blocked,AskPermission}, would that suffice?

yes,

such 3 states thing is something netflix mentioned about too
Comment on attachment 8988067 [details]
Bug 1463919 - Ensure resuming via audio tab indicator bypasses autoplay permission check.

https://reviewboard.mozilla.org/r/253306/#review260474

This looks solid to me. Thanks, cpearce!
Attachment #8988067 - Flags: review?(mconley) → review+
Comment on attachment 8988068 [details]
Bug 1463919 - Test that starting play from tab audio indicator overrides block autoplay.

https://reviewboard.mozilla.org/r/253316/#review260476

Thanks for the test!

::: toolkit/content/tests/browser/browser_block_autoplay_media.js:69
(Diff revision 1)
>    await ContentTask.spawn(tab2.linkedBrowser, SuspendedType.SUSPENDED_BLOCK,
>                            check_audio_suspended);
>  
> +
> +  // Test 4: Disable autoplay, enable asking for permission, and verify
> +  // that when a tab is opened in the background has had its playback

Nit: "when a tab is opened in the background has had its start delayed" -> "when a tab is opened in the background and has had its start delayed"
Attachment #8988068 - Flags: review?(mconley) → review+
Comment on attachment 8988067 [details]
Bug 1463919 - Ensure resuming via audio tab indicator bypasses autoplay permission check.

https://reviewboard.mozilla.org/r/253306/#review260478

::: toolkit/content/browser-content.js:532
(Diff revision 1)
>          break;
>        case "resumeMedia":
> +        // Override the autoplay blocking logic, so that resuming media
> +        // via the audio tab indicator will bypass the permission check
> +        // and allow media to autoplay.
> +        utils.allowAutoplayOverride = true;

Actually, out of curiosity, is this going to survive the lifetime of the document? Like... are we okay if this tab will be able to autoplay content going into the future even at different sites?
ni?ing cpearce for comment 19. Just making sure I understand what rights we're actually giving a tab when we allow it to play like this (is it intentional to be for the tab lifetime, or did we want this to be for the site lifetime?)
Flags: needinfo?(cpearce)
Comment on attachment 8988069 [details]
Bug 1463919 - Tests for prompting for permission to autoplay.

https://reviewboard.mozilla.org/r/253310/#review260480

Test looks good - thanks for adding it! Most of what I'm suggesting here is pretty minor.

::: commit-message-d191a:3
(Diff revision 1)
> +Test that video that autoplays via a play() call or via autoplay attribute
> +plays or is blocked or prompts with a doorhanger for approval and plays when
> +"allow" clicked and doesn't when "block" clicked.

Nit: This commit message has a lot of clauses and can probably be broken down into several sentences. I had to read it a few times to understand what it was saying.

::: toolkit/content/tests/browser/browser_autoplay_policy_request_permission.js:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Nit: I'm pretty sure our tests are supposed to be Public Domain licensed. At least, that was the last I heard about it.

::: toolkit/content/tests/browser/browser_autoplay_policy_request_permission.js:20
(Diff revision 1)
> +    ["media.autoplay.ask-permission", true],
> +    ["media.autoplay.block-event.enabled", true],
> +  ]});
> +});
> +
> +// Content script that creates an autoplay video.

Let's document what the args are expected to be here.

Also, perhaps to make it clearer that this is a content script, perhaps this function should take care of calling ContentTask.spawn, and then we call it like

```js
await loadAutoplayVideo({ browser, ... });
```

in your test helper. I think that's a more typical pattern in our browser mochitest testing code rather than separate outside functions that get passed to ContentTask.spawn.

::: toolkit/content/tests/browser/browser_autoplay_policy_request_permission.js:59
(Diff revision 1)
> +}
> +
> +// Content script that checks whether the video created by loadAutoplayVideo()
> +// started playing.
> +async function checkVideoDidPlay(args) {
> +  let video = content.document.getElementById("v1");

Similar to the above, this function should probably be responsible for calling ContentTask.spawn

::: toolkit/content/tests/browser/browser_autoplay_policy_request_permission.js:72
(Diff revision 1)
> +  let tab = await BrowserTestUtils.openNewForegroundTab(window.gBrowser,
> +                                                        "about:blank");
> +  tab.linkedBrowser.loadURI(VIDEO_PAGE);
> +  await BrowserTestUtils.browserLoaded(tab.linkedBrowser);

See my other comment on how you can simplify this with `BrowserTestUtils.withNewTab`.

::: toolkit/content/tests/browser/browser_autoplay_policy_request_permission.js:128
(Diff revision 1)
> +  let tab = await BrowserTestUtils.openNewForegroundTab(window.gBrowser,
> +                                                        "about:blank");

You can simplify this a bit, and take care of the removal of the tab all in one by using `BrowserTestUtils.withNewTab`, like so:

```js
await BrowserTestUtils.withNewTab({
  gBrowser,
  url: VIDEO_PAGE,
}, browser => {
  // At this point, VIDEO_PAGE is loaded in the
  // new tab, and you have access to its browser
  // element.
  
  // Rest of test goes in here.

});

// Tab gets automatically closed for you.
Attachment #8988069 - Flags: review?(mconley) → review+
Comment on attachment 8988063 [details]
Bug 1463919 - Add AutoplayRequest to encapsulate asking for autoplay permission.

https://reviewboard.mozilla.org/r/253302/#review260500

r- mainly because I feel like I'm missing something regarding user input handling.

::: dom/html/AutoplayRequest.cpp:40
(Diff revision 1)
> +}
> +
> +AutoplayRequest::~AutoplayRequest() {}
> +
> +already_AddRefed<AutoplayRequest>
> +AutoplayRequest::Create(nsPIDOMWindowInner* aWindow, bool aIsHandlingUserInput)

I'm clearly missing something here. aIsHandlingUserInput is always false.
Why do we then even need mIsHandlingUserInput member variable?

::: dom/html/AutoplayRequest.cpp:40
(Diff revision 1)
> +}
> +
> +AutoplayRequest::~AutoplayRequest() {}
> +
> +already_AddRefed<AutoplayRequest>
> +AutoplayRequest::Create(nsPIDOMWindowInner* aWindow, bool aIsHandlingUserInput)

Perhaps pass in nsGlobalWindowInner, since it has GetPrincipal() method. Then there isn't need to ever use GetDoc()

::: dom/media/AutoplayPolicy.cpp:78
(Diff revision 1)
>    return false;
>  }
>  
> +/* static */
> +already_AddRefed<AutoplayRequest>
> +AutoplayPolicy::RequestFor(NotNull<nsIDocument*> aDocument)

nsIDocument& aDocument as param? oh, other code in this file uses NonNull too.
I wonder why.
Attachment #8988063 - Flags: review?(bugs) → review-
(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos / reviews) from comment #20)
> ni?ing cpearce for comment 19. Just making sure I understand what rights
> we're actually giving a tab when we allow it to play like this (is it
> intentional to be for the tab lifetime, or did we want this to be for the
> site lifetime?)

The override should not survive the lifetime of the document. That is, if we load a new document into a tab, the override should not persist. So that means what I have here isn't going to work, thanks for pointing that out!

Maybe we need to store the override somewhere else, like on the inner window.
Flags: needinfo?(cpearce)
Comment on attachment 8988063 [details]
Bug 1463919 - Add AutoplayRequest to encapsulate asking for autoplay permission.

https://reviewboard.mozilla.org/r/253302/#review260500

> I'm clearly missing something here. aIsHandlingUserInput is always false.
> Why do we then even need mIsHandlingUserInput member variable?

You're right, mIsHandlingUserInput should always be false, if we'd had user input, we'd not need to prompt at all. I will remove this field and assume it's always false.

> nsIDocument& aDocument as param? oh, other code in this file uses NonNull too.
> I wonder why.

Sure, I was merely copying the precedent of using NotNull set by the previous author of this code.
Comment on attachment 8988064 [details]
Bug 1463919 - Have HTMLMediaElement ask for autoplay permission when playback otherwise blocked.

https://reviewboard.mozilla.org/r/253304/#review260576

::: dom/html/HTMLMediaElement.h:59
(Diff revision 1)
>  class MediaInputPort;
>  class MediaStream;
>  class MediaStreamGraph;
>  class VideoFrameContainer;
>  namespace dom {
> +class AutoplayRequest;

I've removed AutoplayRequest out dom namespace, it's just in mozilla namespace now.
Comment on attachment 8988069 [details]
Bug 1463919 - Tests for prompting for permission to autoplay.

https://reviewboard.mozilla.org/r/253310/#review260590

::: toolkit/content/tests/browser/browser_autoplay_policy_request_permission.js:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

You're correct! According to https://www.mozilla.org/en-US/MPL/license-policy/ 
"Trivial bits of Mozilla Code, such as testcases [...] should be put in the public domain"
I'll move the code to override the autoplay logic if the audio tab indicator is pressed into a new bug, so that I can land what we have here sooner since Mike will be out for holiday on Monday.
Comment on attachment 8988069 [details]
Bug 1463919 - Tests for prompting for permission to autoplay.

https://reviewboard.mozilla.org/r/253310/#review260480

> You can simplify this a bit, and take care of the removal of the tab all in one by using `BrowserTestUtils.withNewTab`, like so:
> 
> ```js
> await BrowserTestUtils.withNewTab({
>   gBrowser,
>   url: VIDEO_PAGE,
> }, browser => {
>   // At this point, VIDEO_PAGE is loaded in the
>   // new tab, and you have access to its browser
>   // element.
>   
>   // Rest of test goes in here.
> 
> });
> 
> // Tab gets automatically closed for you.

Thanks, that makes it a lot nicer!
Comment on attachment 8988067 [details]
Bug 1463919 - Ensure resuming via audio tab indicator bypasses autoplay permission check.

https://reviewboard.mozilla.org/r/253306/#review260478

> Actually, out of curiosity, is this going to survive the lifetime of the document? Like... are we okay if this tab will be able to autoplay content going into the future even at different sites?

Will continue working on this patch in Bug 1472580 in order to land the rest here independently ASAP.
Attachment #8988067 - Attachment is obsolete: true
Attachment #8988068 - Attachment is obsolete: true
Assignee: nobody → cpearce
Comment on attachment 8988065 [details]
Bug 1463919 - Move ask autoplay permission check into AutoplayPolicy.

Re-requesting review on "Move ask autoplay permission check into AutoplayPolicy", as I think it's changed enough since r+ to warrant re-review, and MozReview won't let me me re-request for some reason.
Attachment #8988065 - Flags: review+ → review?(jyavenard)
Comment on attachment 8988063 [details]
Bug 1463919 - Add AutoplayRequest to encapsulate asking for autoplay permission.

https://reviewboard.mozilla.org/r/253302/#review260956

::: dom/media/AutoplayPolicy.cpp:101
(Diff revision 2)
>    }
>  
> +  // TODO : this old way would be removed when user-gestures-needed becomes
> +  // as a default option to block autoplay.
>    if (!Preferences::GetBool("media.autoplay.enabled.user-gestures-needed", false)) {
> +    // If elelement is blessed, it would always be allowed to play().

element
Attachment #8988063 - Flags: review?(bugs) → review+
Comment on attachment 8988065 [details]
Bug 1463919 - Move ask autoplay permission check into AutoplayPolicy.

https://reviewboard.mozilla.org/r/253312/#review261098

Much more readable that way...

::: dom/media/AutoplayPolicy.h:23
(Diff revision 2)
>  namespace dom {
>  
>  class HTMLMediaElement;
>  class AudioContext;
>  
> +enum class Authorization {

{ on new line...
Attachment #8988065 - Flags: review?(jyavenard) → review+
Comment on attachment 8989095 [details]
Bug 1463919 - Check AutoplayPolicy before activating attribute based autoplay.

https://reviewboard.mozilla.org/r/254156/#review261100

::: dom/html/HTMLMediaElement.cpp:6234
(Diff revision 1)
>    if (!CanActivateAutoplay()) {
>      return;
>    }
>  
> +  switch (AutoplayPolicy::IsAllowedToPlay(*this)) {
> +    case Authorization::Blocked: {

{ } block isn't necessary here

::: dom/html/HTMLMediaElement.cpp:6238
(Diff revision 1)
> +  switch (AutoplayPolicy::IsAllowedToPlay(*this)) {
> +    case Authorization::Blocked: {
> +      return;
> +    }
> +    case Authorization::Prompt: {
> +      EnsureAutoplayRequested(false);

same
Attachment #8989095 - Flags: review?(jyavenard) → review+
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fc42cfddb48c
Add AutoplayRequest to encapsulate asking for autoplay permission. r=smaug
https://hg.mozilla.org/integration/autoland/rev/75087df2875a
Have HTMLMediaElement ask for autoplay permission when playback otherwise blocked. r=jya
https://hg.mozilla.org/integration/autoland/rev/a03d5236779d
Move ask autoplay permission check into AutoplayPolicy. r=jya
https://hg.mozilla.org/integration/autoland/rev/1810cad130d4
Check AutoplayPolicy before activating attribute based autoplay. r=jya
https://hg.mozilla.org/integration/autoland/rev/11017d3044e2
Adjust mochitests to pass after changes. r=jya
https://hg.mozilla.org/integration/autoland/rev/e780c6b275a2
Tests for prompting for permission to autoplay. r=jya,mconley
Depends on: 1485189
You need to log in before you can comment on or make changes to this bug.