Closed Bug 1518922 Opened 11 months ago Closed 11 months ago

simplify setting up a tmpdir for crashreporting

Categories

(Core :: IPC, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.

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:

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?

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?

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
done.
Attachment #9035686 - Flags: review?(agaynor)
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.
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 nfroyd@mozilla.com:
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
Status: NEW → RESOLVED
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.