765065 - Annotation for crash reports: "Are we GCing?"

Status: RESOLVED FIXED

Description

[Jesse Ruderman]

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] [:ted.mielczarek]]

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 [inactive unless it's an emergency] (:billm)]

Yeah, xpconnect already knows when we're in a GC, and it can talk to breakpad.

Comment 3

[Ted Mielczarek [:ted] [:ted.mielczarek]]

Great!

Comment 4

[Josh Matthews [:jdm]]

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 [inactive unless it's an emergency] (:billm)]

Created attachment 651060 [details] [diff] [review]
patch

This seems to do the job, at least on Linux.

Comment 6

[Benjamin Smedberg]

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 [inactive unless it's an emergency] (:billm)]

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 [inactive unless it's an emergency] (:billm)]

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]]

https://hg.mozilla.org/mozilla-central/rev/dad98b444955

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 651060 [details] [diff] [review]
patch

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

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/08e55f351f6d
https://hg.mozilla.org/releases/mozilla-beta/rev/49888a27cff0