Closed Bug 1376359 Opened 2 years ago Closed 2 years ago

stylo: Annotate crash reports

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 2 obsolete files)

Bill and Andrew suggested we do this to make it easier to triage crashes. Seems simple enough.
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+
(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.
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)
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)
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.
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+
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d71a65e4af01
Include the stylo pref in telemetry pings. r=gsvelto
https://hg.mozilla.org/mozilla-central/rev/d71a65e4af01
Status: NEW → RESOLVED
Closed: 2 years ago
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)
(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)
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.