1317167 - Crash in nsContentUtils::IsCallerChrome when playing MP3 with media.autoplay.enabled=false

Status: VERIFIED FIXED

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

Description

[Sinan]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20161113030203

Steps to reproduce:

In Nightly, open a file in this directory listing:
http://media-stream-pmd.rbb-online.de/content/
for example
http://media-stream-pmd.rbb-online.de/content/00a140ee-2796-4e73-8c75-fc0897175cdb_680e680d-9958-43b6-89e8-df76fc5e8a7c.mp3

Addons are disabled


Actual results:

Tab crashes.
Nightly notifies that the tab crashed.


Expected results:

Shoud open the URL as mp3

Comment 1

[YF (Yang)]

Could you provide a crash ID?
https://developer.mozilla.org/docs/How_to_get_a_stacktrace_for_a_bug_report.


WFM in 52.0a1 (2016-11-13) (32-bit) on Win10.

Comment 2

[Sinan]

I'm not sure which one it is, the crash reporter uploaded two:
https://crash-stats.mozilla.com/report/index/cf61c154-195a-4c70-861f-0f66e2161114
https://crash-stats.mozilla.com/report/index/cb082b5b-0a3b-4884-971c-aed1d2161114

By the way, it doesn't happen with Firefox 49.0.2, with many add-ons

Comment 3

[Loic]

I can't reproduce it with FF52 on Win 7. Linux specific?

Comment 4

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Hi Alastor, this seems in your wheelhouse. Do you have ideas of what's going on here? Thanks!

Comment 5

[Alastor Wu [:alwu]]

The function IsCallerChrome() would be removed in bug1316758, so this issue would no longer exist after landing that bug.

Comment 7

[Sinan]

Meanwhile I found out this only happens when setting media.autoplay.enabled=false

Comment 8

[Loic]

