Closed Bug 1012912 Opened 5 years ago Closed 5 years ago

mac: killing plugin-container with SIGABRT doesn't trigger the crash reporter

Categories

(Toolkit :: Crash Reporting, defect)

x86
macOS
defect
Not set
Points:
1

Tracking

()

RESOLVED FIXED
mozilla34
Iteration:
34.1
Tracking Status
firefox33 --- fixed
firefox34 --- fixed

People

(Reporter: benjamin, Assigned: gfritzsche)

Details

(Whiteboard: breakpad-upstream)

Attachments

(2 files)

Killing plugin container with SIGABRT (either internally using abort() or externally by sending it that signal) should trigger our crash reporter. This was fixed for the main Firefox process in bug 717758 but it doesn't appear to work for Flash or Silverlight.

Ted it appears that there is an explicit IsOutOfProcess check which makes this not work: http://hg.mozilla.org/mozilla-central/annotate/41a54c8add09/toolkit/crashreporter/google-breakpad/src/client/mac/handler/exception_handler.cc#l629

Do you know why that's there or if I can safely remove it?
Flags: needinfo?(ted)
This dates back to the original implementation for iOS:
https://code.google.com/p/google-breakpad/source/detail?r=933&path=/trunk/src/client/mac/handler/exception_handler.cc

I don't know why it's there, you could try removing it and see what happens.
Flags: needinfo?(ted)
Removing it works fine on OS X. Is there any argument against removing the check now, either completely or specifically for OS X?

If we could do that now it would make QA for GMP crashreporting less complicated until we have bug 1044408.
Flags: needinfo?(ted)
I don't know any reason why it is like it is, so if changing that works then get a patch up. I'll get it upstreamed as well.
Flags: needinfo?(ted)
Attached patch FixSplinter Review
Attachment #8463950 - Flags: review?(ted)
Status: NEW → ASSIGNED
Iteration: --- → 34.1
Points: --- → 1
Flags: firefox-backlog+
Assignee: nobody → georg.fritzsche
QA Whiteboard: [qa?]
QA Whiteboard: [qa?] → [qa-]
Comment on attachment 8463950 [details] [diff] [review]
Fix

Review of attachment 8463950 [details] [diff] [review]:
-----------------------------------------------------------------

Can you provide a patch against upstream SVN that I can submit there?
Attachment #8463950 - Flags: review?(ted) → review+
Attached patch Upstream patchSplinter Review
https://hg.mozilla.org/mozilla-central/rev/bbeb113185da
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Comment on attachment 8463950 [details] [diff] [review]
Fix

Approval Request Comment
[Feature/regressing bug #]: GMP crash testing.
[User impact if declined]: QA & dev complicated a lot for GMP crash testing, this allows us to just kill the plugin-container for that.
[Describe test coverage new/current, TBPL]: Fine with local testing, up on m-c now.
[Risks and why]: Low-risk as this just removes a check that prevent the signal handler from being installed on OS X.
[String/UUID change made/needed]: None.
Attachment #8463950 - Flags: approval-mozilla-aurora?
Attachment #8463950 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This is going to wait for upstream landing until Chrome switches to their Breakpad replacement, per:
https://breakpad.appspot.com/7744002#msg2
Whiteboard: breakpad-upstream
You need to log in before you can comment on or make changes to this bug.