1493766 - AutoplayPolicy::IsAllowedToPlay() will return wrong result for audible media when page only has temporary autoplay permission (without user activation)

Status: RESOLVED FIXED

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

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

STR: open the HTML file enclosed in 

https://bugzilla.mozilla.org/show_bug.cgi?id=1493089#c2

locally in file:

wait for the video to loop


Assertion failure: AutoplayPolicy::IsAllowedToPlay(*this), at /builds/worker/workspace/build/src/dom/html/HTMLMediaElement.cpp:6200
#01: mozilla_dump_image[/Users/timdream/repo/gecko/obj-x86_64-apple-darwin16.7.0-artifact/dist/NightlyDebug.app/Contents/MacOS/XUL +0x29c86aa]
#02: mozilla_dump_image[/Users/timdream/repo/gecko/obj-x86_64-apple-darwin16.7.0-artifact/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2aeaf43]
#03: mozilla_dump_image[/Users/timdream/repo/gecko/obj-x86_64-apple-darwin16.7.0-artifact/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2b28f4e]
#04: thread-local wrapper routine for mozilla::SchedulerGroup::sTlsValidatingAccess[/Users/timdream/repo/gecko/obj-x86_64-apple-darwin16.7.0-artifact/dist/NightlyDebug.app/Contents/MacOS/XUL +0x11573b]
#05: thread-local wrapper routine for mozilla::SchedulerGroup::sTlsValidatingAccess[/Users/timdream/repo/gecko/obj-x86_64-apple-darwin16.7.0-artifact/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1144c9]
#06: thread-local wrapper routine for mozilla::SchedulerGroup::sTlsValidatingAccess[/Users/timdream/repo/gecko/obj-x86_64-apple-darwin16.7.0-artifact/dist/NightlyDebug.app/Contents/MacOS/XUL +0x11183a]
#07: thread-local wrapper routine for mozilla::SchedulerGroup::sTlsValidatingAccess[/Users/timdream/repo/gecko/obj-x86_64-apple-darwin16.7.0-artifact/dist/NightlyDebug.app/Contents/MacOS/XUL +0x129418]
#08: thread-local wrapper routine for mozilla::SchedulerGroup::sTlsValidatingAccess[/Users/timdream/repo/gecko/obj-x86_64-apple-darwin16.7.0-artifact/dist/NightlyDebug.app/Contents/MacOS/XUL +0x12cfc3]
#09: nsXPTCStubBase::Stub249()[/Users/timdream/repo/gecko/obj-x86_64-apple-darwin16.7.0-artifact/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767006]
#10: nsXPTCStubBase::Stub249()[/Users/timdream/repo/gecko/obj-x86_64-apple-darwin16.7.0-artifact/dist/NightlyDebug.app/Contents/MacOS/XUL +0x716c57]
#11: thread-local wrapper routine for mozilla::dom::FlushRejections::sDispatched[/Users/timdream/repo/gecko/obj-x86_64-apple-darwin16.7.0-artifact/dist/NightlyDebug.app/Contents/MacOS/XUL +0x34e3dc9]
#12: thread-local wrapper routine for mozilla::dom::FlushRejections::sDispatched[/Users/timdream/repo/gecko/obj-x86_64-apple-darwin16.7.0-artifact/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3555b0f]
#13: RecordReplayInterface_DefineRecordReplayControlObject[/Users/timdream/repo/gecko/obj-x86_64-apple-darwin16.7.0-artifact/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4c77e03]
#14: nsXPTCStubBase::Stub249()[/Users/timdream/repo/gecko/obj-x86_64-apple-darwin16.7.0-artifact/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7678c2]
#15: nsXPTCStubBase::Stub249()[/Users/timdream/repo/gecko/obj-x86_64-apple-darwin16.7.0-artifact/dist/NightlyDebug.app/Contents/MacOS/XUL +0x716c57]
#16: RecordReplayInterface_DefineRecordReplayControlObject[/Users/timdream/repo/gecko/obj-x86_64-apple-darwin16.7.0-artifact/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4c77952]
#17: ???[/Users/timdream/repo/gecko/obj-x86_64-apple-darwin16.7.0-artifact/dist/NightlyDebug.app/Contents/MacOS/plugin-container.app/Contents/MacOS/plugin-container +0xf39]

Comment 1

[Nils Ohlmeier [:drno]]

Alastor could you please have a look at this?

Comment 2

[Alastor Wu [:alwu]]

Hi, Dale,

I found that when user click "allow" button on the doorhanger, the temporary permission [1] seems not set correctly. That resulted in that the return value of nsContentUtils::IsExactSitePermAllow() [3] always false, because we can't find the correspending permission in nsPermissionManager [3].

I'm wondering if there is any places in front end would revoke the permission unexpectedly or front-end side doesn't set the permission correctly?

Thank you!

[1] https://searchfox.org/mozilla-central/rev/ffe6eaf2f032e58ec3b0650a87df2c62ae4ca441/browser/modules/PermissionUI.jsm#347

[2] https://searchfox.org/mozilla-central/rev/ffe6eaf2f032e58ec3b0650a87df2c62ae4ca441/dom/media/AutoplayPolicy.cpp#98

