Closed Bug 510505 Opened 15 years ago Closed 12 years ago

add inexpensive skidmarks to crash report

Categories

(Toolkit :: Crash Reporting, defect)

x86
Windows NT
defect
Not set
normal

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: shaver, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

For some of our hard-to-reproduce bugs, it would be helpful if we could (v. cheaply) store a skidmark or 4 before performing some kinds of operation, so that we could know a little more about the context than is available from stack and register data.

As an opening bid, 4 pointer-width integers that can be included in the crash report data on the socorro side, where they should be displayed in decimal, in hex, and as a symbol+offset.

NS_SET_CRASH_SKIDMARK1(thing->type);
NS_SET_CRASH_SKIDMARK4(&this->MethodImTotallyGoingToCrashIn);
In order to test this proposed functionality, I needed tests that actually tested running our exception handler code. This xpcshell test code provides a convenience function in head_crashreporter.js called do_crash(), which runs an xpcshell subprocess, executes some setup code that you provide in that process, and then crashes the process. do_crash then returns the minidump file and the data from the .extra file.
Assignee: nobody → ted.mielczarek
Status: NEW → ASSIGNED
This test works on Mac/Linux but is currently failing on Windows with NS_ERROR_FILE_EXECUTION_FAILED. Need to figure that out.
Ah, our exception handler exits the process with return code -1 on Windows, which causes nsIProcess.run to throw. Works fine if I wrap that with a try/catch.
This works on Windows as well.
Attachment #399488 - Attachment is obsolete: true
Attachment #399809 - Flags: review?(benjamin)
Oops, the way I had do_crash written previously was prone to random failures. It was cleaning up the minidump file in a cleanup function, but that means that the minidump would still be there if you ran do_crash again, so the test would fail if you got unlucky and the new minidump name sorted after the old one. Reworked do_crash to take a callback instead of returning the values, so you get to run your tests in the callback then it cleans up before returning.
Attachment #399809 - Attachment is obsolete: true
Attachment #400154 - Flags: review?(benjamin)
Attachment #399809 - Flags: review?(benjamin)
Attachment #400154 - Flags: review?(benjamin) → review+
This implements the skidmark functionality. I've exposed it via the nsICrashReporter interface, and have unit tests that test it via that path, but I haven't done anything with it from C++ yet. The C++ bit I added to the IDL file is a stab at what shaver proposed, but I don't have a candidate test site to use it at, so I'm not sure if it will work yet. Linking could be a little weird because the crashreporter code is all in toolkit, but I don't want the C++ version to go through XPCOM if possible, since the whole point is to make it low-overhead. It should work fine in libxul builds, and I guess it doesn't really matter for non-libxul, so maybe I can just add a #define for libxul builds and make it a no-op for non-libxul builds.
Crashreporter is linked into libxul even in non-libxul builds, so I think as long as you export the symbols you should be ok to use them from xpconnect or wherever.
Yeah, I didn't really want to export them, it didn't seem worth it. I guess there are still non-libxul apps out there like Thunderbird, so it's probably worth it.
I landed the unit test patch here because I needed it for something else:
http://hg.mozilla.org/mozilla-central/rev/b35ebf2606ed
Assignee: ted.mielczarek → nobody
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.