Last Comment Bug 765065 - Annotation for crash reports: "Are we GCing?"
: Annotation for crash reports: "Are we GCing?"
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
-- enhancement (vote)
: mozilla17
Assigned To: Bill McCloskey (:billm)
: Andrew Overholt [:overholt]
Depends on:
  Show dependency treegraph
Reported: 2012-06-14 15:45 PDT by Jesse Ruderman
Modified: 2012-08-17 01:43 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (9.56 KB, patch)
2012-08-10 17:35 PDT, Bill McCloskey (:billm)
benjamin: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image Jesse Ruderman 2012-06-14 15:45:51 PDT
This would allow us to gather statistics on GC crashes more easily -- and even if stack walking fails.

(For bonus points, say whether it's a compartment / multi-compartment / global GC)
Comment 1 User image Ted Mielczarek [:ted.mielczarek] 2012-06-14 16:08:27 PDT
This is a little bit of a PITA, because only the JS engine knows if we're GCing, right? And the JS engine doesn't know how to talk to Breakpad directly. Do we have a callback that happens when we enter/leave GC where Gecko could add this annotation?
Comment 2 User image Bill McCloskey (:billm) 2012-06-14 16:10:56 PDT
Yeah, xpconnect already knows when we're in a GC, and it can talk to breakpad.
Comment 3 User image Ted Mielczarek [:ted.mielczarek] 2012-06-14 16:27:03 PDT
Comment 4 User image Josh Matthews [:jdm] 2012-06-19 21:15:08 PDT
We can either dump it in the xpconnect callback ( or register a special callback ( to do so, which is conceptually a bit cleaner.
Comment 5 User image Bill McCloskey (:billm) 2012-08-10 17:35:32 PDT
Created attachment 651060 [details] [diff] [review]

This seems to do the job, at least on Linux.
Comment 6 User image Benjamin Smedberg [:bsmedberg] 2012-08-13 09:44:47 PDT
Comment on attachment 651060 [details] [diff] [review]

Remove the stray printf in CrashReporter::SetGarbageCollecting. I'm torn on whether we should be writing "IsGarbageCollecting=0" ever; perhaps it would be better to only write that field if we are garbage collecting?
Comment 7 User image Bill McCloskey (:billm) 2012-08-15 10:50:07 PDT

I changed it so the field is only written if it's 1.
Comment 8 User image Bill McCloskey (:billm) 2012-08-15 11:15:55 PDT
Comment on attachment 651060 [details] [diff] [review]

Just a note about this request:
This patch doesn't fix any bugs. It just adds data to crashreports that make it easier to see which crashes are GC-related. Given that incremental GC landed in FF16, it would be really useful to be able to compare crash rates between 15 and 16. That's why I'm requesting beta approval.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None.
User impact if declined: None.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Very low.
String or UUID changes made by this patch: None.
Comment 9 User image Ryan VanderMeulen [:RyanVM] 2012-08-15 18:44:51 PDT
Comment 10 User image Lukas Blakk [:lsblakk] use ?needinfo 2012-08-16 11:21:05 PDT
Comment on attachment 651060 [details] [diff] [review]

Approving for branches so we're able to get better crash data.

Note You need to log in before you can comment on or make changes to this bug.