Closed Bug 1213007 Opened 9 years ago Closed 9 years ago

gfxCrash - a combination of gfxCriticalError, MOZ_CRASH and telemetry

Categories

(Core :: Graphics, defect)

43 Branch
Unspecified
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- fixed
firefox44 --- fixed

People

(Reporter: milan, Assigned: milan)

References

Details

(Keywords: feature, Whiteboard: [gfx-noted])

Attachments

(2 files, 5 obsolete files)

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.
Whiteboard: [gfx-noted]
Assignee: nobody → milan
(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.
Attached patch (wip) gfxCrash start... (obsolete) — Splinter Review
compiles, envvar to force telemetry behaviour in non-release builds.
Attachment #8671623 - Attachment is obsolete: true
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.
Attachment #8671649 - Attachment is obsolete: true
Attachment #8671951 - Flags: review?(dvander)
Attachment #8671951 - Attachment description: gfxCrash implementation. r=dvander → Part 1. gfxCrash implementation. r=dvander
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
Attachment #8671951 - Flags: review?(dvander) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea24163ab044

Review comments and a minor addition of envvar to force crash in release builds.
Attachment #8673276 - Flags: review+
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.
Attachment #8671952 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/80f5bc32367c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Attachment #8673702 - Flags: checkin? → checkin+
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.
Attachment #8673702 - Flags: approval-mozilla-aurora?
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.
Attachment #8673702 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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!
Flags: needinfo?(milan)
Approval Request Comment
Aurora patch equivalent of the approved uplift.
Flags: needinfo?(milan) → needinfo?(cbook)
Attachment #8680044 - Flags: review+
Attachment #8680044 - Flags: approval-mozilla-aurora?
Comment on attachment 8680044 [details] [diff] [review]
Part 1. Aurora patch equivalent.

Already landed, looks good to me. Retroactively approving!
Attachment #8680044 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1276062
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: