Closed Bug 1466926 Opened 6 years ago Closed 6 years ago

allow extension background script to autoplay by the pref "media.autoplay.allow-extension-backgroundscripts"

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
60 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: pag77, Assigned: alwu)

References

Details

Attachments

(3 files)

Bug 1466926 - part1 : always allow extension background script to autoplay.
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1466926 - part3 : add test.
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1466926 - part2 : add a new preference to control autoplay from background script.
59 bytes, text/x-review-board-request
kmag
: review+
Details 
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180516032328

Steps to reproduce:

about:config
media.autoplay.enabled=false

WebExtensions->background.js

var test_audio = new Audio();
test_audio.preload = 'none';
test_audio.src = 'ANY AUDIO LINK';
test_audio.play();



Actual results:

test_audio.play() - not worked


Expected results:

test_audio.play() - should work in background-javascripts regardless of the settings in about:config?
Setting a component to involve the development team in reviewing this issue.
See also Bug 1412772.
Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
Oleksandr: Sorry, I don't have any experience with or knowledge of WebExtensions. Are you saying that a WebExtension cannot play audio using the HTMLAudioElement if autoplay is disabled?
Blocks: block-autoplay
Flags: needinfo?(pag77)
Rank: 15
Priority: -- → P2
(In reply to Chris Pearce (:cpearce) from comment #2)
> WebExtension cannot play audio using the HTMLAudioElement if autoplay is disabled?

yes
Flags: needinfo?(pag77)
Talking to the team on #webextensions, it seems HTMLMediaElement can determine whether it's in a WebExtension by checking whether the principal of the HTMLMediaElement's owner document has an AddonPolicy.

Looks like there's precedent for that here:
https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/dom/base/nsDocument.cpp#2875
(In reply to Chris Pearce (:cpearce) from comment #4)
> Looks like there's precedent for that here:
> https://searchfox.org/mozilla-central/rev/
> 93d2b9860b3d341258c7c5dcd4e278dea544432b/dom/base/nsDocument.cpp#2875

If this you wrote for me, then I do not understand what you're talking about :(
For the add-on need some special permission in manifest.json?
(In reply to Oleksandr from comment #5)
> (In reply to Chris Pearce (:cpearce) from comment #4)
> > Looks like there's precedent for that here:
> > https://searchfox.org/mozilla-central/rev/
> > 93d2b9860b3d341258c7c5dcd4e278dea544432b/dom/base/nsDocument.cpp#2875
> 
> If this you wrote for me, then I do not understand what you're talking about
> :(
> For the add-on need some special permission in manifest.json?

This was a pointer to the person who ends up fixing this bug in our code. We'll see if we can get this one fixed soon.
Assignee: nobody → alwu
After discussed with aswan on #webextensions, we should prevent the audio in background page (which is the page background script is running on) from starting playing directly. Maybe use another way to make people aware that, like the doorhanger we did now.

Because people won't directly interact with the background page, if the sound suddenly starts, it would be annoying.

I would temporary move this bug to webextension component, to see what are they thinking about playing sound from background page.
Status: UNCONFIRMED → NEW
Component: Audio/Video: Playback → General
Ever confirmed: true
Priority: P2 → --
Product: Core → WebExtensions
I thought we already had a bug for this but I can't find one.

Markus, where should this start for UX design?  For starters:
 - how would we give users a chance to allow/disallow playing audio from a background page
 - how would we indicate where audio is coming from when an extension is playing audio,
 - how could a user mute/unmute audio coming from a particular extension
Assignee: alwu → nobody
Flags: needinfo?(mjaritz)
We're trying to ship block autoplay in 63. So I think we should just allow web extensions to autoplay for now, and circle back on prompting if it makes sense for extensions.
According to the discussion on #extensions, we would let background audio play for now, and have the web extensions UX person figure out what the best solution here long term is.
Assignee: nobody → alwu
Component: General → Audio/Video: Playback
Product: WebExtensions → Core
According to the comment between 2 and 3, add the priority back.
Priority: -- → P2
Comment on attachment 8991123 [details]
Bug 1466926 - part1 : always allow extension background script to autoplay.

https://reviewboard.mozilla.org/r/256078/#review263006

Can't really review this. As far as I know, it isn't documented anywhere what kinds of principals are used in web extensions and that code isn't really DOM stuff, so hard to say whether this is right. kmag or someone might know.

::: dom/base/nsDocument.cpp:12475
(Diff revision 1)
>  }
>  
> +bool
> +nsIDocument::IsDocumentFromWebExtension() const
> +{
> +  return !!(BasePrincipal::Cast(NodePrincipal())->AddonPolicy());

I don't know the principal setup web extensions use. Last time I asked, it wasn't quite as simple as this, but one would need to look at all the principals expanded principal may have.
Attachment #8991123 - Flags: review?(bugs)
OK, I'll change this review request to extension's folk, thank you!
Flags: needinfo?(bugs)
Attachment #8991123 - Flags: review?(aswan)
Comment on attachment 8991123 [details]
Bug 1466926 - part1 : always allow extension background script to autoplay.

https://reviewboard.mozilla.org/r/256078/#review263008

::: dom/base/nsIDocument.h:3392
(Diff revision 1)
>    // document in the document tree.
>    bool HasBeenUserGestureActivated();
>  
> +  // This document is running on the background page which is used specifically
> +  // for the extension.
> +  bool IsDocumentFromWebExtension() const;

This function is being added to nsIDocument, so I think putting "Document" in the name is redundant.

So I recommend you call this "InWebExtensionBackgroundScript()", assuming that our principal only has an AddonPolicy in the background script case.
Comment on attachment 8991123 [details]
Bug 1466926 - part1 : always allow extension background script to autoplay.

redirect to kris
Flags: needinfo?(mjaritz)
Attachment #8991123 - Flags: review?(aswan) → review?(kmaglione+bmo)
Attachment #8991124 - Flags: review?(aswan) → review?(kmaglione+bmo)
Hi, Kris,
Could you help me review my patches? Thank you!
Flags: needinfo?(kmaglione+bmo)
Hm. My understanding is that this is not something we want.

If media is playing in an extension background page, there's no way for a user to know where it's coming from, or to stop it, unless every extension does the proper UI work to handle it. I don't think we can make the assumption that they will.

If we want to allow background pages to play audio, I think we need the right front-end UI elements to surface and control it first.
Flags: needinfo?(kmaglione+bmo) → needinfo?(mconca)
Hi, Kris,

Acutually, both Firefox and Chrome are allowing background script to autoplay in current design. We didn't change current behavior, the purpose of this bug is that we don't want block-autoplay to break the ability that is allowing background script to autoplay.

Thank you!
Flags: needinfo?(kmaglione+bmo)
Kris, what we're trying to do here is allow extensions' background scripts to play audio for the time being, while we figure out the best way to surface UI to allow users to control it. So we're ensuring that we don't break background scripts before we've had a chance to solve their use cases properly.
(In reply to Chris Pearce (:cpearce) from comment #25)
> Kris, what we're trying to do here is allow extensions' background scripts
> to play audio for the time being, while we figure out the best way to
> surface UI to allow users to control it. So we're ensuring that we don't
> break background scripts before we've had a chance to solve their use cases
> properly.

I understand that, but this is something that was never intended to work.

In any case, it's a decision for Mike, at this point. I'm happy to finish reviewing the patch if he signs off.
Flags: needinfo?(kmaglione+bmo)
Extensions that play audio via a background script, even via a user input handler like a browser action button, break if the autoplay pref is set to false. This can be verified with https://addons.mozilla.org/en-US/firefox/addon/worldwide-radio/. We can't let that happen as a default in 63, so I agree that maintaining the status quo makes sense here. Background scripts should be allowed to play audio, even when the autoplay pref is set to false (or set to ask).

That said, it would be nice to introduce a separate pref that applies to background scripts, just in case users want or expect audio to be disabled (e.g. media.autoplay.backgroundscripts.enabled). It should default to true in order to preserve the status quo.

Longer-term, Ddurst and I will be talking to UX to figure out the best way to handle this (hopefully in time for 64).
Flags: needinfo?(mconca)
(In reply to Mike Conca [:mconca] (Denver, CO, USA UTC-6) from comment #27)
> I agree that maintaining the status
> quo makes sense here. Background scripts should be allowed to play audio,
> even when the autoplay pref is set to false (or set to ask).

Thanks Mike.
 
> That said, it would be nice to introduce a separate pref that applies to
> background scripts, just in case users want or expect audio to be disabled

We can totally add a pref to disable autoplay in background scripts. I'd suggest media.autoplay.allow-backgroundscripts to match the convention used by other media.autoplay.allow-* prefs.
Hmmm... Firefox Nightly 63.0a1 (2018-07-25)
All settings in about:config by default

in background.js:
==========
var play_promise = audio.play();
play_promise.then(function() {
//
}).catch(function(error) {
// !!!! NotAllowedError: The play method is not allowed by the user agent or the platform in the current context, possibly because the user denied permission.
});
==========

this "NotAllowedError" - is somehow related to your work?
See Also: → 1478824
Hi, Kris,
Mike has signed this bug, could you help me review my patches?
Thank you!
Flags: needinfo?(kmaglione+bmo)
I don't want it to be lost in the previous comments:

1) background scripts are currently *not* working as they were -- even when there is UI. So that should be fixed by this patch.
2) This patch should be modified to include the new pref to be used for that fix (per #c27 and #c28).
Summary: media.autoplay.enabled=false ; WebExtensions->background.js->Audio.play() not worked → allow extension background script to autoplay by the pref "media.autoplay.allow-extension-backgroundscripts"
Comment on attachment 8991123 [details]
Bug 1466926 - part1 : always allow extension background script to autoplay.

https://reviewboard.mozilla.org/r/256078/#review267652

::: dom/base/nsIDocument.h:3441
(Diff revision 2)
>    // Return true if NotifyUserGestureActivation() has been called on any
>    // document in the document tree.
>    bool HasBeenUserGestureActivated();
>  
> +  // This document is running on the background page which is used specifically
> +  // for the extension.

I think the comment would be clearer as:

"This document is in a WebExtension background script."
Attachment #8991123 - Flags: review?(cpearce) → review+
Comment on attachment 8996364 [details]
Bug 1466926 - part2 : add a new preference to control autoplay from background script.

https://reviewboard.mozilla.org/r/260490/#review267884

::: dom/base/nsDocument.cpp:12616
(Diff revision 1)
>  }
>  
>  bool
>  nsIDocument::InWebExtensionBackgroundScript() const
>  {
> -  return !!(BasePrincipal::Cast(NodePrincipal())->AddonPolicy());
> +  return Preferences::GetBool("media.autoplay.allow-extension-backgroundscripts") &&

You should pass a default value arg of `true` for this.

Also, I'd prefer "media.autoplay.allow-extension-background-pages" or (ideally) "media.autoplay.extensions.background-pages.allowed", since we might want prefs for other types of extension context in the future.
Attachment #8996364 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8991123 [details]
Bug 1466926 - part1 : always allow extension background script to autoplay.

https://reviewboard.mozilla.org/r/256078/#review267886

::: dom/base/nsDocument.cpp:12614
(Diff revision 2)
> +nsIDocument::InWebExtensionBackgroundScript() const
> +{
> +  return !!(BasePrincipal::Cast(NodePrincipal())->AddonPolicy());

This isn't accurate. This will return true for any extension page, whether it's a background page, a popup, a visible tab, a visible iframe, ...

I'd name it something like "IsExtensionPage()" instead.

Also, please don't use `!!` in a `bool` context. A raw pointer will automatically be coerced to a bool here. The `!!` and unnecessary parens just add unnecessary visual noise.
Comment on attachment 8991124 [details]
Bug 1466926 - part3 : add test.

https://reviewboard.mozilla.org/r/256104/#review267888

::: commit-message-63f2f:1
(Diff revision 3)
> +Bug 1466926 - part3 : add test.

The audio.ogg is pretty big (14K). Can you replace it with an audio file that's as close to empty as possible?

Here's a short, silent ogg file that should do: https://send.firefox.com/download/df6fe26772/#SIPpShjD0PzDNDPeJnRc3A

It also has the benefit of not playing audible audio when people are running tests.

::: browser/components/extensions/test/browser/browser_ext_autoplayInBackground.js:16
(Diff revision 3)
> +
> +async function testAutoplayInBackgroundScript(enableScript) {
> +  info(`- setup test preference, enableScript=${enableScript} -`);
> +  await setup_test_preference(enableScript);
> +
> +  info("- install extension -");

Not necessary

::: browser/components/extensions/test/browser/browser_ext_autoplayInBackground.js:42
(Diff revision 3)
> +  } else {
> +    await extension.awaitMessage("play-failed");
> +    ok(true, "play promise was rejected!");
> +  }
> +
> +  info("- unstall extension -");

Not necessary
Attachment #8991124 - Flags: review?(kmaglione+bmo) → review+
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8e1182e32a9f
part1 : always allow extension background script to autoplay. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/2c49a0d07941
part2 : add a new preference to control autoplay from background script. r=kmag
https://hg.mozilla.org/integration/autoland/rev/c26db39e0c0a
part3 : add test. r=kmag
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: