Last Comment Bug 765065 - Annotation for crash reports: "Are we GCing?"
: Annotation for crash reports: "Are we GCing?"
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla17
Assigned To: Bill McCloskey (:billm)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency tree / graph
 
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed

Attachments
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
View All Add an attachment (proposed patch, testcase, etc.)

Description 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 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 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 Ted Mielczarek [:ted.mielczarek] 2012-06-14 16:27:03 PDT 
Great!
Comment 4 Josh Matthews [:jdm] 2012-06-19 21:15:08 PDT 
We can either dump it in the xpconnect callback (http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSRuntime.cpp#583) or register a special callback (http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/nsXPConnect.cpp?force=1#2299) to do so, which is conceptually a bit cleaner.
Comment 5 Bill McCloskey (:billm) 2012-08-10 17:35:32 PDT 
Created attachment 651060 [details] [diff] [review]
patch

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

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 Bill McCloskey (:billm) 2012-08-15 10:50:07 PDT 
https://hg.mozilla.org/integration/mozilla-inbound/rev/dad98b444955

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

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 Ryan VanderMeulen [:RyanVM] 2012-08-15 18:44:51 PDT 
https://hg.mozilla.org/mozilla-central/rev/dad98b444955
Comment 10 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-16 11:21:05 PDT 
Comment on attachment 651060 [details] [diff] [review]
patch

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.