If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Implement writing of minidumps from hang detection on OS X

RESOLVED FIXED

Status

()

Toolkit
Crash Reporting
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: ted, Assigned: ted)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(blocking2.0 beta5+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
In bug 559228 I implemented exception handling for child processes, but not writing of minidumps from the hang detector. We're going to want that too, but it's a little bit more work and I wanted to get the exception handling into 4.0b4.
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
blocking2.0: --- → ?

Updated

7 years ago
blocking2.0: ? → beta5+
(Assignee)

Comment 1

7 years ago
Created attachment 468366 [details] [diff] [review]
almost there

This is close, I just need to fix a couple of things. This works with a test page calling .hang() on the test plugin. Two things:
1) The browser process doesn't wind up with exception info, so it gets no signature, need to fix that
2) Running test_hangning.html in Mochitest hangs.
(Assignee)

Comment 2

7 years ago
(In reply to comment #1)
> 2) Running test_hangning.html in Mochitest hangs.

This turned out to be an issue I had previously filed with Breakpad:
http://code.google.com/p/google-breakpad/issues/detail?id=334

so I wrote a patch:
http://breakpad.appspot.com/165001
(Assignee)

Comment 3

7 years ago
Created attachment 469045 [details] [diff] [review]
Implement plugin hang dump reporting on OS X

Ok, this works and fixes the issues I mentioned previously. This contains two Breakpad patches that are up for review upstream:
http://breakpad.appspot.com/165001/show
http://breakpad.appspot.com/172001/show

The process_util_mac changes are copied from Chromium. In upstream, the LaunchChild implementation was merged with the POSIX one, we could just take that change wholesale to get slightly more in sync (and reduce our codesize by a little bit), but I figure that could come as a followup, since it'd mean a bit more fiddling to match upstream changes.

Most of the other changes are because on OS X, you need a Mach port to do anything useful to another process (like writing a minidump), and the only way to get one of those is to have it messaged to you via Mach IPC from the other process. To get the task port, we use fork_and_get_task from Chromium, which does the messaging right after forking. To get the thread port, we send the thread index in the IPC message when the plugin inits, and look up the port by index on the parent side. (You can get the list of thread ports given the task port. Mach ports are unique per-process, so you can't send the actual port name across non-Mach IPC.)
Attachment #468366 - Attachment is obsolete: true
Attachment #469045 - Flags: review?(jones.chris.g)
Comment on attachment 469045 [details] [diff] [review]
Implement plugin hang dump reporting on OS X

This looks mostly fine to me, though I didn't do a super-close review.

I'm not overjoyed at adding a third kind of Process*/Thread* thing, but I don't have better ideas on hand.  That plus content processes on the horizon probably means the code used to grab subprocess minidumps will want some refactoring, but that's not your fault and doesn't affect beta5.
Attachment #469045 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 5

7 years ago
FWIW, I started down the road of "typedef task_t ProcessHandle", but it wound up being a very invasive change. It certainly makes more sense IMO, but right now we share POSIX code between Linux and OS X all over the place, and lots of it assumes that ProcessHandle == pid_t. We could probably do a two-part refactoring, and audit all users of ProcessHandle to switch ones that want PIDs to use ProcessID instead, and then take the ones that really do need a handle and split the POSIX impl to have a separate OS X path. I wish task_for_pid() would at least work for your own child processes, but it's pretty locked down :-/
(Assignee)

Comment 6

7 years ago
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/846c67bccbfd
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 7

7 years ago
We need to be using 'posix_spawnp' on Mac OS X in order to set the preferred architecture attribute for children, which is necessary for cross-architecture plugin support. This patch moved us in the wrong direction there. See "posix_spawnattr_setbinpref_np".
(Assignee)

Comment 8

7 years ago
Well, if you've got a solution that can do what you want and also allow us to get the child's task_t, feel free to implement it.
You need to log in before you can comment on or make changes to this bug.