(In reply to s.birbalta from comment #7)
> Meanwhile I found out this only happens when setting
> media.autoplay.enabled=false

Nice, in this case, I crash too.
https://crash-stats.mozilla.com/report/index/ce19946d-7f68-4039-aaec-c919c2161115

Comment 9

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

So this isn't a bug we need to think to fix on branches?
bug 1316758 won't land to any branches.

Can the value of media.autoplay.enabled be changed using some UI?
Like http://searchfox.org/mozilla-central/source/mobile/android/base/resources/xml/preferences_advanced.xml#63 ?

Comment 10

[Loic]

Reg range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c22b06e7fba4a0c6bf4c1fdf451ebf52633b6767&tochange=10d414508e1798890cbe04007a726c80a6dc7514

Let's reopen the bug to check if the issue disappears after bug 1316758. If not, this crash should be fixed normally.

Comment 11

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

(In reply to Olli Pettay [:smaug] from comment #9)
> So this isn't a bug we need to think to fix on branches?
> bug 1316758 won't land to any branches.
> 
> Can the value of media.autoplay.enabled be changed using some UI?
> Like
> http://searchfox.org/mozilla-central/source/mobile/android/base/resources/
> xml/preferences_advanced.xml#63 ?

UI to change media.autoplay.enabled is exposed in Firefox for Android only; their's no UI exposed (other than about:config) to change it on desktop.

Comment 12

[Alastor Wu [:alwu]]

(In reply to Olli Pettay [:smaug] from comment #9)
> So this isn't a bug we need to think to fix on branches?
> bug 1316758 won't land to any branches.
> 
> Can the value of media.autoplay.enabled be changed using some UI?
> Like
> http://searchfox.org/mozilla-central/source/mobile/android/base/resources/
> xml/preferences_advanced.xml#63 ?

Hi, Smaug,

Why you said that "bug 1316758 won't land to any branches"?

And remove this function seems a good solution if it is not useful anymore (the reason was described in bug1316758 comment0).

Comment 13

[Alastor Wu [:alwu]]

The problem here was we lacks the current JS context and it was caused by the special initialization process for the video document. JW said that the video document would be initialized before lots of DOM thing (bad thing but hard to fix that), that is why we can't get the JS context in SubjectPrincipal().

There are three checking conditions in IsAllowedToPlay() [1], but in the [2] we only need to check the second and third conditions (so we don't need to call IsAllowedToPlay() there).

So the easy way to fix this crash is to "don't check the first checking condition in CanActivateAutoplay()."

[1] http://searchfox.org/mozilla-central/rev/8562d3859b89ac89f46690b9ed2c473e0728d6c0/dom/html/HTMLMediaElement.cpp#6020

[2] http://searchfox.org/mozilla-central/rev/8562d3859b89ac89f46690b9ed2c473e0728d6c0/dom/html/HTMLMediaElement.cpp#4988a

Comment 14

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

(In reply to Alastor Wu [:alwu] from comment #12)
 
> Why you said that "bug 1316758 won't land to any branches"?
Uh, I was thinking different bug (the bug removing nsContentUtils::IsCallerChrome everywhere). Sorry. Is bug 1316758 safe enough for branches?

Comment 15

[Alastor Wu [:alwu]]

(In reply to Olli Pettay [:smaug] from comment #14)
> (In reply to Alastor Wu [:alwu] from comment #12)
>  
> > Why you said that "bug 1316758 won't land to any branches"?
> Uh, I was thinking different bug (the bug removing
> nsContentUtils::IsCallerChrome everywhere). Sorry. Is bug 1316758 safe
> enough for branches?

I'll commit another patch to fix this crash, the root cause is in the comment13.

Comment 16

[Alastor Wu [:alwu]]

Attachment: Bug 1317167 - only need to do the untrust JS checking in play().

The first checking condition in the IsAllowedPlay() is used to prevent the play() operation
is called by untrusted JS when media.autoplay.enabled=false. Therefore, we don't need to check
that in CanActivateAutoplay().

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

Comment 18

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8811186 [details]
Bug 1317167 - only need to do the untrust JS checking in play().

https://reviewboard.mozilla.org/r/93404/#review93718

::: dom/html/HTMLMediaElement.cpp:6024
(Diff revision 1)
>  bool
> -HTMLMediaElement::IsAllowedToPlay()
> +HTMLMediaElement::IsAllowedToPlay(bool aIsCalledFromPlay)
>  {
> -  // Prevent media element from being auto-started by a script when
> -  // media.autoplay.enabled=false
> -  if (!mHasUserInteraction &&
> +  // Prevent media element from being auto-started via calling play() by a script
> +  // when media.autoplay.enabled=false
> +  if (aIsCalledFromPlay &&

We don't want to check handling-user-input if this function is called by CanActivateAutoplay(), right?

Then the site will be able to start playback without user interaction by setting 'autoplay' to true even if IsAutoplayEnabled() returns false. This doesn't sound right to me.

Comment 19

[Alastor Wu [:alwu]]

Comment on attachment 8811186 [details]
Bug 1317167 - only need to do the untrust JS checking in play().

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

Comment 20

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8811186 [details]
Bug 1317167 - only need to do the untrust JS checking in play().

https://reviewboard.mozilla.org/r/93404/#review93730

Comment 21

[Alastor Wu [:alwu]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #18)
> We don't want to check handling-user-input if this function is called by
> CanActivateAutoplay(), right?
> 
> Then the site will be able to start playback without user interaction by
> setting 'autoplay' to true even if IsAutoplayEnabled() returns false. This
> doesn't sound right to me.

The playback would still be blocked by the second condition in [1], mAutoplayEnabled would be false if we turn off the pref.

[1] http://searchfox.org/mozilla-central/rev/efcb1644e49c36445e75d89b434fdf4c84832c84/dom/html/HTMLMediaElement.cpp#4969

Comment 22

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b46247378cfd15d9b3440ace0b7e192a09b5204d&selectedJob=31211777

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0005d0bfadf7
only need to do the untrust JS checking in play(). r=jwwang

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0005d0bfadf7

Comment 25

[Marcia Knous [:marcia]]

Tracking 53- since this is resolved fixed.

Comment 26

[Julien Cristau [:jcristau]]

Alastor, is this something we should get in 52, or do we expect media.autoplay.enabled=false to be a small enough edge case that it's not worth it?

Comment 27

[Julien Cristau [:jcristau]]

s.birbalta, can you verify that this is fixed in tomorrow's nightly?

Comment 28

[Boris Zbarsky [:bzbarsky]]

> or do we expect media.autoplay.enabled=false to be a small enough edge case that it's not worth it?

Well, this is the #1 Android topcrash on nightly, per bug 1316271.  So...

It's too bad people didn't copy the tracking flag when the dupped the tracked bug here.

Comment 29

[Alastor Wu [:alwu]]

I'll uplift this fix to 52.

Comment 30

[Alastor Wu [:alwu]]

Comment on attachment 8811186 [details]
Bug 1317167 - only need to do the untrust JS checking in play().

Approval Request Comment
[Feature/regressing bug #]: fix the crash
[User impact if declined]: it would cause crash if user set media.autoplay.enabled=false 
[Describe test coverage new/current, TreeHerder]: pass all the current tests
[Risks and why]: almost no risk, only skip the pref checking for autoplay
[String/UUID change made/needed]: no

Comment 31

[Boris Zbarsky [:bzbarsky]]

> [User impact if declined]: it would cause crash if user set media.autoplay.enabled=false 

To be clear, this is the _default_ setting on Android.  Hence this being a topcrash there.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Or at least in some configurations......  Looks like only if the SDK version is < 16, which means Android 2.3-4.0 per the commit message that added that code.  On those Android versions, the default is the crashing configuration.

Comment 33

[Julien Cristau [:jcristau]]

Comment on attachment 8811186 [details]
Bug 1317167 - only need to do the untrust JS checking in play().

fix crash when playing audio in some configurations on aurora52

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7561e199d0f4

Comment 35

[Sinan]

(In reply to Julien Cristau [:jcristau] from comment #27)
> s.birbalta, can you verify that this is fixed in tomorrow's nightly?

Sorry for answering late, somehow bugzilla doesn't remember that I'm logged in (via Github) and therefore I didn't see I should answer. So, the particular issue is also fixed for me.

Comment 36

[Ting-Yu Chou [:ting] (away)]

This signature is still occurred with Nightly 20161120030205 on Android, it is also #9 top crash of the same Nightly on Windows.

Comment 37

[Alastor Wu [:alwu]]

(In reply to Ting-Yu Chou [:ting] from comment #36)
> This signature is still occurred with Nightly 20161120030205 on Android, it
> is also #9 top crash of the same Nightly on Windows.

The bug 1316758 would fix the crash completely.
This bug removed the wrong condition checking in media element, the bug1316758 removed all other calls for nsContentUtils::IsCallerChrome.

Comment 38

[Bogdan Maris, Desktop QA]

Reproduced the initial crash using old Nightly from 2016-11-13 (bp-2e095f32-be14-421f-9a2a-143e82170210). Verified that the crash does not occur on Firefox 52 beta 5 with media.autoplay.enabled set to both true or false.

Comment 39

[Bogdan Maris, Desktop QA]

Also verified in latest Developer Edition 53.0a2 as well.