[3] https://searchfox.org/mozilla-central/rev/ffe6eaf2f032e58ec3b0650a87df2c62ae4ca441/extensions/cookie/nsPermissionManager.cpp#2772

Comment 3

[Dale Harvey (:daleharvey)]

Hey Alastor

Temporary permissions are only stored in the frontend (https://dxr.mozilla.org/mozilla-central/source/browser/modules/SitePermissions.jsm#23) so nsContentUtils::IsExactSitePermAllow isnt going to see them being set.

I am not sure there is a simple way to do so, depending on your use case it may be possible to ask the frontend for the current permission, but if you could explain how the above is broken then :johannh is best to ask

Comment 4

[Alastor Wu [:alwu]]

Attachment: Bug 1493766 - part1 : notify when temporary permission changed.

Since temporary permissions are only stored in the front-end side, we can't know whether we have
allowed page to autoplay or not without sending a request. Therefore, we want to notify the back-end
side when the temporary permissions changed.

Comment 5

[Alastor Wu [:alwu]]

Attachment: Bug 1493766 - part2 : save the status of temporary autoplay permission in outer window.

In order to know whether we have temporary autoplay permission without creating a request, we need to
save it to the top level document so that we can get the correct returned value for AutoplayPolicy::IsAllowedToPlay().

Comment 6

[Alastor Wu [:alwu]]

Attachment: Bug 1493766 - part3 : modify test.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9012400 [details]
Bug 1493766 - part2 : save the status of temporary autoplay permission in outer window.

Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.) has approved the revision.

Comment 8

[Alastor Wu [:alwu]]

Hi, Johannh, 

Could you give me some feedback about my reply [1]?

This bug will also affect the plan of implementing `video.allowedToPlay`[2]. We want to implement that new attribute for media elemenet in order to let sites know whether they can start play or not without showing the doorhanger. If everytime we checking temporary permission would cause a new promt showing, that is completely violate the rationale of the `video.allowedToPlay`.

Thank you!

[1] https://phabricator.services.mozilla.com/D7011#172372
[2] https://github.com/whatwg/html/issues/3617

Comment 9

[Johann Hofmann [:johannh]]

Hey, sorry for the delay, this really isn't a pleasant issue (and I guess it was to be expected that bending temporary permissions to do something they were not designed to would have some side effects).

I think the cleanest way to solve this is to take the same path as a normal permission request but instruct the front-end to avoid prompting. Syncing the temporary permission state into content land sounds really hard to get right.

This could be done by adding a "probe" method to nsIContentPermissionPrompt that prompts in PermissionUI.jsm could optionally implement. In this method we could simply query the corresponding permission and return its status. It's really not beautiful, but I'm struggling to come up with a better way to do this that's not rewriting the whole thing.

Paolo used to work on this, too and might have an opinion, CC'ing him just in case.

Comment 11

[Alastor Wu [:alwu]]

(In reply to Johann Hofmann [:johannh] from comment #9)
> Hey, sorry for the delay, this really isn't a pleasant issue (and I guess it
> was to be expected that bending temporary permissions to do something they
> were not designed to would have some side effects).
> 
> I think the cleanest way to solve this is to take the same path as a normal
> permission request but instruct the front-end to avoid prompting. Syncing
> the temporary permission state into content land sounds really hard to get
> right.
> 
> This could be done by adding a "probe" method to nsIContentPermissionPrompt
> that prompts in PermissionUI.jsm could optionally implement. In this method
> we could simply query the corresponding permission and return its status.
> It's really not beautiful, but I'm struggling to come up with a better way
> to do this that's not rewriting the whole thing.
> 
> Paolo used to work on this, too and might have an opinion, CC'ing him just
> in case.

Hi, Johann,

As we need to know the result synchronously, the way which requires a round-trip between content and chrome process seems not work for us.

The API `AutoplayPolicy::IsAllowedToPlay()` is designed as synchronous in the beginning, and the WebAPI we're discussing now also requires synchronous result in order to let sites know whether they can play autoplay media or not.

Comment 12

[Johann Hofmann [:johannh]]

Followed up via IRC and Vidyo a while ago, so clearing needinfo. Sorry for all the delays from my side, this is quite a difficult problem.

Comment 13

[Alastor Wu [:alwu]]

Attachment: Bug 1493766 - part4 : add test.

Add a test to ensure the temporary autoplay permission is sync correctly between JS and C++ side.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e0a171941b2
part1 : notify when temporary permission changed. r=johannh
https://hg.mozilla.org/integration/autoland/rev/41fe6af27564
part2 : save the status of temporary autoplay permission in outer window. r=johannh,smaug
https://hg.mozilla.org/integration/autoland/rev/7a9c6dea709e
part3 : modify test. r=smaug
https://hg.mozilla.org/integration/autoland/rev/2c6f77ee5cf5
part4 : add test. r=johannh

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/0e0a171941b2
https://hg.mozilla.org/mozilla-central/rev/41fe6af27564
https://hg.mozilla.org/mozilla-central/rev/7a9c6dea709e
https://hg.mozilla.org/mozilla-central/rev/2c6f77ee5cf5