514188 - sync to breakpad revision 463 to pick up Linux client rewrite

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[(not currently active) Ted Mielczarek]

In bug 510920 josh was trying to sync up to a newer breakpad to pick up some OS X fixes. However, in Breakpad revision 384, a rewrite of the Linux client code was landed, so taking that code from upstream requires some work. I got some of it working, but it didn't seem worthwhile to block the OS X fixes on that, so I'm spinning that off into a separate bug. I think the Linux client rewrite probably has some things we want, and it's generally good practice to sync up with upstream every once in a while anyway.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

One of the advantage of the linux client rewrite, is that it seems to get the crash address correct. i.e. instead of reporting the eip for SIGSEGV it reports the faulting memory address.

Comment 2

[(not currently active) Ted Mielczarek]

I'm going to fix this before I get started on bug 525849, doesn't make sense to try to use the (now replaced) Linux Breakpad code as a starting point.

Comment 3

[(not currently active) Ted Mielczarek]

Just to document the steps I'm using, since I never remember to do it and always waste a bunch of time.
Starting from a clean mozilla-central tree:
rm -rf toolkit/crashreporter/google-breakpad
svn export --ignore-externals -r 434 ../google-breakpad ./toolkit/crashreporter/google-breakpad
hg st -un | grep "Makefile$" | xargs rm # to remove Breakpad Makefiles that we aren't using
hg st -n | grep "Makefile\.in$" | xargs hg revert --no-backup # restore our Makefile.ins
hg addremove toolkit/crashreporter/google-breakpad/

(I should put this on a wiki somewhere.)

Comment 4

[(not currently active) Ted Mielczarek]

Here's what the Breakpad changes look like, I'm not going to bother trying to attach them here:
http://hg.mozilla.org/try/rev/e3b777e00767

Comment 5

[(not currently active) Ted Mielczarek]

It looks like we'll get 64-bit Linux client support out of this as well.

Comment 6

[(not currently active) Ted Mielczarek]

The exception handler is busted for us, we crash in its signal handler. My unittests pass, so I suspect this is due to us having a chained signal handler.

Comment 7

[(not currently active) Ted Mielczarek]

Attachment: fix nsProfileLock to use a SA_SIGINFO style signal handler

This fixes nsProfileLock so it chains to Breakpad's signal handler properly.

Comment 8

[(not currently active) Ted Mielczarek]

Attachment: build system changes

These are the build system changes needed to build with the latest Breakpad SVN (r437).

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 416285 [details] [diff] [review]
build system changes

I'm assuming that this review also applies to the Breakpad patch that I didn't attach here (but linked to above).

Comment 10

[Benjamin Smedberg]

I didn't review it: it's a straight import of the project code, right? (In which case review isn't necessary).

Comment 11

[(not currently active) Ted Mielczarek]

Yes.

Comment 12

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/2138d22ff018
http://hg.mozilla.org/mozilla-central/rev/09e8d73fda8b

Comment 13

[(not currently active) Ted Mielczarek]

Oops, this also got pushed:
http://hg.mozilla.org/mozilla-central/rev/4354c4d85277

Comment 14

[Bill Gianopoulos [:WG9s]]

OK. I am assuming it is way too late to get this into Firefox 3.6.  However, I think this needs to be in 3.6.1.  I suspect many Linux users have AMD64 processors.

Comment 15

[(not currently active) Ted Mielczarek]

This isn't going to land on the branch. It's too much churn.

Comment 16

[Bill Gianopoulos [:WG9s]]

<soapbox>So yet another reason I will never run a 3.6.x version and recommend other just stay at 3.5 until 3.7 or whatever is next appears because 3.6 seems to be  no useful new features or fixes release.</soapbox>

Comment 17

[(not currently active) Ted Mielczarek]

Please leave content-free advocacy out of bugzilla. Thanks.

Comment 18

[Bill Gianopoulos [:WG9s]]

Well it is interesting in learning that you have zero interest in supporting Linux users.

Comment 19

[(not currently active) Ted Mielczarek]

All of our Linux users use distro builds anyway. But this bug is fixed.

Comment 20

[Bill Gianopoulos [:WG9s]]

Hmm.I am a linux user.  I don't use a distro build.  So, it would seem your previous assertion is incorrect.

But to stop this stupid argument, I realize this is a big change and that is why I said for 3.6 it was not a good idea and suggested it for 3.6.1.

And perhaps for 3.6.1 or even .2 it is not ready.  If you think all Linux users use distro builds, then since they all have crashreporter disabled you will never get any information about Linux only crashes.

The point is that those Linux users who do NOT use distro builds, are the same class of user who most likely have an AMD64 processor and have not been able to give useful crash information since we stopped using talkbalk (which worked just fine by the way) and replaced it with breakpad/crashreporter which had this issue in that it does not work at all with an AMD64 processor.

So, the point is do we really want to go through the entire 3.6 cycle with only getting useful crash reports from people using Intel processors (which I contend is the minority of Linux users)?

Comment 21

[Matěj Cepl]

(In reply to comment #20)
> If you think all Linux users
> use distro builds, then since they all have crashreporter disabled you will
> never get any information about Linux only crashes.

There are attempts (see bug 447771) to allow distribution builds to report crashes to crash-stats.mozilla.org

Comment 22

[(not currently active) Ted Mielczarek]

I backed the update patches out due to bug 535071:
http://hg.mozilla.org/mozilla-central/rev/5694f93345f7
http://hg.mozilla.org/mozilla-central/rev/088fc4c3f9df
http://hg.mozilla.org/mozilla-central/rev/35df81a48484
http://hg.mozilla.org/mozilla-central/rev/bd072f4c6a78

(I left the signal handler patch in.)

Comment 23

[(not currently active) Ted Mielczarek]

I have a patch up for review upstream to fix the bustage we saw:
http://breakpad.appspot.com/49008

Comment 24

[(not currently active) Ted Mielczarek]

(In reply to comment #3)
> (I should put this on a wiki somewhere.)

I did, so I don't forget next time:
https://wiki.mozilla.org/Breakpad/Updating_to_latest_Breakpad_from_SVN

Comment 25

[(not currently active) Ted Mielczarek]

I backed out the nsProfileLock change, I think without the Breakpad update it actually broke things worse:
http://hg.mozilla.org/mozilla-central/rev/d303e54445e7
http://hg.mozilla.org/mozilla-central/rev/a3dea6c607dd

Comment 26

[(not currently active) Ted Mielczarek]

Ok, fixed the upstream issue:
http://code.google.com/p/google-breakpad/source/detail?r=461

I'll get my patches refreshed and relanded ASAP.

Comment 27

[(not currently active) Ted Mielczarek]

Attachment: build system changes (to r461)

A few changes to accommodate upstream changes since the previous patch. Interdiff should show it's not a lot.

Comment 28

[(not currently active) Ted Mielczarek]

Actually synced to 463, I misled myself somewhere.

bsmedberg said to carry his review forward since there aren't any major changes.

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/3975716a6eb4 (nsProfileLock changes)
http://hg.mozilla.org/mozilla-central/rev/5e311ec4e775 (breakpad sync)
http://hg.mozilla.org/mozilla-central/rev/defc32dee2f8 (build system changes)

Comment 29

[Will Roberts [:bws42]]

Does this mean that you'll be enabling the crash reporter on the trunk x86_64 Linux nightlies soonish?

Comment 30

[(not currently active) Ted Mielczarek]

I'm not sure. It works there, but we don't have the ability to walk the stack on the server side yet. That will depend on bug 464750, which is in progress.

Comment 31

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/d80f756ee858

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 33

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430