1518922 - simplify setting up a tmpdir for crashreporting

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Description

[Nathan Froyd [:froydnj]]

We have this code for setting up a temporary directory for crash reporting:

(Unix) https://searchfox.org/mozilla-central/rev/5053031ba7621fa8f63f42de4c204ab3561e4e59/ipc/glue/GeckoChildProcessHost.cpp#730-743
(Windows) https://searchfox.org/mozilla-central/rev/5053031ba7621fa8f63f42de4c204ab3561e4e59/ipc/glue/GeckoChildProcessHost.cpp#1015-1028

and code for receiving it:

https://searchfox.org/mozilla-central/rev/5053031ba7621fa8f63f42de4c204ab3561e4e59/toolkit/xre/nsEmbedFunctions.cpp#597-611

Notice the duplicated conditions, and the failure to correctly check for the VR process in all cases. (And the flat-out duplicated logic in the Unix and Windows cases, sigh.)

We should simplify this, whether with a GECKO_PROCESS_TYPE parameter that indicates whether the process needs a temporary directory for crash reporting (the content process doesn't because...sandboxing hasn't locked it down yet?) or some other scheme that I haven't thought of.

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

Hmm, I suspect I was the last person to touch how content processes do crash reporting, but I don't remember there being a directory.

A year and change ago, the content process used to create a new file on crash, write some stuff to it, and then send the filename up to the parent. I changed it so that at process startup, the child inherits an fd from the parent, and writes to that. None of the code I wrote was specific to content processes, but it's possible the methods I was changing were already only for content processes. The goal of all of this was to remove the need for content processes to ever need to create a new file, which was achieved (at least on macOS).

What are other processes needing a whole directory for? Seems like there's room for rationalizing content processes vs. everything else?

Comment 2

[Nathan Froyd [:froydnj]]

Actually, you know what? We setup crashReportTmpDir:

https://searchfox.org/mozilla-central/rev/5053031ba7621fa8f63f42de4c204ab3561e4e59/toolkit/xre/nsEmbedFunctions.cpp#597-611

and then we never use it again. So we should just remove all this code as dead, then?

Comment 3

[Nathan Froyd [:froydnj]]

Ah, it looks like Alex removed the use of crashReportTmpDir in bug 1407693:

https://hg.mozilla.org/mozilla-central/rev/bcc0a91dd43f#l13.112

So yeah, I think this code is dead, and we can ditch all of it. Alex, do you feel comfortable reviewing that code deletion?

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

Sure, I can review.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 1 - remove dead code for extra crashreporting directory; r=Alex_Gaynor

The command-line parameter used by nsEmbedFunctions.cpp is turned into
an nsIFile, and then said nsIFile is never used.  Its last use was
deleted in bug 1407693, where we reworked how extra annotations were
done.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: part 2 - remove GetChildProcessTmpDir

After part 1, this function and the code associated with it is no longer
used.

The bug 1257098 reference makes me a little nervous, as that bug isn't fixed
yet (maybe it no longer applies?), but given that I can't see exactly what
difference this code makes in that case...I'll be doing a try run with these
patches shortly.

Comment 7

[Gabriele Svelto [:gsvelto]]

Comment on attachment 9035688 [details] [diff] [review]
part 2 - remove GetChildProcessTmpDir

Thanks for cleaning this up!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef4c81995f6
part 1 - remove dead code for extra crashreporting directory; r=Alex_Gaynor
https://hg.mozilla.org/integration/mozilla-inbound/rev/20e70e44d305
part 2 - remove GetChildProcessTmpDir; r=gsvelto

Comment 9

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/4ef4c81995f6
https://hg.mozilla.org/mozilla-central/rev/20e70e44d305