Closed
Bug 1163239
Opened 9 years ago
Closed 9 years ago
GMP Crash at GMPParent::ChildTerminated() on OSX on FF38 on shutdown after using openh264.
Categories
(Core :: WebRTC: Audio/Video, defect)
Core
WebRTC: Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: ehugg, Assigned: ehugg)
Details
(Keywords: crash)
Attachments
(1 file, 3 obsolete files)
1.57 KB,
patch
|
ehugg
:
review+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Testing FF38 release candidates with H264 and consistently get these crashes on OSX. Do not see them on Windows builds: https://crash-stats.mozilla.com/report/index/c91923a7-f0f4-4abf-ac87-300ee2150508 https://crash-stats.mozilla.com/report/index/6b5ccc8f-461d-48bd-a1ac-48f362150508 H264 video works fine, crash comes on browser exit.
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → ethanhugg
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
Comment on attachment 8603656 [details] [diff] [review] Check mGMPThread on shutdown Review of attachment 8603656 [details] [diff] [review]: ----------------------------------------------------------------- This hack makes the shutdown crash go away on OSX.
Comment 3•9 years ago
|
||
I also have not seen them on Linux64 - only on Mac. They're not 100% for me, but frequent.
Assignee | ||
Comment 4•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8603656 -
Attachment is obsolete: true
Assignee | ||
Comment 5•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8604200 -
Attachment is obsolete: true
Assignee | ||
Comment 6•9 years ago
|
||
Comment on attachment 8604203 [details] [diff] [review] Check mGMPThread on shutdown Review of attachment 8604203 [details] [diff] [review]: ----------------------------------------------------------------- Without this patch FF38 will go start crash reporter on shutdown every time after doing video calls with ciscospark.com on OSX. do_GetService is returning null here: https://dxr.mozilla.org/mozilla-central/source/dom/media/gmp/GMPParent.cpp#415 since we are shutting down and we are doing a deref of null here: https://dxr.mozilla.org/mozilla-central/source/dom/media/gmp/GMPParent.cpp#375
Attachment #8604203 -
Flags: review?(cpearce)
Comment 7•9 years ago
|
||
Comment on attachment 8604203 [details] [diff] [review] Check mGMPThread on shutdown Review of attachment 8604203 [details] [diff] [review]: ----------------------------------------------------------------- I think the core problem here that when GMPParent::DeleteProcess() calls mProcess->Delete(NS_NewRunnableMethod(this, &GMPParent::ChildTerminated)), the ChildTerminated task being dispatched is racing with the shutdown notification from xpcom, that is, the shutdown observer in GeckoMediaPluginServiceParent::Observe() runs, and shutsdown the GMP thread before the ChildTerminated task gets a chance to run. Fixing it properly is probably more trouble than its worth. Sorry for the slow review; I felt it necessary to understand what was going on here. ::: dom/media/gmp/GMPParent.cpp @@ +405,5 @@ > nsRefPtr<GMPParent> self(this); > + nsIThread* gmpThread = GMPThread(); > + > + if (!gmpThread) { > + // Bug 1163239 - this can happen on shutdown. Please amend the comment to also say that the PluginTerminated also just removes the GMP from the GMPService, but if we've already shutdown, that's already been done, so not doing this here will probably be OK.
Attachment #8604203 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 8•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8604203 -
Attachment is obsolete: true
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8607111 [details] [diff] [review] Check mGMPThread on shutdown Updated the comment and bringing forward r+ from cpearce.
Attachment #8607111 -
Flags: review+
Assignee | ||
Comment 10•9 years ago
|
||
Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=24aefcb4190f
Assignee | ||
Comment 11•9 years ago
|
||
Marking as checkin-needed. Try run is in comment #10.
Keywords: checkin-needed
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b84ee790d254
Keywords: checkin-needed
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b84ee790d254
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 15•9 years ago
|
||
Please mark status-xxxx, and consider if we should ask for uplift to 40 or ESR38. It sounds like the main impact would be spurious null-deref plugin crashes
Flags: needinfo?(ethanhugg)
Assignee | ||
Comment 16•9 years ago
|
||
This is a wallpaper patch for a race condition on shutdown which was probably fixed for real by cpearce in bug 1171496 and uplifted to 39 and 40. That said, it wouldn't hurt to have this low-risk patch uplifted to 40 so I will request that. I don't know about the ESR release. It looks like the patch in bug 1171496 is not going into ESR38, so perhaps this one would prevent some crash reports there.
status-firefox39:
--- → affected
status-firefox40:
--- → affected
status-firefox42:
--- → fixed
Flags: needinfo?(ethanhugg)
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8607111 [details] [diff] [review] Check mGMPThread on shutdown Approval Request Comment [Feature/regressing bug #]: 1163239 - bug 1171496 may also fix this but we don't know for sure. [User impact if declined]: Firefox may crash on shutdown after using a GMP plugin so it would crashreport on a user who just closed the browser. [Describe test coverage new/current, TreeHerder]: This fix has been in FF41 for some time. Tested by hand with Spark. [Risks and why]: We're just checking for null here before doing a deref. It happens on shutdown so we do not need to recover. [String/UUID change made/needed]: none
Attachment #8607111 -
Flags: approval-mozilla-beta?
Comment 18•9 years ago
|
||
Comment on attachment 8607111 [details] [diff] [review] Check mGMPThread on shutdown Crash fix that has been on 41 for a month. Let's get this into 40 beta3. Beta+
Attachment #8607111 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in
before you can comment on or make changes to this bug.
Description
•