1287614 - Collect uncaught chrome exceptions in order to improve quality.

Status: NEW

(Core :: General, defect)

Description

[Doug Turner (:dougt)]

We currently collect uncaught signals from C/C++.  We also collect uncaught javascript exceptions from addons.  It might be a good idea to collect uncaught javascript exceptions from any chrome code.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: report_chrome_exceptions.patch

Bill, is this the best way to do this?

Comment 2

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

Comment on attachment 8774572 [details] [diff] [review]
report_chrome_exceptions.patch

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

My main concern here is that we'll get so much data back that it will break things on the server side. This happened a while back with the message manager message name telemetry. Even with just add-ons, this histogram is already big enough that I can't load it in telemetry.mozilla.org. Roberto, do you have any advice here?

Also, the name JS_TELEMETRY_ADDON_EXCEPTIONS needs to be fixed.

::: js/src/jsexn.cpp
@@ +710,5 @@
>  
>      JSCompartment* comp = stack->compartment();
>      JSAddonId* addonId = comp->creationOptions().addonIdOrNull();
>  
>      // We only want to send the report if the scope that just have thrown belongs to an add-on.

Please fix the comment.

@@ +726,5 @@
>                                                               &bytes)
>                                       : "anonymous";
>  
> +    UniqueChars addonIdChars(addonId ? JS_EncodeString(cx, addonId) :
> +                                       JS_smprintf("system"));

js_strdup should be fine.

Comment 3

[Roberto Agostino Vitillo (:rvitillo)]

(In reply to Bill McCloskey (:billm) from comment #2)
> Comment on attachment 8774572 [details] [diff] [review]
> report_chrome_exceptions.patch
> 
> Review of attachment 8774572 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> My main concern here is that we'll get so much data back that it will break
> things on the server side. This happened a while back with the message
> manager message name telemetry. Even with just add-ons, this histogram is
> already big enough that I can't load it in telemetry.mozilla.org. Roberto,
> do you have any advice here?

We can collect more data for this histogram but we will have to disable aggregation for it on t.m.o. You will still be able to analyze the data through a custom Spark job though.

Comment 4

[Lonnen :lonnen]

I recently noticed some concerns from Benjamin about possible personal data in collecting JS errors --  https://wiki.mozilla.org/Firefox/Data_Collection#Common_Problems

It's unclear if it applies in this context. :needinfo'ing him

Comment 5

[Benjamin Smedberg]

"We also collect uncaught javascript exceptions from addons." !?! Where is this documented?

This definitely ought to go through a data collection review. It's not clear to me what data this actually collects (and that should be documented in-tree), and it's also not clear what actions we're planning on taking and who's responsible for that.

The particular concerns are about leaking identifying data into telemetry payloads. It's definitely not ok to just .toString() an exception and send it, because that will often contain user data as a part of the exception.

I do want us to be able to collect these, as long as we can make strong assertions about what data we are collecting and who is/how we're going to use this data.

Comment 6

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

(In reply to Benjamin Smedberg [:bsmedberg] from comment #5)
> "We also collect uncaught javascript exceptions from addons." !?! Where is
> this documented?
> 
> This definitely ought to go through a data collection review. It's not clear
> to me what data this actually collects (and that should be documented
> in-tree), and it's also not clear what actions we're planning on taking and
> who's responsible for that.

This was originally added in bug 1080212. You and Allison were pretty involved in the design of what was collected. I assumed that sufficed for data collection review.

If there's more we should do here, can you give details? Where should the in-tree documentation be located?

> The particular concerns are about leaking identifying data into telemetry
> payloads. It's definitely not ok to just .toString() an exception and send
> it, because that will often contain user data as a part of the exception.

Right, we're not doing that. We report:
addon-id
name of function throwing the exception
filename the function is in (just the basename, not the dirname)
line number

Comment 7

[Benjamin Smedberg]

Aha, I remember a little bit of this. From what you've described, yes it should be fine to collect just error counts/function/filename. The data review needs to happen against the actual changes to histograms.json or other documentation.

We have been tightening up the documentation requirements since then. So re-reviewing JS_TELEMETRY_ADDON_EXCEPTIONS, I'd ask for: 1) a detailed description of the "key" 2) a description of how this is going provide value (who's going to monitor it, what actions they're going to take). Is anyone using the data from bug 1080212 right now?

So for this new collection, is it going into a histogram? Similar concerns apply here. Let's document the keys and data in detail, and document how this is going to be used, which will help determine whether this should actually be expires: never or should start out as a 6-month data collection period.