Closed Bug 380172 Opened 17 years ago Closed 15 years ago

Breakpad integration for Camino

Categories

(Camino Graveyard :: General, defect, P1)

Product:
Camino Graveyard
Component:
General
Platform:
All
macOS
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Camino2.0

People

(Reporter: alqahira, Assigned: stuart.morgan+bugzilla)

Details

(Whiteboard: l10n)

Attachments

(3 files)

Makefile changes [landed]
2.42 KB, patch
alqahira
: review+
Details | Diff | Splinter Review
makefile, take 2 [landed]
3.34 KB, patch
alqahira
: review+
Details | Diff | Splinter Review
initial build integration
55.67 KB, patch
mikepinkerton
: superreview+
Details | Diff | Splinter Review 
We need to integrate with Breakpad; the goal is to do it for 1.6.

There are several parts to this:

1) Hooks/exception handlers or whatever
1a) Breakpad code on the branch
2) Reporter client app
3) Server stuff

mento and smorgan will tackle this; we can file separate bugs and make this a meta if we want to.
Per Sam, this is P2 for branch and P1 for trunk when we get to 2.0.
Priority: -- → P2
Setting priority per 1.6 roadmap.
Priority: P2 → P1
Setting priority per 1.6 roadmap.
Priority: P1 → P2
Nominating for a1 per the meeting.
Flags: camino1.6a1?
We agreed this is going to be in a1 (though it may not be a blocker per se) at the meeting today.
Assignee: nobody → mark
Flags: camino1.6a1? → camino1.6a1+
Not that mento needs my help, but you can probably save yourself a lot of pain by just using the code from toolkit/crashreporter.  I don't think it's really tied into any of the rest of toolkit.
We can't use the upload client part of that code; the decision to use Mozilla's one-language-per-binary localization makes it unsuitable for our use.
mento says not going to make a1. really-really-really want for b1.
Flags: camino1.6b1?
Flags: camino1.6a1-
Flags: camino1.6a1+
Pushing off :(

Retargeting for 2.0 with P1 as stated before.
Flags: camino1.6b3? → camino1.6b3-
Priority: P2 → P1
Target Milestone: Camino1.6 → Camino2.0
Version: unspecified → Trunk
We won't block b1 on this (meta or otherwise), but we'll need to block b2 on it.
Flags: camino2.0b2+
Flags: camino2.0b1?
Flags: camino2.0b1-
Assignee: mark → stuart.morgan+bugzilla
Hardware: PowerPC → All
I landed an initial pull of Breakpad at google-breakpad (we can't use the toolkit copy because it's too old to have the things we need and it isn't likely to be update on 1.9.0). I'll need to rev it at least one more time with changes I plan to make and upstream, and I may need to make slight local patches, but this is a starting place to work from.

The next step is to make the source available in objdir builds, which is what this does.

I'm doing this incrementally to make it easier to work on both shared and static so the project I land will work in both without tree breakage ;)
Attachment #371346 - Flags: review?(alqahira)
Comment on attachment 371346 [details] [diff] [review]
Makefile changes [landed]

Patch looks good; r=ardissone
Attachment #371346 - Flags: review?(alqahira) → review+
Attachment #371346 - Attachment description: Makefile changes → Makefile changes [landed]
I realized that we are going to need to build at least dump_syms, and likely other tools, so we need those xcode projects too. This does them all at once so we don't have to rev each time we realize we want another tool. Sadly, it's getting pretty ugly :(

This may require manually whacking the google-breakpad folder in objdir on non-clean-build build machines to get it working right.
Attachment #371386 - Flags: review?(alqahira)
This is a bit rough, but it will start to get us crash reports, so I'd like to land it soon and do follow up bugs for the rest. That way we can start shaking out issues that I *don't* know about in parallel with fixing the rest.

I have one small outstanding patch to breakpad that blocks this landing, but I wanted to get the review started. If anyone tests this patch now, the title of the crash dialog will make no sense ("The the Camino developers program Camino ..."), but that won't be true by the time is lands.

Things that I want to do follow-ups for (quickly ;) ):
- We won't actually get comments, even though the UI is there. I think that will just a very tiny local patchto Breakpad, changing a key in the header to match what Socorro looks for (unavoidable, I'm afraid).
- We show UI for email, but shouldn't. Socorro will throw it away whatever we do, so we need to remove the UI. I have plans to make that configurable in Breakpad upstream.
- We don't get uptime numbers (we tell Scorro it crashed after 1 second, since it requires a value). I believe I can upstream that, and then we'll just need another key change.
- We aren't generating or uploading symbols yet, so the crash logs won't be so useful yet. But at least we'll have numbers ;)

Other notes:
- I plan to do work shortly to make the entire UI flexible and make all localization happen in strings files rather than the nib; that will be done upstream. So people shouldn't start localizing the nib.
- Breakpad's UI doesn't have a relaunch option. I'm suppressing the Apple UI in this patch because it's really confusing to have both, but that means users will lose relaunch :( We may just have to live with that, or it may be something we can push into Breakpad, depending on time.
- I may change to only suppressing Apple's UI in release, to make getting crash logs for debug builds easier for developers; we'll see what people think once we're living on it.
- We'll need Camino added as a search option to Socorro, since right now you have to change the URL manually.

And Smokey will be glad to hear that I've build both Static and Shared on Tiger, so there's a good chance I won't break the build :)
Attachment #371392 - Flags: superreview?(mikepinkerton)
(In reply to comment #14)
> - I may change to only suppressing Apple's UI in release, to make getting crash
> logs for debug builds easier for developers; we'll see what people think once
> we're living on it.

We provide a hidden defaults key: "OSCrashreporter" in the toolkit code that controls this, FWIW:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/mac_utils.mm#51

> - We'll need Camino added as a search option to Socorro, since right now you
> have to change the URL manually.

You'll need to file a Server Ops bug, I think something like bug 486706, since we still don't have an admin page for adding app versions. I'm not sure what exactly the SQL looks like these days though. You can ask aking about it.
Also, just curious, how does:
(In reply to comment #14)
> - I plan to do work shortly to make the entire UI flexible and make all
> localization happen in strings files rather than the nib; that will be done
> upstream. So people shouldn't start localizing the nib.

jibe with:
(In reply to comment #7)
> We can't use the upload client part of that code; the decision to use Mozilla's
> one-language-per-binary localization makes it unsuitable for our use.

?
Because we'll ships a .strings file for every language we support in a single application, the same way we do with all the other strings in Camino. Users won't have to pick a single language at download time the way the Mozilla localization system forces them to.
Breakpad r326 landed, which unblocks the initial landing.
Attachment #371386 - Flags: review?(alqahira) → review+
Comment on attachment 371386 [details] [diff] [review]
makefile, take 2 [landed]

>-	mkdir -p sharedmenuscocoa sparkle growl google-breakpad/src/client/mac
>+	mkdir -p sharedmenuscocoa sparkle growl google-breakpad/src/client/mac \
>+	  google-breakpad/src/tools/mac/dump_syms \
>+		google-breakpad/src/tools/mac/crash_report \
>+		google-breakpad/src/tools/mac/symupload \

Please make the indentation consistent here on all the subsequent lines (two spaces).  Since we're already at four lines here now, I'd also prefer that each directory were on its own line to be consistent with the rest of the Makefile.  The trailing backslash on symupload is unnecessary unless you're also adding $(NULL) at the very end of the list like we do elsewhere.

Please also add the rest of the build/ dirs to GARBAGE_DIRS as you mentioned on irc.

r=ardissone with those changes.
Comment on attachment 371386 [details] [diff] [review]
makefile, take 2 [landed]

Landed with those changes. If you've built since yesterday, you'll want to nuke the google-breakpad directory in your $OBJDIR
Attachment #371386 - Attachment description: makefile, take 2 → makefile, take 2 [landed]
Attachment #371392 - Flags: superreview?(mikepinkerton) → superreview+
Comment on attachment 371392 [details] [diff] [review]
initial build integration

sr=pink

Socorro?
Landed on CVS trunk. I'll file follow-ups for everything left, and post them all here.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Filed:
- Bug 488512 (symbols)
- Bug 488513 (updating breakpad for Socorro support, which covers both uptime and getting comemnts)
- Bug 488514 (updating breakpad for l10n, which covers l10n and removing the email field)
- Bug 488518 (pref for showing Apple's crash UI)
- Bug 488528 (getting listed on crash-stats.mozilla.com)

If I've forgotten anything, please file another follow-up!
(In reply to comment #20)
> (From update of attachment 371386 [details] [diff] [review])
> Landed with those changes. If you've built since yesterday, you'll want to nuke
> the google-breakpad directory in your $OBJDIR

Just for the record, since we hit this on the tinderboxen, if you didn't nuke before pulling, you'll experience cvs checkout failure until you also nuke google-breakpad in $SRCDIR (and then nuke it again in $OBJDIR).
Also for the record (since we're not filing a bug and there's not much we can do), there were slight but noticeable Ts hits on all the boxen:

0.92% on cb-xserve01, ~1.24% on boxset, and ~2.89% on cb-minibinus01 (its "normal swing range" is much wider, so whatever the true regression is will behard to ascertain reliably).
(In reply to comment #24)

Bug 488618 on exploring the possibility of scraping the Console
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: