Closed Bug 1317167 Opened 3 years ago Closed 3 years ago

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

Categories

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

52 Branch
x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 + verified
firefox53 - verified

People

(Reporter: s.birbalta, Assigned: alwu)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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
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.
Severity: normal → critical
Flags: needinfo?(s.birbalta)
Keywords: crash
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
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
Crash Signature: [@ nsContentUtils::IsCallerChrome ]
Flags: needinfo?(s.birbalta)
I can't reproduce it with FF52 on Win 7. Linux specific?
Component: Untriaged → DOM
Product: Firefox → Core
Summary: Crash with specific URLs → Crash in nsContentUtils::IsCallerChrome with playing MP3
Hi Alastor, this seems in your wheelhouse. Do you have ideas of what's going on here? Thanks!
Flags: needinfo?(alwu)
The function IsCallerChrome() would be removed in bug1316758, so this issue would no longer exist after landing that bug.
Flags: needinfo?(alwu)
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1316758
Meanwhile I found out this only happens when setting media.autoplay.enabled=false
(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
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 ?
Flags: needinfo?(alwu)
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.
Blocks: 1302350
Status: RESOLVED → REOPENED
Ever confirmed: true
Keywords: regression
Resolution: DUPLICATE → ---
Summary: Crash in nsContentUtils::IsCallerChrome with playing MP3 → Crash in nsContentUtils::IsCallerChrome with playing MP3 with media.autoplay.enabled=false
Crash Signature: [@ nsContentUtils::IsCallerChrome ] → [@ nsContentUtils::IsCallerChrome ] [@ nsContentUtils::SubjectPrincipal ]
(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.
Component: DOM → Audio/Video: Playback
Summary: Crash in nsContentUtils::IsCallerChrome with playing MP3 with media.autoplay.enabled=false → Crash in nsContentUtils::IsCallerChrome when playing MP3 with media.autoplay.enabled=false
(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).
Flags: needinfo?(alwu) → needinfo?(bugs)
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
(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?
Flags: needinfo?(bugs)
Assignee: nobody → 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.
Duplicate of this bug: 1316271
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 on attachment 8811186 [details]
Bug 1317167 - only need to do the untrust JS checking in play().

https://reviewboard.mozilla.org/r/93404/#review93730
Attachment #8811186 - Flags: review?(jwwang) → review+
(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
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0005d0bfadf7
only need to do the untrust JS checking in play(). r=jwwang
https://hg.mozilla.org/mozilla-central/rev/0005d0bfadf7
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Tracking 53- since this is resolved fixed.
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?
Flags: needinfo?(alwu)
s.birbalta, can you verify that this is fixed in tomorrow's nightly?
Flags: needinfo?(s.birbalta)
> 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.
I'll uplift this fix to 52.
Flags: needinfo?(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
Attachment #8811186 - Flags: approval-mozilla-aurora?
> [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.
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 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
Attachment #8811186 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Flags: needinfo?(s.birbalta)
This signature is still occurred with Nightly 20161120030205 on Android, it is also #9 top crash of the same Nightly on Windows.
(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.
Flags: qe-verify+
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.
Flags: qe-verify+
Also verified in latest Developer Edition 53.0a2 as well.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.