1547698 - Refactor and clean up the code used for writing out .extra files

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, task)

Description

[Gabriele Svelto [:gsvelto]]

In bug 1420363 we'd like to use JSON for the files holding the crash annotations instead of the current INI-like format. Before we do so however the code that writes out the files needs to be cleaned up and modularized:

• Historically the .extra file for a content process was created by the content process itself and then handed over to the main process for processing upon a crash. This behavior was changed in bug 1407693 so now all the annotations are sent through a pipe. However leftovers from the original approach are still present.

• Before content processes were a thing we had a single set of functions for writing out .extra files, when child processes were added we started bolting on bits and pieces to these functions to accommodate for plugin and then content processes. The result is an unreadable mess where the behavior of a certain function (e.g. WriteExtraData()) depends on its parameters, the state of the file as well as some internal state making it impossible to understand what's going on. These should be separated and cleaned up.

• Last but not least we have different platform-specific function for writing out data that could be easily replaced with better (already existing) abstractions to cut down on the #ifdef'ing.

Comment 1

[Gabriele Svelto [:gsvelto]]

Here's an outline of what I'm doing:

• For content processes instead of writing crash annotations to the .extra file right away I keep them in an annotation table, merge all the incoming ones (exception-time annotations, overrides in CrashReporterHost, etc...) then write them out in the .extra file at once. This will make switching the .extra file format easily.

• For main process crashes I'm refactoring the existing code so that everything that can be shared with content processes will be. This means cutting down on the baroque calls to WriteExtraData() and consolidating everything into a single safe function that is safe to call from within the exception handler. Again this should allow changing the .extra file format in only one place. Note that this will also be used for the event file since the event file also contains the crash annotations. Having just one function for both is going to cut the relevant code in half (at last).

• Since I'm at it I'm chopping away old stuff (e.g. unused code related to handling pid-suffixed .extra files) and cleaning up TODOs that could have been fixed ages ago (like removing XRE_TakeMinidumpForChild() from nsXULAppAPI.h).

Comment 2

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1547698 - Remove unused IPC methods for taking minidumps

This removes the XRE_TakeMinidumpForChild() which does not need to be
exposed anymore in the XUL API as well as
IToplevelProtocol::TakeMinidump() which was simply unused.

Comment 3

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1547698 - Remove unused and non-public bits from the exception handler

Comment 4

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1547698 - Refactor the code that writes the .extra file for a content process crash or hang

Upon a content process crash or hang crash annotations were incrementally
written into the .extra file starting with the exception handler callback and
then in a number of different places before the file was ready for submission.
This had a number of downsides: since the annotations were directly added to
the file it was impossible to tell which ones were already written at a
certain point in time, additionally some were written twice or even thrice.
The code doing the writing would also behave differently depending on the
contents of the file, the parameters passed to it and the contents of global
variables.

This change overhauls the whole process by keeping the annotations into a
temporary per-crash annotation table which is filled with all the required
annotations before being written out in a single pass when they are ready.

The annotations are gathered from the main process annotation table, the
per-process one (held by the CrashReporterHost) and exception-time specific
ones.

The resulting annotations are slightly different than before the patch: first
of all there are no more duplicate entries in the .extra file and secondly all
content/plugin process hangs annotations are properly filtered, before
annotations that were main process-only would leak into them.

Comment 6

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1547698 - Reorganize the code used to write the annotations upon a main process crash

Annotation on main process crashes are written to both the .extra file
(for crash submission) and to the event file so that the browser can
detect the crash when restarting even if the crash report files have
been deleted.

This patch factorizes all the code that writes to both files, cutting
down all the duplicate calls, and fixes an issue with the
BreakpadReserveAddress and BreakpadReserveSize annotations which were
not written to the event file.

Comment 7

[Gabriele Svelto [:gsvelto]]

All the patches are in, this should make the code a bit more readable and makes it possible to convert the .extra file to JSON.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef9124d5d07f
Remove unused IPC methods for taking minidumps r=froydnj
https://hg.mozilla.org/integration/autoland/rev/9497f265bed4
Remove unused and non-public bits from the exception handler r=froydnj
https://hg.mozilla.org/integration/autoland/rev/0b3558c3da85
Refactor the code that writes the .extra file for a content process crash or hang r=froydnj
https://hg.mozilla.org/integration/autoland/rev/e013f1f17109
Reorganize the code used to write the annotations upon a main process crash r=froydnj

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/ef9124d5d07f
https://hg.mozilla.org/mozilla-central/rev/9497f265bed4
https://hg.mozilla.org/mozilla-central/rev/0b3558c3da85
https://hg.mozilla.org/mozilla-central/rev/e013f1f17109