Closed Bug 1518922 Opened 11 months ago Closed 11 months ago

simplify setting up a tmpdir for crashreporting


(Core :: IPC, enhancement)

Not set



Tracking Status
firefox66 --- fixed


(Reporter: froydnj, Assigned: froydnj)


(Blocks 1 open bug)



(2 files)

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


and code for receiving it:

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.

Summary: simply setting up a tmpdir for crashreporting → simplify setting up a tmpdir for crashreporting

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?

Actually, you know what? We setup crashReportTmpDir:

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

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

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

Flags: needinfo?(agaynor)

Sure, I can review.

Flags: needinfo?(agaynor)
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
Attachment #9035686 - Flags: review?(agaynor)
After part 1, this function and the code associated with it is no longer

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.
Attachment #9035688 - Flags: review?(gsvelto)
Attachment #9035686 - Flags: review?(agaynor) → review+

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

Thanks for cleaning this up!

Attachment #9035688 - Flags: review?(gsvelto) → review+
Pushed by
part 1 - remove dead code for extra crashreporting directory; r=Alex_Gaynor
part 2 - remove GetChildProcessTmpDir; r=gsvelto
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Assignee: nobody → nfroyd
You need to log in before you can comment on or make changes to this bug.