1476555 - No notification to the user if autoplay is configured to permanently block

Status: VERIFIED FIXED

(Firefox :: Site Identity, enhancement, P1)

Description

[Dale Harvey (:daleharvey)]

Configure autoplay to Block Always then visit https://html5zombo.com/, there is no indication to the user that we have blocked it from autoplaying

Comment 1

[Dale Harvey (:daleharvey)]

Theres a similiar bug open @ https://bugzilla.mozilla.org/show_bug.cgi?id=1471723 for all icons, Ill give a shot at fixing it for all blocked permissions but leave this one in case we only fix it for autoplay

Comment 2

[Dale Harvey (:daleharvey)]

So one issue here is that as Chris predicted, when autoplay is configured to block we do that check in c++ and never defer to SitePermissions so the frontend has no way to even know of a request was blocked. 

It seems like the choices would be to remove the BLOCKED checked in c++ and have it call PROMPT everytime, having SitePermissions check whether its blocked or not and give that back, or for c++ to send an event down when something it blocked.

Johann I am not certain on how the rest of the permissions (geolocation etc) work when globally blocked and would be good to have this work for everything, got any pointers on how you think it should be done?

Comment 3

[Johann Hofmann [:johannh]]

(In reply to Dale Harvey (:daleharvey) from comment #2)
> So one issue here is that as Chris predicted, when autoplay is configured to
> block we do that check in c++ and never defer to SitePermissions so the
> frontend has no way to even know of a request was blocked. 
> 
> It seems like the choices would be to remove the BLOCKED checked in c++ and
> have it call PROMPT everytime, having SitePermissions check whether its
> blocked or not and give that back, or for c++ to send an event down when
> something it blocked.

Yes, that's basically what I thought, and I think it works best when the C++ just doesn't check the permission itself.

> Johann I am not certain on how the rest of the permissions (geolocation etc)
> work when globally blocked and would be good to have this work for
> everything, got any pointers on how you think it should be done?

I think you'd already do this endeavor a great service if you implemented this for one specific use case (autoplay) and from your patch we can figure out how to generalize it to the other permissions (they all have some sort of code that checks for blocked in C++ so I think it mostly requires consolidation in the front-end anyway).

Thanks!

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

It sounds like the fix here is to make the C++ function AutoplayPolicy::IsAllowedToPlay() return a bool. On true, assume we can play, otherwise, assume we need to ask.

There's work to spec a synchronous Web API HTMLMediaElement.allowedToPlay [1] for which Gecko would need to be able to check synchronously whether we've been granted permission to autoplay. The current form of that API should behave as if requests for permission would be denied, so Gecko doesn't need to distinguish between the ask and block cases, but may still need to poll for an ALLOW permission.

Having C++ always ask permission to autoplay means that the front end code would need to handle honouring the media.autoplay.default pref. That is, Gecko would dispatch a "can origin X play?" request to the front end, the front end would lookup the permission for origin X, if it's UNKNOWN, it would check media.autoplay.default, and if that's BLOCK show the control centre block icon and send back a block response to Gecko.

To me it seems that all would require us to add a hook to PermissionPromptForRequestPrototype which PermissionPromptPrototype.prompt() calls if it encounters and UNKNOWN permission to give the front end a chance to abort asking for permission with a default value?

[1] https://github.com/whatwg/html/issues/3617#issuecomment-401968833

Comment 5

[Dale Harvey (:daleharvey)]

The frontend already checks the pref in SitePermissions, however I used the wrong constants, fixing it with 

-      if (state == Ci.nsIAutoplay.ALLOW) {
+      if (state == Ci.nsIAutoplay.ALLOWED) {
         return SitePermissions.ALLOW;
-      } if (state == Ci.nsIAutoplay.BLOCK) {
-        return SitePermissions.DENY;
+      } if (state == Ci.nsIAutoplay.BLOCKED) {
+        return SitePermissions.BLOCK;

and having c++ ignore the blocked case means we ask the frontend correctly and it behaves appropriately, now I think its just a matter of adding the persistent permission so it can be picked up by SitePermissions.getAll and then it should all work I believe

Comment 6

[Dale Harvey (:daleharvey)]

Attachment: Bug 1476555 - Show notification when autoplay blocked globally.

Review commit: https://reviewboard.mozilla.org/r/259010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259010/

Comment 7

[Dale Harvey (:daleharvey)]

Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259010/diff/1-2/

Comment 8

[Dale Harvey (:daleharvey)]

So this is a 'working' patch, will show the user a notification when autoplay is globally blocked. Just trying to think of the cleanest way to store that in the frontend as it doesnt fit into the current code very cleanly

I think its likely best to create a new object similiar to |TemporaryBlockedPermissions| that informs the UI about any global blocks when they have been accessed, but doesnt effect the actual fetching of permissions

Comment 9

[Dale Harvey (:daleharvey)]

Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259010/diff/2-3/

Comment 10

[Dale Harvey (:daleharvey)]

Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

Hey Florian

I just wanted to check that I am going in the right direction (on the JS side, Chris can check the c++) as I am writing the tests for this, patch looks to be working well for me although I would like to make sure there arent cases where we could be leaving listeners that we do not want, I think we will probably want to limit it to the autoplay-media permission at first

Cheers

Comment 11

[Code Review Bot [:reviewbot]]

Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

https://reviewboard.mozilla.org/r/259010/#review266984


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


::: browser/modules/SitePermissions.jsm:481
(Diff revision 3)
> -    if (state == this.UNKNOWN || state == this.getDefault(permissionID)) {
> +
> +    let defaultPermission = this.getDefault(permissionID);
> +
> +    if (scope == this.SCOPE_POLICY) {
> +      if (state == this.BLOCK) {
> +        dump('!!! SETTING GLOBALLY BLOCKED PERMISSION\n');

Error: Strings must use doublequote. [eslint: quotes]

::: browser/modules/SitePermissions.jsm:484
(Diff revision 3)
> +    if (scope == this.SCOPE_POLICY) {
> +      if (state == this.BLOCK) {
> +        dump('!!! SETTING GLOBALLY BLOCKED PERMISSION\n');
> +        GloballyBlockedPermissions.set(browser, permissionID);
> +      } else {
> +        dump('!!! REMOVING GLOBALLY BLOCKED PERMISSION\n');

Error: Strings must use doublequote. [eslint: quotes]

Comment 12

[Dale Harvey (:daleharvey)]

Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259010/diff/3-4/

Comment 13

[:Gijs (he/him)]

(In reply to Dale Harvey (:daleharvey) from comment #5)
> The frontend already checks the pref in SitePermissions, however I used the
> wrong constants, fixing it with 
> 
> -      if (state == Ci.nsIAutoplay.ALLOW) {
> +      if (state == Ci.nsIAutoplay.ALLOWED) {
>          return SitePermissions.ALLOW;
> -      } if (state == Ci.nsIAutoplay.BLOCK) {
> -        return SitePermissions.DENY;
> +      } if (state == Ci.nsIAutoplay.BLOCKED) {
> +        return SitePermissions.BLOCK;
> 
> and having c++ ignore the blocked case means we ask the frontend correctly
> and it behaves appropriately, now I think its just a matter of adding the
> persistent permission so it can be picked up by SitePermissions.getAll and
> then it should all work I believe

Can you file a follow-up bug to write tests so that this code is actually covered by tests, so we notice if we write bogus stuff here? Thank you! :-)

Comment 14

[Florian Quèze [:florian]]

(In reply to Dale Harvey (:daleharvey) from comment #10)

> I just wanted to check that I am going in the right direction (on the JS
> side, Chris can check the c++) as I am writing the tests for this, patch
> looks to be working well for me although I would like to make sure there
> arent cases where we could be leaving listeners that we do not want, I think
> we will probably want to limit it to the autoplay-media permission at first

I looked at the patch and didn't see any obvious problem, but I haven't worked on this code much in the last year, so I would be more comfortable if Johann checks the direction this is taking once he returns.

Comment 15

[Dale Harvey (:daleharvey)]

> Can you file a follow-up bug to write tests so that this code 
> is actually covered by tests, so we notice if we write bogus 
> stuff here? Thank you! :-)

Yup definitely worth making sure to not make the same mistake. This patch 
will add tests to make sure that code is tested for autoplay and Marks bug 
looks good for making sure it doesnt happen again

Comment 16

[Dale Harvey (:daleharvey)]

Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259010/diff/4-5/

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: WIP Patch: Add icon appears without clicking the identity panel

It looks like you're not actually testing that the block icon appears in the URL bar when we're set to block by default, only that it's there when you click on the identity box?

I think you can test that the icon appears in the URL bar with this patch. Note it's hacky in that I added a setTimeout() in order to account for the delay it takes for the icon to appear; that needs to be fixed. But this is the general approach you could take for testing whether the icon shows.

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

https://reviewboard.mozilla.org/r/259010/#review268154

Almost there on the C++ side, just some unnecessary code to remove. It would be good if you could add a test for the block icon appearing when we block too.

::: dom/media/AutoplayPolicy.cpp:140
(Diff revision 5)
>  
>    if (IsMediaElementAllowedToPlay(aElement)) {
> -    return nsIAutoplay::ALLOWED;
> +    return true;
>    }
>  
> -  return autoplayDefault;
> +  // Muted content

You'll need to rebase on top of Bug 1480281, which changes this code.

Note IsMediaElementAllowedToPlay() checks everything here except autoplayDefault. So you don't need to re-add the checks for volume/muted/IsWindowAllowedToPlay().

Comment 19

[Dale Harvey (:daleharvey)]

Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259010/diff/5-6/

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

https://reviewboard.mozilla.org/r/259010/#review268486

C++ looks fine.

::: dom/media/AutoplayPolicy.cpp:169
(Diff revision 6)
>      // If element is blessed, it would always be allowed to play().
>      return (autoplayDefault == nsIAutoplay::ALLOWED ||
>              aElement.IsBlessed() ||
> -            EventStateManager::IsHandlingUserInput())
> -              ? nsIAutoplay::ALLOWED : nsIAutoplay::BLOCKED;
> +            EventStateManager::IsHandlingUserInput());
> +  }
> +

Extra blank line here.

Comment 21

[Johann Hofmann [:johannh]]

Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

https://reviewboard.mozilla.org/r/259010/#review268704

So, my original idea for this was that, instead of displaying the blocked icon, we could show a minimized doorhanger (just the doorhanger icon) that the user could maximize and approve the permission whenever they liked. However, I don't mind if you prefer this variant (showing a crossed-out icon) and maybe it's a little more consistent.

Anyway, this patch is definitely a feedback+, just that I'd like to avoid conflating two concepts into SCOPE_POLICY.

Thanks!

::: browser/modules/PermissionUI.jsm:288
(Diff revision 6)
> +        // If the request is blocked by a global setting then we record
> +        // a flag that lasts for the duration of the current page load
> +        // to notify the user that the permission has been blocked.
> +        // Currently only applies to autoplay-media
> +        if (state == SitePermissions.getDefault(this.permissionKey) &&
> +            this.permissionKey === "autoplay-media") {

How about introducing a flag like this.showGloballyBlocked instead of doing this comparison here?

::: browser/modules/SitePermissions.jsm:136
(Diff revision 6)
>  
> +// This hold a flag per browser to indicate whether we should show the
> +// user a notification as a permission has been requested that has been
> +// blocked globally. We only want to notify the user in the case that
> +// they actually requested the permission within the current page load
> +// so will clear the flag on navigation

typo: page load so will clear the flag

(also missing a dot at the end)

::: browser/modules/SitePermissions.jsm:153
(Diff revision 6)
> +      entry[prePath] = {};
> +    }
> +
> +    entry[prePath][id] = true;
> +
> +    // Listen to any top level navigations, once we so one, clear the flag

typo: once we so one?

::: browser/modules/SitePermissions.jsm:477
(Diff revision 6)
>     *        The browser object to set temporary permissions on.
>     *        This needs to be provided if the scope is SCOPE_TEMPORARY!
>     */
>    set(uri, permissionID, state, scope = this.SCOPE_PERSISTENT, browser = null) {
> +
> +    if (scope == this.SCOPE_POLICY && state == this.BLOCK) {

I know using SCOPE_POLICY is convenient because it already handles the UI correctly for the identity popup, but I think it's a bad idea to conflate the two concepts. We should come up with a new scope instead, something like SCOPE_GLOBAL?

It's a little bit more work by updating the places where we use SCOPE_POLICY to also accept GLOBAL but I think it's worth it.

::: browser/modules/SitePermissions.jsm:480
(Diff revision 6)
>    set(uri, permissionID, state, scope = this.SCOPE_PERSISTENT, browser = null) {
> +
> +    if (scope == this.SCOPE_POLICY && state == this.BLOCK) {
> +      GloballyBlockedPermissions.set(browser, permissionID);
> +      browser.dispatchEvent(new browser.ownerGlobal
> +                            .CustomEvent("PermissionStateChange"));

nit: This indentation looks weird to me. Can you either give it an indentation of 2 spaces or make it align with the dot of .ownerGlobal?

I'm actually not even sure you need to put this on a new line :)

Comment 22

[Dale Harvey (:daleharvey)]

Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259010/diff/6-7/

Comment 23

[Dale Harvey (:daleharvey)]

Yeh I was considering a new flag but I wasnt certain that it wasnt what SCOPE_POLICY is actually made for, awesome thanks made those changes

Comment 24

[Dale Harvey (:daleharvey)]

Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259010/diff/7-8/

Comment 25

[Johann Hofmann [:johannh]]

Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

https://reviewboard.mozilla.org/r/259010/#review269410

r=me, see comments :)

Thanks for this and sorry for the delay!

::: browser/base/content/test/permissions/browser_autoplay_blocked.js:23
(Diff revision 8)
> +
> +function autoplayBlockedIcon() {
> +  return document.querySelector("#blocked-permissions-container " +
> +                                ".blocked-permission-icon.autoplay-media-icon");
> +}
> +add_task(async function testMainViewVisible() {

super tiny nit: can you add blank space above the add_task?

::: browser/modules/SitePermissions.jsm:155
(Diff revision 8)
> +
> +    entry[prePath][id] = true;
> +
> +    // Listen to any top level navigations, once we see one clear the flag
> +    // and remove the listener.
> +    browser.addProgressListener({

This is fine by me, but I just wanted to throw in what I would have done here:

Call a function like SitePermissions.clearGlobalPermissionForBrowser(browser) in https://searchfox.org/mozilla-central/rev/ca869724246f4230b272ed1c8b9944596e80d920/browser/base/content/browser.js#4677

Anyway, just a thought, not sure if it's worth a follow-up or whether we should just forget about it :)

::: browser/modules/SitePermissions.jsm:189
(Diff revision 8)
> +      let timeStamps = entry[prePath];
> +      for (let id of Object.keys(timeStamps)) {
> +        permissions.push({
> +          id,
> +          state: SitePermissions.BLOCK,
> +          scope: SitePermissions.SCOPE_POLICY

I think this is supposed to be SCOPE_GLOBAL now, right?

Comment 26

[Dale Harvey (:daleharvey)]

Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259010/diff/8-9/

Comment 27

[Dale Harvey (:daleharvey)]

Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259010/diff/9-10/

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1cee6e0d07e7
Show notification when autoplay blocked globally. r=cpearce,johannh

Comment 29

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/1cee6e0d07e7

Comment 30

[Gyula Palko[:gpalko]]

Verified, that the issue is no longer reproducible on Nightly 63.0a1(20180817100105).