1213007 - gfxCrash - a combination of gfxCriticalError, MOZ_CRASH and telemetry

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

gfxCriticalError is useful, because it crashes in DEBUG, and it leaves traces of bad things that happened in the about:support, as well as crash reports.  Usually it takes a crash report for us to notice them.

MOZ_CRASH is useful, especially now that it's annotated, it crashes in both DEBUG and NDEBUG.  It is also drastic in that it tells us about bad things out there by crashing the browser/tab.

Telemetry is also useful, and non-invasive, in that we get some information, but the users experience is not disturbed by it.

Enter gfxCrash (I actually like gfxSurprise, but...)

In DEBUG, gfxCrash would be equivalent to gfxCriticalError.

In Nightly and Dev Edition, gfxCrash would equivalent to MOZ_CRASH.

In Beta and Release, gfxCrash would be some kind of telemetry.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Milan Sreckovic [:milan] from comment #0)
> ...
> In Beta and Release, gfxCrash would be some kind of telemetry.

Enumerated would be the way to go.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: (wip) gfxCrash outline...

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: (wip) gfxCrash start...

compiles, envvar to force telemetry behaviour in non-release builds.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 1. gfxCrash implementation. r=dvander

In DEBUG, gfxCrash is like gfxCriticalError - it prints and asserts.
In NDEBUG in Nightly and Dev Edition, gfxCrash is like MOZ_CRASH.
In NDEBUG in Beta and Release, gfxCrash is a GFX_CRASH telemetry histogram.

You can force the telemetry behaviour in Nightly and Dev Edition by setting MOZ_GFX_CRASH_TELEMETRY environment variable.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 2. Using gfxCrash instead of MOZ_CRASH in a few places.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8671951 [details] [diff] [review]
Part 1. gfxCrash implementation. r=dvander

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

Looks good, though I'm not a big fan of "CrashOrCall". Maybe "DoCrashDiagnostic" or something?

::: gfx/2d/Logging.h
@@ +566,5 @@
> +// On beta and release versions, it will telemetry count, but proceed.
> +//
> +// You should create a (new) enum in the LogReason and use it for the reason
> +// parameter to ensure uniqueness.
> +#define gfxCrash(reason) gfxCriticalError(int(LogOptions::AutoPrefix) | int(LogOptions::AssertOnCall) | int(LogOptions::CrashOrCall), reason)

nit: should be ", (reason))" since it's in a macro but I doubt it really matters here.

::: gfx/thebes/gfxPlatform.cpp
@@ +302,5 @@
> +void
> +CrashStatsLogForwarder::CrashOrCall(LogReason aReason)
> +{
> +#ifndef RELEASE_BUILD
> +  // Let's us test the telemetry aproach in non-release builds

nit: Lets us test the Telemetry path

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 1. gfxCrash implementation. Carry r=dvander

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea24163ab044

Review comments and a minor addition of envvar to force crash in release builds.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 1. gfxCrash implementation. Carry r=dvander

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f417f2c18245

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8671952 [details] [diff] [review]
Part 2. Using gfxCrash instead of MOZ_CRASH in a few places.

Let the basic stuff land, we can start using it and land it with different bugs.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80f5bc32367c

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/80f5bc32367c

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8673702 [details] [diff] [review]
Part 1. gfxCrash implementation. Carry r=dvander

Approval Request Comment
This is a new feature that lets us record what was previously a MOZ_CRASH, without actually crashing.  So, we get the information that we use MOZ_CRASH for, without messing up the user experience.  If we uplift to Aurora, we'll have it be in Beta in a week, which gives us a chance of doing that kind of thing on Beta.

Right now, this patch just gives us the API to use, nobody is using it, so it should be safe.

Comment 13

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8673702 [details] [diff] [review]
Part 1. gfxCrash implementation. Carry r=dvander

This sounds amazing, gets the infrastructure in place so we can stop crashes but still get good diagnostic info.  Yes please uplift to aurora.

Comment 14

[Carsten Book [:Tomcat]]

Hi, this has problems uplifting to aurora:

grafting 308698:80f5bc32367c "Bug 1213007 - Part 1. Implementing gfxCrash. r=dvander"
merging gfx/2d/Factory.cpp
merging gfx/2d/Logging.h
merging gfx/thebes/gfxPlatform.cpp
merging toolkit/components/telemetry/Histograms.json
warning: conflicts during merge.
merging toolkit/components/telemetry/Histograms.json incomplete! (edit conflicts, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

could you take a look, thanks!

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 1. Aurora patch equivalent.

Approval Request Comment
Aurora patch equivalent of the approved uplift.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2f90ffb5ee45

Comment 17

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8680044 [details] [diff] [review]
Part 1. Aurora patch equivalent.

Already landed, looks good to me. Retroactively approving!