Crash in mozilla::MediaShutdownManager::Register

RESOLVED FIXED in Firefox -esr52

Status

()

P1
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

({crash})

unspecified
mozilla54
x86
Windows 10
crash
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox51 unaffected, firefox52 wontfix, firefox-esr52 fixed, firefox53 fixed, firefox54 fixed)

Details

(crash signature)

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
This bug was filed from the Socorro interface and is 
report bp-51e020b8-569c-44a4-b41c-a096a2170206.
=============================================================

We failed the assertion at
http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/dom/media/MediaShutdownManager.cpp#98

Somehow the media element tried to create a decoder after shutdown had begun.
(Assignee)

Updated

2 years ago
Assignee: nobody → jwwang
Priority: -- → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8843175 - Flags: review?(gsquelart)
Attachment #8843176 - Flags: review?(gsquelart)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8843176 [details]
Bug 1343787. Part 2 - move the Register() calls to Load().

https://reviewboard.mozilla.org/r/116942/#review118578
Attachment #8843176 - Flags: review?(gsquelart) → review+

Comment 4

2 years ago
mozreview-review
Comment on attachment 8843175 [details]
Bug 1343787. Part 1 - allow MediaShutdownManager::Register() to fail.

https://reviewboard.mozilla.org/r/116940/#review118580
Attachment #8843175 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 5

2 years ago
Thanks!

Comment 6

2 years ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/063290664ef1
Part 1 - allow MediaShutdownManager::Register() to fail. r=gerald
https://hg.mozilla.org/integration/autoland/rev/32f15879f537
Part 2 - move the Register() calls to Load(). r=gerald

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/063290664ef1
https://hg.mozilla.org/mozilla-central/rev/32f15879f537
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
If I'm reading crash-stats correctly, this affects 52 and 53 as well. Too late for Fx52 at this point, but please request Aurora and ESR52 approval on this when you get a chance.
status-firefox51: --- → unaffected
status-firefox52: --- → wontfix
status-firefox53: --- → affected
status-firefox-esr52: --- → affected
Flags: needinfo?(jwwang)
Indeedy -- this is ranked #12 in the crashes for Windows Aurora of 20170302004002.
(Assignee)

Comment 10

2 years ago
Created attachment 8843801 [details] [diff] [review]
1343787_aurora_53_fix.patch
Attachment #8843801 - Flags: review+
(Assignee)

Comment 11

2 years ago
Comment on attachment 8843801 [details] [diff] [review]
1343787_aurora_53_fix.patch

Approval Request Comment
[Feature/Bug causing the regression]:unknown
[User impact if declined]:crash
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:1336356 must be uplifted first.
[Is the change risky?]:low risk
[Why is the change risky/not risky?]:The change is simple which add error handling to some functions.
[String changes made/needed]:none
Flags: needinfo?(jwwang)
Attachment #8843801 - Flags: approval-mozilla-aurora?
Attachment #8843801 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8843801 [details] [diff] [review]
1343787_aurora_53_fix.patch

Part three of a big media shutdown crash fix, please uplift this after the patches in bug 1336345 and bug 1336356.
Attachment #8843801 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 13

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/9c697f08d301
status-firefox53: affected → fixed
Setting qe-verify- based on JW Wang's assessment on manual testing needs (see Comment 11) and the fact that this fix has automated coverage.
Flags: qe-verify-
(Assignee)

Comment 15

2 years ago
Created attachment 8847918 [details] [diff] [review]
1343787_esr52_fix.patch
(Assignee)

Comment 16

2 years ago
Comment on attachment 8847918 [details] [diff] [review]
1343787_esr52_fix.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: crash
Fix Landed on Version:54
Risk to taking this patch (and alternatives if risky): The change is simple which add error handling to some functions. This patch has been uplifted to 54 and 53.
String or UUID changes made by this patch: none
Attachment #8847918 - Flags: approval-mozilla-esr52?
Comment on attachment 8847918 [details] [diff] [review]
1343787_esr52_fix.patch

crash fix, regression in 52, esr52+
Attachment #8847918 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+

Comment 18

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/28287b7f0938
status-firefox-esr52: affected → fixed
You need to log in before you can comment on or make changes to this bug.