366970 - crash reporter needs to send Product, Build ID, Platform

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[(not currently active) Ted Mielczarek]

The crash reporter client needs to send the Product Name, Build ID, and Platform along with the crash report.  For XULRunner apps, these will come from application.ini.  For Firefox, I'm not sure where we can get Product Name.  Platform we can just #ifdef I suppose.

Comment 1

[(not currently active) Ted Mielczarek]

I think I should probably just grab them from the nsXULAppInfo in XRE_Main, and then pass them on the commandline to the crash reporter:
http://mxr.mozilla.org/seamonkey/source/toolkit/xre/nsXULAppAPI.h

Comment 2

[(not currently active) Ted Mielczarek]

Attachment: work in progress

This doesn't quite work, but it's close.  Something is wrong in either BuildCommandLine or AirbagMinidumpCallback.  I am doing some string copying using memcpy, so I dunno.

Comment 3

[(not currently active) Ted Mielczarek]

Attachment: cleaned up

Yeah, so, the logic worked, but apparently xul.dll wasn't being updated when I compiled due to windbg having the pdb open...

I cleaned it up, it's ready to go.

Comment 4

[(not currently active) Ted Mielczarek]

Attachment: for real

I left a debugging MessageBox in that last one...

Comment 5

[Benjamin Smedberg]

This is a passing review, because we talked and you're going to work on the generic API that saves metadata in a separate file.

You need to be careful about nsXREAppData fields. They can be null in some cases.

> const PRInt32 kGUIDLength = 36;

Please document whether this contains hyphens or not.

As discussed, I'm wary of using commandline arguments for the crash reporter.

The method of locating crashreporter.exe is not going to work for XR apps. You could fix this here, or punt for later (if you're going to punt, please file a followup bug). It should use the location of appdata->xreDirectory.

Don't use nsMemory in new code, use NS_Free/Alloc.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 259417 [details] [diff] [review]
for real

Gonna rewrite this to use the API that I was going to implement anyway...

Comment 7

[(not currently active) Ted Mielczarek]

Attachment: add an API for additional data, and use it

This patch adds an API to send additional data with the crash report.  The data gets stored in a file with the same basename as the minidump, but with a .extra extension.  The file format is key=value, one per line.  Newlines are escaped as \n in values, and backslashes are escaped as \\.  Newlines and equals signs are forbidden in keys (the method will return an error).  The crash reporter client simply reads this file in line by line, and sends a form parameter for each key, containing the value.

Comment 8

[Mark Mentovai]

Comment on attachment 259800 [details] [diff] [review]
add an API for additional data, and use it

This mostly looks good, I've just got a few comments.

+// xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
+const static PRInt32 kGUIDLength = 36;

My preference is for |static const|, but if there's precedent here or if you feel strongly, then whatever.

+// length of a GUID + .dmp" (yes, trailing double quote)
+const static PRInt32 kMinidumpFilenameLength = kGUIDLength + 5;
+const static PRUnichar dumpFileExtension[] = {'.', 'd', 'm', 'p',
+                                              '\"', '\0'}; // .dmp"

Better:

static const PRInt32 kMinidumpFilenameLength =
  kGUIDLength + sizeof(dumpFileExtension) / sizeof(dumpFileExtension[0]);

+// this is setup so we don't have to do heap allocation in the handler

"set up" is two words.

Have you checked the CRT source on Windows to make sure that memcpy is safe?  Some CRT routines are not handler-safe, in surprising cases.

>+nsresult AnnotateCrashReport(const nsACString &key, const nsACString &data)

There are a few functions functions in here now that just get tossed into the global namespace.  The old ones all have Airbag somewhere in their name (not at the beginning), but this new one doesn't.  If they can't be added to a namespace or made into static class methods, it probably makes sense to at least name everything with some common prefix.  This obviously doesn't apply to functions marked static.  (CrashRep_SetExceptionHandler, CrashRep_Annotate, etc.?)

Comment 9

[(not currently active) Ted Mielczarek]

Attachment: patch for checkin

Ok, I addressed all of Mark's comments.  The entire nsAirbagExceptionHandler file got wrapped in namespace CrashReporter {}.  I figured I'd attach this patch just for clarity.

Comment 10

[(not currently active) Ted Mielczarek]

Checked in.