[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

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

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 :)

I'm fixing the Gecko/C++ issue in bug 1477881.

Will take this one for the frontend fix

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.

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

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?

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. :)

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

Comment on attachment 8997456 [details] Bug 1477273 - Allow autoplay-media permission to be temporarily allowed. https://reviewboard.mozilla.org/r/261208/#review269548 Thanks!

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

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