397200 - OS X dylibs have non-unique signatures causing incorrect stack traces

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fun fun. Turns out that the signature algorithm in breakpad on OS X uses the dylib name, the version, and the compatibility version. Most of the dylibs produced for Firefox and XULRunner don't have either version or compatibility version information included in them (or, if they do, the breakpad algorithms are failing to extract them). This causes the XUL library, for instance, to have this signature *every time* it gets hashed:

  XUL  4C5558000001000000010000000000070

Not so great! There are lots of other dylibs that suffer this same problem, take a look at the modules section of any crash report on OSX (like bp-4e41d9c2-6829-11dc-bfe1-001a4bd46e84 for instance).

I'd suggest we get this fixed quickly because crash reports submitted with older versions of libraries will produce incorrect stack traces when processed alongside a newer version of the symbol file. Ted has already verified that the crash server only contains *two* symbol files for XUL, one for intel and one for ppc. Everything else that came before has been overwritten because the filenames are identical.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Here's the breakpad signature algorithm on OS X:

http://mxr.mozilla.org/mozilla/source/toolkit/crashreporter/google-breakpad/src/common/mac/macho_id.cc#163

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The structure it's using also has a timestamp member. I think we should definitely include that in the hash.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ok, so, looking a little deeper I found that the breakpad algorithm is working properly, it's just that we are always setting the version and compat version of our non-component dylibs to 1. I still don't like the way that the signature isn't really a hash at all... Changing the filename results in a different signature, for instance (which makes xulrunner-stub a bit of a pain). So now we have to figure out what to do with that version number.

Maintaining a version text file for each dylib seems like a huge headache. Doing something based on build ID (timestamp) also seems like a bad idea because the code may not have changed.

As ugly as it sounds I think patching breakpad to always use MD5 for the signature might be the best way forward here. That's currently what the linux implementation does, and the code is all there and works properly (that's what our component dylibs use, somehow the -bundle flag keeps the LC_ID_DYLIB command out of the macho header), so it would be a simple matter of commenting out four lines of code here:

http://mxr.mozilla.org/mozilla/source/toolkit/crashreporter/google-breakpad/src/common/mac/file_id.cc#73

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch to use MD5

Per discussion on IRC we're going to patch this locally until google updates their code with the exact same fix.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 282149 [details] [diff] [review]
Patch to use MD5

r=me

Comment 6

[Mark Mentovai]

Comment on attachment 282149 [details] [diff] [review]
Patch to use MD5

We should be ready to update this in our svn copy in a week or two.  Poke me if you need to.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 282149 [details] [diff] [review]
Patch to use MD5

This is causing us to have unreliable stack traces on OS X. We should get this in ASAP.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Need to ping mento about getting this upstream.

Comment 10

[(not currently active) Ted Mielczarek]

For future reference, this did eventually get upstreamed:
http://code.google.com/p/google-breakpad/source/detail?r=313