848861 - Send stack traces with FHR errors

Status: RESOLVED FIXED

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

Description

[Gregory Szorc [:gps]]

+++ 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.

Comment 1

[Gregory Szorc [:gps]]

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<[e-mail]:5849 < [e-mail]:192 < [e-mail]:64 < [e-mail]:145 < [e-mail]:45 < [e-mail]:190 < [e-mail]:5860 < [e-mail]:982 < PDBU_telemetry/<[e-mail]:1015"
  ]

Comment 2

[Gregory Szorc [:gps]]

Attachment: Send stack traces, v1

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).

Comment 3

[Richard Newman [: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.

Comment 4

[Richard Newman [:rnewman]]

Let's keep this on our radar for uplift, because it'll help debugging *a lot*.

Comment 5

[Mike Connor [:mconnor]]

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.

Comment 6

[Gregory Szorc [:gps]]

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.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[Gregory Szorc [:gps]]

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.

Comment 9

[bhavana bajaj [:bajaj]]

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

Comment 10

[Gregory Szorc [:gps]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/06b482d67485