Closed Bug 848861 Opened 7 years ago Closed 7 years ago

Send stack traces with FHR errors

Categories

(Firefox Health Report Graveyard :: Client: Desktop, defect, P2)

defect

Tracking

(firefox20 unaffected, firefox21 fixed, firefox22 fixed)

RESOLVED FIXED
Firefox 22
Tracking Status
firefox20 --- unaffected
firefox21 --- fixed
firefox22 --- fixed

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #845431 +++

In bug 845431 we added more errors to FHR's payload. It's resulted in some useful information in the payloads, including new bugs being filed. However, the error messages themselves often don't contain enough information to act on. e.g. bug 846840.

I'm proposing we add stack traces to the errors as well. I plan to do this with CommonUtils.exceptionStr(). This particular implementation munges paths so only the basename is reported. e.g. file:///Users/gps/src/mozilla-central/modules/foo.jsm would be reported as "foo.jsm". This is good for a few reasons: 1) it's concise 2) it removes potentially user-identifying components from the path.

I have a working patch in my patch queue. Next time I build I'll put it on and post an example of what the new output looks like.
Here is a payload snippet with stack traces on:

  "errors": [
    "Provider error: org.mozilla.places: Failed to collect: This is a dummy error. JS Stack trace: PlacesProvider.prototype<._collectData@HealthReport.jsm:5849 < TaskImpl_run@Task.jsm:192 < effort@promise.js:64 < resolveDeferred@promise.js:145 < then@promise.js:45 < resolve@promise.js:190 < onResult@HealthReport.jsm:5860 < reportResult@PlacesDBUtils.jsm:982 < PDBU_telemetry/<.handleResult@PlacesDBUtils.jsm:1015"
  ]
Patch is trivial. This review is all about privacy concerns. I'm reasonably confident the stacks won't leak anything. I think the error message has more potential for leaking than the stacks do (since we strip all but the basename off the filenames).
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #722909 - Flags: review?(rnewman)
Comment on attachment 722909 [details] [diff] [review]
Send stack traces, v1

Review of attachment 722909 [details] [diff] [review]:
-----------------------------------------------------------------

I'm happy with the scope of this. The only concerns I can think of:

* if a user add-on ends up in the call stack (but we already report add-ons)
* a filename or function name (e.g., in user.js) leaks some user info -- presumably very, very unlikely

My vote is to land this, and examine the responses we get from Nightly users to search for privacy concerns. If there are any, then don't ship it to Aurora, and/or back out from Nightly.

To be extra thorough, limit stack inclusion to users who have opted in to crash reporting, which necessarily includes stacks. That probably gives us 99% overlap whilst still respecting user intent.

Second set of eyes, please.
Attachment #722909 - Flags: superreview?(mconnor)
Attachment #722909 - Flags: review?(rnewman)
Attachment #722909 - Flags: review+
Let's keep this on our radar for uplift, because it'll help debugging *a lot*.
Priority: -- → P2
Comment on attachment 722909 [details] [diff] [review]
Send stack traces, v1

sr=me, but we should file a bug to explicitly track sign-offs for reporting this data on release channels.  I am not 100% sure that we have coverage for errors in payloads, and I'd like to cover those bases.
Attachment #722909 - Flags: superreview?(mconnor) → superreview+
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcf9c06aa2cb

Will file a new P1 to track the status of error reporting for beta and release channels.
Target Milestone: --- → mozilla22
https://hg.mozilla.org/mozilla-central/rev/bcf9c06aa2cb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment on attachment 722909 [details] [diff] [review]
Send stack traces, v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): FHR
User impact if declined: We won't have as much visibility into errors until 22.
Testing completed (on m-c, etc.): It's baked for a while. Things look good.
Risk to taking this patch (and alternatives if risky): Very low.
String or IDL/UUID changes made by this patch: None

We were waiting on sign-off to uplift this patch because we weren't sure if it was appropriate for the Beta/Release channels. We have obtained sign-off, so requesting uplift.
Attachment #722909 - Flags: approval-mozilla-aurora?
Comment on attachment 722909 [details] [diff] [review]
Send stack traces, v1

low risk patch helping with more visibility into error's & has the needed sign-off.Approving for uplift
Attachment #722909 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: Metrics and Firefox Health Report → Client: Desktop
Flags: superreview+
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla22 → Firefox 22
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.