If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

stylo: Annotate crash reports

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks: 3 bugs)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Bill and Andrew suggested we do this to make it easier to triage crashes. Seems simple enough.
Created attachment 8881321 [details] [diff] [review]
Annotate crash reports for stylo. v1
Attachment #8881321 - Flags: review?(nfroyd)
Attachment #8881321 - Flags: review?(cam)
Comment on attachment 8881321 [details] [diff] [review]
Annotate crash reports for stylo. v1

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

::: dom/base/nsDocument.cpp
@@ +13143,5 @@
> +#ifdef MOZ_CRASHREPORTER
> +      static bool sAnnotatedCrashReport = false;
> +      if (!sAnnotatedCrashReport) {
> +        CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("StyloIsEnabled"),
> +                                           NS_LITERAL_CSTRING("Yes"));

I guess this is OK, although it doesn't (and I suppose we can't) distinguish between crashes that occur due to non-stylo-backed documents in the same process that also loaded a stylo-backed document previously.  I wonder how the crash reporter determines which document is active, so that it can report the crashing URL (and whether we could use the same mechanism)?
Attachment #8881321 - Flags: review?(cam) → review+
Besides crash reports, for the field to also end up in the crash ping you should add it to the whitelist here:

https://dxr.mozilla.org/mozilla-central/rev/6b98953213350d847f2d1c8165922af47b96eee4/toolkit/components/crashes/CrashManager.jsm#227

... and here:

https://dxr.mozilla.org/mozilla-central/rev/6b98953213350d847f2d1c8165922af47b96eee4/toolkit/crashreporter/client/ping.cpp#124

And also add an entry to the documentation here:

https://dxr.mozilla.org/mozilla-central/rev/6b98953213350d847f2d1c8165922af47b96eee4/toolkit/components/telemetry/docs/data/crash-ping.rst#42
(In reply to Cameron McCormack (:heycam) from comment #2)
> I guess this is OK, although it doesn't (and I suppose we can't) distinguish
> between crashes that occur due to non-stylo-backed documents in the same
> process that also loaded a stylo-backed document previously.

I think this use-case is rare enough that it probably doesn't warrant spending time on.
Created attachment 8881428 [details] [diff] [review]
Annotate crash reports for stylo.
Attachment #8881428 - Flags: review?(gsvelto)
Attachment #8881428 - Flags: review+
Attachment #8881321 - Attachment is obsolete: true
Attachment #8881321 - Flags: review?(nfroyd)
Comment on attachment 8881428 [details] [diff] [review]
Annotate crash reports for stylo.

LGTM though you shouldn't need to check if you're annotating the crash report more than once. AnnotateCrashReport() replaces the previous entry anyway so it's safe to call it more than once for the same annotation:

https://dxr.mozilla.org/mozilla-central/rev/53477d584130945864c4491632f88da437353356/toolkit/crashreporter/nsExceptionHandler.cpp#2328

Also, since we're adding stuff to the crash ping you probably need a data review too.
Attachment #8881428 - Flags: review?(gsvelto) → review+
(In reply to Gabriele Svelto [:gsvelto] from comment #6)
> Comment on attachment 8881428 [details] [diff] [review]
> Annotate crash reports for stylo.
> 
> LGTM though you shouldn't need to check if you're annotating the crash
> report more than once. AnnotateCrashReport() replaces the previous entry
> anyway so it's safe to call it more than once for the same annotation:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 53477d584130945864c4491632f88da437353356/toolkit/crashreporter/
> nsExceptionHandler.cpp#2328

Yeah, but I wanted to avoid the extra hashtable lookup on every document load.

> 
> Also, since we're adding stuff to the crash ping you probably need a data
> review too.

I'm not sure who to flag for that. Can you flag the appropriate person?
Flags: needinfo?(gsvelto)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7)
> Yeah, but I wanted to avoid the extra hashtable lookup on every document
> load.

OK, makes sense.

> I'm not sure who to flag for that. Can you flag the appropriate person?

NI'ing Benjamin for the data review.
Flags: needinfo?(gsvelto) → needinfo?(benjamin)

Comment 9

3 months ago
data-r=me, although I suggest you should do this differently. Recording this *only* for crashes and not for other telemetry would be unfortunate, and assuming that stylo is pref-controlled, we could instead just add that pref to environment.settings.userPrefs, by adding it to this list:

https://dxr.mozilla.org/mozilla-central/rev/53477d584130945864c4491632f88da437353356/toolkit/components/telemetry/TelemetryEnvironment.jsm#158
Flags: needinfo?(benjamin)
Created attachment 8882065 [details] [diff] [review]
Include the stylo pref in telemetry pings. v1

Like this?
Attachment #8882065 - Flags: review?(gsvelto)
Attachment #8881428 - Attachment is obsolete: true
It seems that prefs recorded here are only visible when logged in to crash-stats.  Will that be convenient enough for us?  Can we search for crashes based on the value in the TelemetryEnvironment JSON blob, or does the CrashReporter::AnnotateCrashReport from the previous patch allow that?
I will be also interested to know whether we could leverage the annotation to monitor the trend of crash rate when we start to enable stylo by default and by level of population.

Comment 13

3 months ago
We can file a socorro issues to make this value a toplevel searchable field.

In terms of crash *rates*, I believe we already aggregate crash rates by experiments/pref rollouts, but not by pref values. My understanding of your rollout plan is that you're using pref experiments for initial testing, so that should be fine.
Attachment #8882065 - Flags: review?(gsvelto) → review+

Comment 14

3 months ago
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d71a65e4af01
Include the stylo pref in telemetry pings. r=gsvelto

Comment 15

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d71a65e4af01
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(In reply to Benjamin Smedberg [:bsmedberg] from comment #13)
> We can file a socorro issues to make this value a toplevel searchable field.

I'm not sure if this is easily feasible. Adrian, do you know?
This is a value in the TelemetryEnvironment field.
Flags: needinfo?(adrian)
Blocks: 1348896
(In reply to Marco Castelluccio [:marco] from comment #16)
> (In reply to Benjamin Smedberg [:bsmedberg] from comment #13)
> > We can file a socorro issues to make this value a toplevel searchable field.
> 
> I'm not sure if this is easily feasible. Adrian, do you know?
> This is a value in the TelemetryEnvironment field.

Given that the TelemetryEnvironment is a JSON blob encoded as a string, we will need to write some code to make that usable in Elasticsearch first. That makes it not trivial, but still very feasible. Please file a bug and assign it to either :willkg or :peterbe.
Flags: needinfo?(adrian)
Blocks: 1379162
I also don't see a way, even when logged in, to view the telemetry field in the reports list, so it's hard to see if a crash signature is stylo-only -- it requires two clicks for each crash checked.
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #18)
> I also don't see a way, even when logged in, to view the telemetry field in
> the reports list, so it's hard to see if a crash signature is stylo-only --
> it requires two clicks for each crash checked.

For the record (because there are multiple bugs going on about this), the TelemetryEnvironment field is now always whitelisted and no longer depends on PII access privileges. 

It's under the Metadata tab. E.g. https://crash-stats.mozilla.com/report/index/a90aa729-63b4-4223-9d4c-133c70170711#tab-metadata
You need to log in before you can comment on or make changes to this bug.