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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox39 --- wontfix
firefox40 --- fixed
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: ehugg, Assigned: ehugg)

Details

(Keywords: crash)

Attachments

(1 file, 3 obsolete files)

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.
Attached patch Check mGMPThread on shutdown (obsolete) — Splinter Review
Assignee: nobody → ethanhugg
Status: NEW → ASSIGNED
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.
I also have not seen them on Linux64 - only on Mac.  They're not 100% for me, but frequent.
Attached patch Check mGMPThread on shutdown (obsolete) — Splinter Review
Attachment #8603656 - Attachment is obsolete: true
Attached patch Check mGMPThread on shutdown (obsolete) — Splinter Review
Attachment #8604200 - Attachment is obsolete: true
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 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+
Attachment #8604203 - Attachment is obsolete: true
Comment on attachment 8607111 [details] [diff] [review]
Check mGMPThread on shutdown


Updated the comment and bringing forward r+ from cpearce.
Attachment #8607111 - Flags: review+
Marking as checkin-needed.  Try run is in comment #10.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b84ee790d254
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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)
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.
Flags: needinfo?(ethanhugg)
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 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.