Closed Bug 368206 Opened 13 years ago Closed 13 years ago

integrate breakpad exception handler/crash reporter on OS X

Categories

(Toolkit :: Crash Reporting, defect)

PowerPC
macOS
defect
Not set

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: nobody → dcamp
Attached patch checkpoint (obsolete) — Splinter Review
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.
Attached patch first cut at a reporter (obsolete) — Splinter Review
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)
keyedobjects.nib is binary so didn't end up in the patch
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)
Drive by:
Why not use a .strings file and NSLocalizedString() to do the localization?

http://developer.apple.com/documentation/MacOSX/Conceptual/BPInternational/index.html
Because we'd like to use the same format that we do for win/linux
Fair enough. Looked OK otherwise, I'll leave the style stuff to mento.
(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>.
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+
Summary: integrate airbag exception handler/crash reporter on OS X → integrate breakpad exception handler/crash reporter on OS X
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?
Attachment #261502 - Flags: review? → review?(mark)
Attached patch v2 (obsolete) — Splinter Review
Attachment #261502 - Attachment is obsolete: true
Attachment #262001 - Flags: review?(mark)
Attachment #262001 - Flags: first-review+
Attachment #261502 - Flags: review?(mark)
(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
(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 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 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+
Attached patch v3 (obsolete) — Splinter Review
Attachment #262001 - Attachment is obsolete: true
Fixes for mark's comments.
+# 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.
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.
Attached patch v4 (obsolete) — Splinter Review
fixed the makefile, also fixed it for the updater I stole that snippet from.
Attachment #262282 - Attachment is obsolete: true
Attached patch v4.1 (obsolete) — Splinter Review
(oops, old version of the patch)
Attachment #262514 - Attachment is obsolete: true
Blocks: 378581
Attachment #262515 - Attachment is obsolete: true
mscott, would you mind checking this in?
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. 
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) 
I need to do a UB build to make sure this doesn't screw up unify.
Attachment #262771 - Flags: review?(ted.mielczarek)
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+
I did a UB build and it issues some warning during unify, but works. This is all checked in!
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Maybe file a new bug to fix the warnings?
Backed out the configure change since it's burning tinderboxen.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Configure test to enable breakpad on OS X.
Attachment #262771 - Attachment is obsolete: true
Attachment #263992 - Flags: review?(benjamin)
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+
Some static linking changes needed to avoid breaking Thunderbird or anything else linked statically.
Attachment #264006 - Flags: review?(benjamin)
Attachment #264006 - Flags: review?(benjamin) → review+
Checked in the last two patches.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
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.
Comment on attachment 263992 [details] [diff] [review]
enable breakpad on os x

Checked in again...
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 → ---
Fourth time's the charm?  Checked in yet again.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.