Closed Bug 1477273 Opened 6 years ago Closed 6 years ago

Block autoplay doorhanger is displayed multiple times when temporary permission is granted to site

Categories

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

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

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- verified

People

(Reporter: gpalko, Assigned: daleharvey)

References

Details

Attachments

(2 files)

Bug 1477273 - Allow autoplay-media permission to be temporarily allowed.
59 bytes, text/x-review-board-request
johannh
: review+
Details
0001-Bug-1477273-Followup-to-ensure-correct-pref-in-test.patch
1.22 KB, patch
Details | Diff | Splinter Review 
[Environment:]
Windows 10, Mac OSX 10.13
Nightly 63.0a1 BuildId 20180720101508

[Steps:]
1. Open https://edition.cnn.com/videos
2. Uncheck the "Remember this decision" checkbox
3. Click Allow Autoplay

[Actual Result:]
 The featured video starts playing on the page
 The Block Autoplay doorhanger is displayed again 

[Expected Result:]
 The doorhanger should be displayed once

[Note:] 
- the number of doorhanger appearances depends on how fast you perform steps 2-3
Rank: 19
Priority: -- → P2
There are two problems here.

1. (Gecko/C++) We cancel the permission request in the AutoplayPermissionRequest destructor [1], and if we get a genuine cancel from the doorhanger. The Request reports the cancel to the AutoplayPermissionManager, but we reuse the same manager across different requests. So if a second request for permission comes in, we create a new AutoplayPermissionRequest and fire that off to content, but the first could be destroyed after the second is dispatched, and the cancel in the first's destructor could be reported to the manager as the second's result. We should clear the AutoplayPermissionRequest's reference to the Manager in Approve() and Cancel() so that we can't mixup the responses like this.

2. (Chrome/JS) PermissionPromptPrototype.prompt() [2] only stores one time permissions for Allow clicks [3] but stores temporary Block permissions for block actions [4]. We really want autoplay-media permissions for the top level document to apply to all subdocuments with a temporary permission. The security implications for allowing media playback are a lot less serious than the other things we require permissions for, and we're careful to only request permission on the top level document. So it seems we should be changing the permission scope to temporary for allows for "autoplay-media"?

johann: What do you think about 2 above? Can we add something to PermissionPromptPrototype to allow us to specify a temporary scope for permission request allow actions? Also, who can we get to review PermissionUI while you're out on PTO? Thanks!


[1] https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/dom/html/AutoplayPermissionRequest.cpp#37
[2] https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/browser/modules/PermissionUI.jsm#266
[3] https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/browser/modules/PermissionUI.jsm#341
[4] https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/browser/modules/PermissionUI.jsm#335
Flags: needinfo?(jhofmann)
Yeah, as you mentioned, unlike temporarily blocked, there's no inherent concept of temporary allow in our permission front-end so far. The temporary block mechanism was designed to cover "tab-range" (even through non-user navigations to different sites) and ignore frames, which is usually not a good idea for the "allow" case. From a quick glance I agree that this could be harmless enough to do for autoplay, though. This would need a little more detailed assessment, obviously.

I've asked :florian to help you out while I'm on PTO, feel free to direct questions or reviews to him :)
Flags: needinfo?(jhofmann)
I'm fixing the Gecko/C++ issue in bug 1477881.
Will take this one for the frontend fix
Assignee: nobody → dharvey
Blocks: block-autoplay-frontend
Btw sorry for the delay, got side tracked but I think the best place to fix this would be in the same patch as https://bugzilla.mozilla.org/show_bug.cgi?id=1476555, I am working on a patch on top of it to fix them both
Actually modifying temporary permissions was easier since they already have the same semantics, this is a working patch, Johann not too keen on on hardcoding "if (permission == BLOCK || key == "autoplay", wondering if you think: 

  "autoplay-media": {
    exactHostMatch: true,
    permitTemporaryAllow: true 

or something similiar is worth it / cleaner
Comment on attachment 8997456 [details]
Bug 1477273 - Allow autoplay-media permission to be temporarily allowed.

That patch seems fine to me on a brief look. Needs a test. :)

(In reply to Dale Harvey (:daleharvey) from comment #8)
> Actually modifying temporary permissions was easier since they already have
> the same semantics, this is a working patch, Johann not too keen on on
> hardcoding "if (permission == BLOCK || key == "autoplay", wondering if you
> think: 
> 
>   "autoplay-media": {
>     exactHostMatch: true,
>     permitTemporaryAllow: true 
> 
> or something similiar is worth it / cleaner

Yes, I think that's worth it / cleaner.
Attachment #8997456 - Flags: feedback+
We talked about this issue at the block autoplay work week, and we decided we want to minimize the number of times we show the doorhanger. So we feel a non-remember allow should persist for the lifetime of the tab, and survive tab reloads. So we should do a temporary allow when "remember" is unchecked.
Comment on attachment 8997456 [details]
Bug 1477273 - Allow autoplay-media permission to be temporarily allowed.

Will clear review since looks like implementation may change
Attachment #8997456 - Flags: review?(jhofmann)
ok so that makes it as the above patch, the only difference being that this temporary permission is not affected by the |clearTemporaryPermissions| that is called when the user manually reloads the page (not when they navigate)

Could add another flag, surviveReload: true and have clear() iterate through all existing permissions and only clear when there is no survivePreload

However although understand wanting to avoid the doorhanger, it does seem like making more special cases for autoplay is going to confuse the user, why does geolocation reask permission and autoplay not? 

Johannh what do you think about this?
Flags: needinfo?(jhofmann)
Ultimately I'd leave this up to your team, I'm really not a big fan of introducing the inconsistency you mention, but if there's strong desire for this sort of behavior I wouldn't r- a surviveReload flag either. :)
Flags: needinfo?(jhofmann)
I'm fine with going with what's consistent with other temporary permissions for now, provided we fix the core issue of one tab requesting permission multiple times inside a single load.
Awesome will reflag for review then, cheers
Attachment #8997456 - Flags: review?(jhofmann)
Comment on attachment 8997456 [details]
Bug 1477273 - Allow autoplay-media permission to be temporarily allowed.

https://reviewboard.mozilla.org/r/261208/#review269548

Thanks!
Attachment #8997456 - Flags: review?(jhofmann) → review+
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/906fa624f03b
Allow autoplay-media permission to be temporarily allowed. r=johannh
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a4b1382784fa
Followup to ensure correct pref in test
https://hg.mozilla.org/mozilla-central/rev/906fa624f03b
https://hg.mozilla.org/mozilla-central/rev/a4b1382784fa
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Verified, that the issue is no longer reproducible on Nightly 63.0a1(20180816220128).
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: