Closed
Bug 368206
Opened 17 years ago
Closed 17 years ago
integrate breakpad exception handler/crash reporter on OS X
Categories
(Toolkit :: Crash Reporting, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: ted, Assigned: dcamp)
References
Details
Attachments
(4 files, 8 obsolete files)
7.47 KB,
application/octet-stream
|
Details | |
42.27 KB,
patch
|
Details | Diff | Splinter Review | |
597 bytes,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
1.52 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
Once we sync up with Airbag SVN, we need to do client side integration on OS X. We'll need to set the exception handler, execute the crash reporter on an exception, and implement a basic crash reporter app.
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → dcamp
Reporter | ||
Comment 1•17 years ago
|
||
Just some basic work to get nsAirbagExceptionHandler building on OS X, plus Makefiles for the breakpad code. Doesn't actually implement the platform-specific parts, and I didn't touch the crashreporter code. I didn't test this on Win32 to make sure I didn't break anything, but I'll do that soon.
Assignee | ||
Comment 2•17 years ago
|
||
This adds osx support to the exception handler, and adds a simple reporting app.
Attachment #260760 -
Attachment is obsolete: true
Attachment #261502 -
Flags: second-review?(joshmoz)
Attachment #261502 -
Flags: first-review?(ted.mielczarek)
Assignee | ||
Comment 3•17 years ago
|
||
keyedobjects.nib is binary so didn't end up in the patch
Reporter | ||
Comment 4•17 years ago
|
||
Comment on attachment 261502 [details] [diff] [review] first cut at a reporter I'd prefer to have mento do the Mac review, since he can take a look at your Breakpad API usage as well.
Attachment #261502 -
Flags: second-review?(joshmoz) → second-review?(mark)
Comment 5•17 years ago
|
||
Drive by: Why not use a .strings file and NSLocalizedString() to do the localization? http://developer.apple.com/documentation/MacOSX/Conceptual/BPInternational/index.html
Comment 6•17 years ago
|
||
Because we'd like to use the same format that we do for win/linux
Comment 7•17 years ago
|
||
Fair enough. Looked OK otherwise, I'll leave the style stuff to mento.
Comment 8•17 years ago
|
||
(quick first pass only) dcamp, can you zip up the whole top-level nib and post it here instead of just keyedobjects.nib? What happens to dumps if the user hits "Don't Send"? Do they accumulate? Indentation in main is inconsistent. What's errors.h doing here? These days, we usually just include <Carbon/Carbon.h> instead of <Types.h>.
Reporter | ||
Comment 9•17 years ago
|
||
Comment on attachment 261502 [details] [diff] [review] first cut at a reporter >diff -r 3299b845a7da toolkit/airbag/client/Makefile.in >--- a/toolkit/airbag/client/Makefile.in Fri Apr 13 13:02:19 2007 -0400 >+++ b/toolkit/airbag/client/Makefile.in Fri Apr 13 14:44:07 >+ifeq ($(OS_ARCH),Darwin) >+CMMSRCS += crashreporter_osx.mm >+OS_LIBS += -framework Cocoa >+LIBS += $(DEPTH)/toolkit/airbag/airbag/src/client/mac/handler/$(LIB_PREFIX)exception_handler_s.$(LIB_SUFFIX) >+LIBS += $(DEPTH)/toolkit/airbag/airbag/src/common/mac/$(LIB_PREFIX)breakpad_mac_common_s.$(LIB_SUFFIX) >+LOCAL_INCLUDES += -I$(srcdir) -I$(srcdir)/../airbag/src/common/mac/ >+endif I'd prefer the LIBS lines to be written like: LIBS += \ $(DEPTH)/... \ $(DEPTH)/... \ $(NULL) >+ >+ifeq ($(OS_ARCH),Darwin) >+libs:: >+ mkdir -p $(DIST)/bin/crashreporter.app >+ rsync -a --exclude CVS --exclude "*.in" $(srcdir)/macbuild/Contents $(DIST)/bin/crashreporter.app >+ sed -e "s/@APP_NAME@/$(MOZ_APP_DISPLAYNAME)/" $(srcdir)/macbuild/Contents/Resources/English.lproj/InfoPlist.strings.in | \ >+ iconv -f UTF-8 -t UTF-16 > $(DIST)/bin/crashreporter.app/Contents/Resources/English.lproj/InfoPlist.strings >+ mkdir -p $(DIST)/bin/crashreporter.app/Contents/MacOS >+# copy and remove to un-symlink (FSCopyObject seems to not copy symlinks) >+ rm -f $(DIST)/bin/crashreporter.app/Contents/MacOS/crashreporter >+ cp -f $(DIST)/bin/crashreporter $(DIST)/bin/crashreporter.app/Contents/MacOS >+ rm -f $(DIST)/bin/crashreporter >+ cp -f $(DIST)/bin/crashreporter.ini $(DIST)/bin/crashreporter.app/Contents/MacOS >+ rm -f $(DIST)/bin/crashreporter.ini >+endif This is really ugly. We have to do all this just to build an app bundle? If so, that sucks. >diff -r 3299b845a7da toolkit/airbag/client/errors.h Are you using this file? I didn't see it included anywhere. >diff -r 3299b845a7da toolkit/airbag/nsAirbagExceptionHandler.cpp >+#ifdef XP_WIN32 >+typedef wchar_t XP_CHAR; >+#define ToNewXPChar(x) ToNewUnicode(x) >+#define ConvertUTF16toXPChar(x) x >+#define XPStrlen(x) wcslen(x) >+#define CRASH_REPORTER_FILENAME "crashreporter.exe" >+#define PATH_SEPARATOR "\\" >+#define XP_PATH_SEPARATOR L"\\" >+#else >+typedef char XP_CHAR; >+#define ToNewXPChar(x) ToNewUTF8String(x) >+#define ConvertUTF16toXPChar(x) NS_ConvertUTF16toUTF8(x) >+#define XPStrlen(x) strlen(x) >+#define CRASH_REPORTER_FILENAME "crashreporter" > #define PATH_SEPARATOR "/" >+#define XP_PATH_SEPARATOR "/" >+#endif // XP_WIN32 Despite the fact that this code is from my patch, I've since been reminded that convention dictates that macros be named in all uppercase, so you should convert these from mixed case. +static XP_CHAR *crashReporterPath; Nit: XP_CHAR* is the style in this file. (* snug to the type name) >+static void >+Concat(XP_CHAR *str, int size, const XP_CHAR *toAppend) Here too. >+ XP_CHAR minidumpPath[4096] = {'\0',}; I'm not really keen on having a fixed size buffer here, regardless of how big it is. But you do a length check in Concat, so it's not a deal-breaker. >+#ifdef XP_MACOSX >+static nsresult GetExecutablePath(nsString& exePath) >+{ ... >+} >+ >+#else > > static nsresult GetExecutablePath(nsString& exePath) > { > #ifdef XP_WIN32 ... > } >+ >+#endif Can you fix the #ifdefing here? Either make the #ifdefs surround completely separate copies of the function for each platform, or put them around the body of the function. If you have to, change the #ifdef XP_MACOSX to #if defined(XP_MACOSX) so you can do #elif like at the top of the file. >diff -r 3299b845a7da toolkit/airbag/test/Makefile.in >+LIBS += $(DEPTH)/toolkit/airbag/airbag/src/client/mac/handler/$(LIB_PREFIX)exception_handler_s.$(LIB_SUFFIX) >+LIBS += $(DEPTH)/toolkit/airbag/airbag/src/common/mac/$(LIB_PREFIX)breakpad_mac_common_s.$(LIB_SUFFIX) Same thing here as in the previous Makefile, split this out to one assignment with multiple lines. r=me with those changes.
Attachment #261502 -
Flags: first-review?(ted.mielczarek) → first-review+
Reporter | ||
Updated•17 years ago
|
Summary: integrate airbag exception handler/crash reporter on OS X → integrate breakpad exception handler/crash reporter on OS X
Comment 10•17 years ago
|
||
dcamp, can you zip up the whole nib bundle directory and not just keyedobjects.nib per comment 8?
Attachment #261502 -
Flags: second-review?(mark) → review?
Reporter | ||
Updated•17 years ago
|
Attachment #261502 -
Flags: review? → review?(mark)
Assignee | ||
Comment 11•17 years ago
|
||
Attachment #261502 -
Attachment is obsolete: true
Attachment #262001 -
Flags: review?(mark)
Attachment #262001 -
Flags: first-review+
Attachment #261502 -
Flags: review?(mark)
Assignee | ||
Comment 12•17 years ago
|
||
Attachment #261503 -
Attachment is obsolete: true
Assignee | ||
Comment 13•17 years ago
|
||
(In reply to comment #9) > >+ > >+ifeq ($(OS_ARCH),Darwin) > >+libs:: > >+ mkdir -p $(DIST)/bin/crashreporter.app > >+ rsync -a --exclude CVS --exclude "*.in" $(srcdir)/macbuild/Contents $(DIST)/bin/crashreporter.app > >+ sed -e "s/@APP_NAME@/$(MOZ_APP_DISPLAYNAME)/" $(srcdir)/macbuild/Contents/Resources/English.lproj/InfoPlist.strings.in | \ > >+ iconv -f UTF-8 -t UTF-16 > $(DIST)/bin/crashreporter.app/Contents/Resources/English.lproj/InfoPlist.strings > >+ mkdir -p $(DIST)/bin/crashreporter.app/Contents/MacOS > >+# copy and remove to un-symlink (FSCopyObject seems to not copy symlinks) > >+ rm -f $(DIST)/bin/crashreporter.app/Contents/MacOS/crashreporter > >+ cp -f $(DIST)/bin/crashreporter $(DIST)/bin/crashreporter.app/Contents/MacOS > >+ rm -f $(DIST)/bin/crashreporter > >+ cp -f $(DIST)/bin/crashreporter.ini $(DIST)/bin/crashreporter.app/Contents/MacOS > >+ rm -f $(DIST)/bin/crashreporter.ini > >+endif > > This is really ugly. We have to do all this just to build an app bundle? If > so, that sucks. I pulled that from the updater makefile. It is ugly... > > >diff -r 3299b845a7da toolkit/airbag/client/errors.h > > Are you using this file? I didn't see it included anywhere. Removed > Despite the fact that this code is from my patch, I've since been reminded that > convention dictates that macros be named in all uppercase, so you should > convert these from mixed case. Done. > +static XP_CHAR *crashReporterPath; > > Nit: XP_CHAR* is the style in this file. (* snug to the type name) Done. > >+ XP_CHAR minidumpPath[4096] = {'\0',}; > > I'm not really keen on having a fixed size buffer here, regardless of how big > it is. But you do a length check in Concat, so it's not a deal-breaker. I'm never a big fan of fixed size buffers, but it seems safer on the whole than trying to use a heap-allocated buffer in the exception callback? > Can you fix the #ifdefing here? Either make the #ifdefs surround completely > separate copies of the function for each platform, or put them around the body > of the function. If you have to, change the #ifdef XP_MACOSX to #if > defined(XP_MACOSX) so you can do #elif like at the top of the file. Done. > Same thing here as in the previous Makefile, split this out to one assignment > with multiple lines. Done
Assignee | ||
Comment 14•17 years ago
|
||
(In reply to comment #8) > (quick first pass only) > > dcamp, can you zip up the whole top-level nib and post it here instead of just > keyedobjects.nib? Done > > What happens to dumps if the user hits "Don't Send"? Do they accumulate? Changed that to only accumulate dumps if they user Tried to send and failed, to be consistent with the win32 version (which doesn't have a don't send option). Future reporter versions should probably be able to resubmit those. > Indentation in main is inconsistent. Fixed. > What's errors.h doing here? Removed. > These days, we usually just include <Carbon/Carbon.h> instead of <Types.h>. Fixed.
Comment 15•17 years ago
|
||
Comment on attachment 262003 [details]
MainMenu.nib, belongs in toolkit/airbag/client/macbuild/Contents/Resources/English.lproj
Should we get a default button in ConfirmWindow?
Comment 16•17 years ago
|
||
Comment on attachment 262001 [details] [diff] [review] v2 + rsync -a --exclude CVS --exclude "*.in" $(srcdir)/macbuild/Contents $(DIST)/bin/crashreporter.app Use -C instead of --exclude CVS. + char path[PATH_MAX + 1]; + OSStatus status = FSRefMakePath(&fsRef, (UInt8*)path, PATH_MAX); + int len = strlen(path); + path[len] = '/'; + path[len + 1] = '\0'; + + if (status != noErr) + return NS_ERROR_FAILURE; Check status before writing to *path. +static void +Concat(XP_CHAR* str, int size, const XP_CHAR* toAppend) Useful concatenators often return a pointer to the end of the string they modified, so that successive concats don't need to start looking from the beginning of the string - you'd want to make size an in-out param to do that too, though. + XP_CHAR minidumpPath[4096] = "\0"; Are 4096 and 8192 magic? Should they be constants or macros? + O_WRONLY | O_CREAT | O_TRUNC, + 0666); I'd prefer 0644 and O_EXCL, but this matches what Breakpad does now (didn't catch it then, I guess). And on Windows, we'd need to change to CREATE_ALWAYS to CREATE_NEW. So just leave this as-is for consistency, I guess. + // not much we can do in c ase of error Check comment. + (void) execv(crashReporterPath, argv); execl might be easier here. + } else { + return succeeded; + } +#endif + return succeeded; } You can either share that last return or move it somewhere else so that it's not disembodied from the only code that uses it. Looks good otherwise.
Attachment #262001 -
Flags: review?(mark) → review+
Assignee | ||
Comment 17•17 years ago
|
||
Attachment #262001 -
Attachment is obsolete: true
Assignee | ||
Comment 18•17 years ago
|
||
Fixes for mark's comments.
Comment 19•17 years ago
|
||
+# copy and remove to un-symlink (FSCopyObject seems to not copy symlinks) Putting that in the middle of a sequence of make commands isn't good Makefile syntax, some makes will fail there. There's an $(INSTALL) invocation you can use instead of this rm/cp stuff.
Reporter | ||
Comment 20•17 years ago
|
||
Dave, fix Mark's last comment, and then I think we should land this. We'll leave it not building by default (in configure) until we resolve the SDK issues that keep it from compiling with the 10.3 SDK.
Assignee | ||
Comment 21•17 years ago
|
||
fixed the makefile, also fixed it for the updater I stole that snippet from.
Attachment #262282 -
Attachment is obsolete: true
Assignee | ||
Comment 22•17 years ago
|
||
(oops, old version of the patch)
Attachment #262514 -
Attachment is obsolete: true
Assignee | ||
Comment 23•17 years ago
|
||
Attachment #262515 -
Attachment is obsolete: true
Assignee | ||
Comment 24•17 years ago
|
||
mscott, would you mind checking this in?
Comment 25•17 years ago
|
||
Hey Dave, you should be able to get anyone with CVS access to check this in as long as it has review from a toolkit peer or owner. If you need me to, I can start up a firefox tree and port the patch.
Comment 26•17 years ago
|
||
When crashing Firefox with breakpad enabled: 2007-04-25 09:24:43.389 crashreporter[29141] CFLog (0): CFPropertyListCreateFromXMLData(): plist parse failed; the data is not proper UTF-8. The file name for this data could be: /builds/clean-trunk/ff-debug/dist/MinefieldDebug.app/Contents/MacOS/crashreporter.app/Contents/Resources/English.lproj/MainMenu.nib The parser will retry as in 10.2, but the problem should be corrected in the plist. 2007-04-25 09:24:43.393 crashreporter[29141] An uncaught exception was raised 2007-04-25 09:24:43.393 crashreporter[29141] *** -[NSKeyedUnarchiver initForReadingWithData:]: incomprehensible archive (0x50, 0x4b, 0x3, 0x4, 0xa, 0x0, 0x0, 0x0) 2007-04-25 09:24:43.393 crashreporter[29141] *** Uncaught exception: <NSInvalidArgumentException> *** -[NSKeyedUnarchiver initForReadingWithData:]: incomprehensible archive (0x50, 0x4b, 0x3, 0x4, 0xa, 0x0, 0x0, 0x0)
Comment 27•17 years ago
|
||
I need to do a UB build to make sure this doesn't screw up unify.
Attachment #262771 -
Flags: review?(ted.mielczarek)
Reporter | ||
Comment 28•17 years ago
|
||
Comment on attachment 262771 [details] [diff] [review] Enable airbag on intel only, rev. 1 r=me if it doesn't break Universal builds.
Attachment #262771 -
Flags: review?(ted.mielczarek) → review+
Comment 29•17 years ago
|
||
I did a UB build and it issues some warning during unify, but works. This is all checked in!
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 30•17 years ago
|
||
Maybe file a new bug to fix the warnings?
Reporter | ||
Comment 31•17 years ago
|
||
Backed out the configure change since it's burning tinderboxen.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 32•17 years ago
|
||
Configure test to enable breakpad on OS X.
Attachment #262771 -
Attachment is obsolete: true
Attachment #263992 -
Flags: review?(benjamin)
Comment 33•17 years ago
|
||
Comment on attachment 263992 [details] [diff] [review] enable breakpad on os x I think the original version was more readable, but it doesn't really matter.
Attachment #263992 -
Flags: review?(benjamin) → review+
Reporter | ||
Comment 34•17 years ago
|
||
Some static linking changes needed to avoid breaking Thunderbird or anything else linked statically.
Attachment #264006 -
Flags: review?(benjamin)
Updated•17 years ago
|
Attachment #264006 -
Flags: review?(benjamin) → review+
Reporter | ||
Comment 35•17 years ago
|
||
Checked in the last two patches.
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 36•17 years ago
|
||
Comment on attachment 263992 [details] [diff] [review] enable breakpad on os x Backed this out for now since it broke on the 10.3.9 SDK. I got the patch into Breakpad SVN, and I reopened bug 379518 to get it into CVS.
Reporter | ||
Comment 37•17 years ago
|
||
Comment on attachment 263992 [details] [diff] [review] enable breakpad on os x Checked in again...
Comment 38•17 years ago
|
||
I had to back this out due to bustage. Checking in configure.in; /cvsroot/mozilla/configure.in,v <-- configure.in new revision: 1.1810; previous revision: 1.1809 done
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 39•17 years ago
|
||
Fourth time's the charm? Checked in yet again.
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•