1776197 - Remove the IPC channel used to send crash annotations

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, task)

Description

[Gabriele Svelto [:gsvelto]]

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.

Comment 1

[Gabriele Svelto [:gsvelto]]

BTW if someone has alternatives on how to make the parent process locate the annotations in the child process I'm all ears.

Comment 2

[Ray Kraesig [:rkraesig]]

On Windows, what we do for the launcher process is:

• define an ordinary (global) variable;
• determine at runtime the offset from our image's base address for that variable;
• translate that using the child process's base address; and then
• do a cross-process memory write into the child process using ::WriteProcessMemory().

(Here's the start of that nonsense.)

This also doesn't touch the symbol table.

Of course, I don't know if comparable mechanisms to determine child-process base-addresses and to read/write their memory exist on macOS and Linux — for that matter, I don't even know if program images are guaranteed to be laid out contiguously there, or if ASLR applies on a per-section basis. But it may be worth considering.

(Or perhaps we should instead consider using the mechanism you describe here in the launcher process...)

Comment 3

[Ray Kraesig [:rkraesig]]

(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

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 5

[Gabriele Svelto [:gsvelto]]

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.

Comment 6

[Gabriele Svelto [:gsvelto]]

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.

Comment 7

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1776197 - mozannotation_client implementation r=afranchuk

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.

Comment 8

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1776197 - mozannotation_server crate implementation r=afranchuk

This crate contains the logic used to find the annotations within another
process and pull them out.

Depends on D173697

Comment 9

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1776197 - Adjust crash generation to pull annotations out of child processes using the mozannotation crates r=afranchuk

Depends on D173698

Comment 10

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1776197 - Remove the old IPC channel used for retrieving annotations and all related machinery r=afranchuk

Depends on D173699

Comment 11

[Gabriele Svelto [:gsvelto]]

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.

Comment 12

[Gabriele Svelto [:gsvelto]]

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.

Comment 13

[Gabriele Svelto [:gsvelto]]

Updating the update: I'm unstuck, yay! Time for the Windows and macOS implementations.

Comment 14

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1776197 - Tweak GeckoView tests to check for child process annotations r=owlish

Depends on D173700

Comment 16

[Gabriele Svelto [:gsvelto]]

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.

Comment 17

[Gabriele Svelto [:gsvelto]]

Ugh, silly issue in the previous try run, new one is here: https://treeherder.mozilla.org/jobs?repo=try&revision=d0f5bf1b6e508c3b469d643090d73691f64279b3

Comment 18

[Gabriele Svelto [:gsvelto]]

Try is green(ish), patches are rebased, manual tests look good. Alright chums, let's do this!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a45d357f36c6
mozannotation_client implementation r=afranchuk
https://hg.mozilla.org/integration/autoland/rev/5ba9e96d7820
mozannotation_server crate implementation r=afranchuk
https://hg.mozilla.org/integration/autoland/rev/eec04732422a
Adjust crash generation to pull annotations out of child processes using the mozannotation crates r=afranchuk
https://hg.mozilla.org/integration/autoland/rev/b665cbdccb30
Remove the old IPC channel used for retrieving annotations and all related machinery r=geckoview-reviewers,afranchuk,owlish
https://hg.mozilla.org/integration/autoland/rev/e88255995477
Tweak GeckoView tests to check for child process annotations r=owlish

Comment 20

[Natalia Csoregi [:nataliaCs]]

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