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

VERIFIED FIXED in Firefox 52

Status

()

--
critical
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: s.birbalta, Assigned: alwu)

Tracking

({crash, regression})

52 Branch
mozilla53
x86_64
Linux
crash, regression
Points:
---

Firefox Tracking Flags

(firefox50 unaffected, firefox51 unaffected, firefox52+ verified, firefox53- verified)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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

2 years ago
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
(Reporter)

Comment 2

2 years ago
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

Updated

2 years ago
Crash Signature: [@ nsContentUtils::IsCallerChrome ]
Flags: needinfo?(s.birbalta)

Comment 3

2 years ago
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)
(Assignee)

Comment 5

2 years ago
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
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1316758
(Reporter)

Comment 7

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

Comment 8

2 years ago
(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)

Comment 10

2 years ago
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
status-firefox50: --- → unaffected
status-firefox51: --- → unaffected
status-firefox52: --- → affected
status-firefox53: --- → affected
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
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

Updated

2 years ago
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.

Updated

2 years ago
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
(Assignee)

Comment 12

2 years ago
(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)
(Assignee)

Comment 13

2 years ago
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)

Updated

2 years ago
Assignee: nobody → alwu
(Assignee)

Comment 15

2 years ago
(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 hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1316271

Comment 18

2 years ago
mozreview-review
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 hidden (mozreview-request)

Comment 20

2 years ago
mozreview-review
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+
(Assignee)

Comment 21

2 years ago
(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 23

2 years ago
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

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0005d0bfadf7
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Tracking 53- since this is resolved fixed.
tracking-firefox53: ? → -
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.
tracking-firefox52: ? → +
(Assignee)

Comment 29

2 years ago
I'll uplift this fix to 52.
Flags: needinfo?(alwu)
(Assignee)

Comment 30

2 years ago
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+

Comment 34

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/7561e199d0f4
status-firefox52: affected → fixed
(Reporter)

Comment 35

2 years ago
(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.
(Assignee)

Comment 37

2 years ago
(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.
status-firefox52: fixed → verified
Flags: qe-verify+
Also verified in latest Developer Edition 53.0a2 as well.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
You need to log in before you can comment on or make changes to this bug.