Remove the IPC channel used to send crash annotations
Categories
(Toolkit :: Crash Reporting, task)
Tracking
()
Tracking | Status | |
---|---|---|
firefox116 | --- | fixed |
People
(Reporter: gsvelto, Assigned: gsvelto)
References
(Blocks 2 open bugs)
Details
Attachments
(5 files)
The idea here is to get rid of the pipe we use to send crash annotations from child processes to the main process. To do so the main process - and in the future the crash monitor - need to be able to pull the crash annotations directly from the crashed process' memory. This has several advantages:
- One less IPC channel, duh
- We can't end up in a deadlock between this channel and the one used by Breakpad to ask for minidump generation (something that has plagued us for a long time)
- We can cut away a large piece of code that does complex stuff in the child process exception handler
- We can make crash annotations a lot cheaper by moving from a push to a pull model
I've studied what would be the best way to do this and I found an interesting approach in Crashpad that does not involve exposing a symbol (and thus having to parse the symbol table in the target process). What they do is create a section (on Linux, Windows and macOS) with a well known name and stuff the global structure that holds crash annotations there. The process that needs to extract the allocations pulls it out of the target process' sections and thus knows where to look for them without having to touch the symbol table.
I haven't thought of a better (cross-platform) way to do it that doesn't involve any work from the child process so I think I'll go for it.
Assignee | ||
Comment 1•2 years ago
|
||
BTW if someone has alternatives on how to make the parent process locate the annotations in the child process I'm all ears.
Comment hidden (obsolete) |
Comment 3•2 years ago
|
||
(Addendum: I forgot how I got here, and I now see that you've implied you're well into an implementation strategy already. Sorry for the distraction!)
Comment 4•2 years ago
|
||
FWIW, I'm pretty sure it's safe to assume that all segments (and, by extension, sections) will get the same offset to their virtual addresses when loaded. In particular, if that weren't the case, all PC-relative accesses from code to data/bss would need to be relocated, and modern toolchains try pretty hard to avoid text relocations. However, we still have cross-architecture child processes for media plugins on arm64 on some platforms, so that complicates things a little.
Also, to clarify a little for the historical record: Crashpad's approach iterates the program headers (a.k.a. segments) to find the one with type PT_NOTE
, which contains the various .note.*
sections concatenated, iterates the notes (they're in a standard format with length fields etc.) to find the one (or ones?) with Crashpad's identifier, and that has a relative pointer to a struct that it knows how to read. IMHO it's not awful, but it's a bit complicated and we may not need that level of generality.
Assignee | ||
Comment 5•2 years ago
|
||
Thanks for the tips, one of the problems I'm facing is that I really want this system to be ready for when we'll take minidumps from an external process instead of from the main process. Because of this I'm trying to design this in a way that minimizes what the reader needs to know about the layout of the process holding the annotations. I'm stuffing the object that holds the annotations within a process in a dedicated section with an easy-to-find name. This way the reader process only needs to scour the sections looking for the right one and read from there. This is a compromise: other stuff could be put in that section, messing up my assumptions, and the annotation table could be included more than once but in practice neither are likely to happen.
BTW I hope to be able to upload my set of very-ugly-but-working-on-linux patches later tonight.
Assignee | ||
Comment 6•2 years ago
|
||
Scratch what I said above. For various practical reasons putting the crash annotations in a separate section is not a good idea because of the way we could fail to find it. I've got to do the notes thing like crashpad (sigh) hoping that I won't encounter the linker bugs they've run into back in the day.
Assignee | ||
Comment 7•2 years ago
|
||
This crate contains the required bits to store annotations in a process and
let the mozannotation_server crate find them and pull them out. Functionally
there's not much happening here as all the actual functionality is in
mozannotation_server.
Assignee | ||
Comment 8•2 years ago
|
||
This crate contains the logic used to find the annotations within another
process and pull them out.
Depends on D173697
Assignee | ||
Comment 9•2 years ago
|
||
Depends on D173698
Assignee | ||
Comment 10•2 years ago
|
||
Depends on D173699
Updated•2 years ago
|
Assignee | ||
Comment 11•2 years ago
|
||
Alright, here comes the WIP for this. It's still rough around the edges but it's functionally complete (on Linux and Android) and passes all the tests we have. I still need to factorize the platform-specific functionality in the new crates and add support for Windows and macOS but I'd say that I'm 90% there now.
Assignee | ||
Comment 12•2 years ago
|
||
Update: I'm stuck dealing with rust's metadata hash. I need to figure a way to retrieve a symbol name (which boils down to finding the metadata hash when it gets compiled) and I haven't found a good way yet.
Assignee | ||
Comment 13•2 years ago
|
||
Updating the update: I'm unstuck, yay! Time for the Windows and macOS implementations.
Updated•1 years ago
|
Updated•1 years ago
|
Updated•1 years ago
|
Updated•1 years ago
|
Assignee | ||
Comment 14•1 years ago
|
||
Depends on D173700
Assignee | ||
Comment 16•1 year ago
|
||
All the reviews are in. Rebased my patch set and a final try run is here: https://treeherder.mozilla.org/jobs?repo=try&revision=c06d3790a5a07402720ed97535c560d1a1d39ec6
If all goes well I'll land this today.
Assignee | ||
Comment 17•1 year ago
|
||
Ugh, silly issue in the previous try run, new one is here: https://treeherder.mozilla.org/jobs?repo=try&revision=d0f5bf1b6e508c3b469d643090d73691f64279b3
Assignee | ||
Comment 18•1 year ago
|
||
Try is green(ish), patches are rebased, manual tests look good. Alright chums, let's do this!
Comment 19•1 year ago
|
||
Comment 20•1 year ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a45d357f36c6
https://hg.mozilla.org/mozilla-central/rev/5ba9e96d7820
https://hg.mozilla.org/mozilla-central/rev/eec04732422a
https://hg.mozilla.org/mozilla-central/rev/b665cbdccb30
https://hg.mozilla.org/mozilla-central/rev/e88255995477
Description
•