gfxCrash - a combination of gfxCriticalError, MOZ_CRASH and telemetry

RESOLVED FIXED in Firefox 43

Status

()

Core
Graphics
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: milan, Assigned: milan)

Tracking

({feature})

43 Branch
mozilla44
Unspecified
All
feature
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed, firefox44 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Whiteboard: [gfx-noted]
(Assignee)

Updated

2 years ago
Assignee: nobody → milan
(Assignee)

Comment 1

2 years ago
(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.
(Assignee)

Comment 2

2 years ago
Created attachment 8671623 [details] [diff] [review]
(wip) gfxCrash outline...
(Assignee)

Comment 3

2 years ago
Created attachment 8671649 [details] [diff] [review]
(wip) gfxCrash start...

compiles, envvar to force telemetry behaviour in non-release builds.
Attachment #8671623 - Attachment is obsolete: true
(Assignee)

Comment 4

2 years ago
Created attachment 8671951 [details] [diff] [review]
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.
Attachment #8671649 - Attachment is obsolete: true
Attachment #8671951 - Flags: review?(dvander)
(Assignee)

Updated

2 years ago
Attachment #8671951 - Attachment description: gfxCrash implementation. r=dvander → Part 1. gfxCrash implementation. r=dvander
(Assignee)

Comment 5

2 years ago
Created attachment 8671952 [details] [diff] [review]
Part 2. Using gfxCrash instead of MOZ_CRASH in a few places.
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+
(Assignee)

Comment 7

2 years ago
Created attachment 8673276 [details] [diff] [review]
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.
Attachment #8673276 - Flags: review+
(Assignee)

Comment 8

2 years ago
Created attachment 8673702 [details] [diff] [review]
Part 1. gfxCrash implementation. Carry r=dvander

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f417f2c18245
Attachment #8671951 - Attachment is obsolete: true
Attachment #8673276 - Attachment is obsolete: true
Attachment #8673702 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8673702 - Flags: checkin?
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 9

2 years ago
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
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44

Updated

2 years ago
Attachment #8673702 - Flags: checkin? → checkin+
(Assignee)

Comment 12

2 years ago
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)
(Assignee)

Comment 15

2 years ago
Created attachment 8680044 [details] [diff] [review]
Part 1. Aurora patch equivalent.

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?
https://hg.mozilla.org/releases/mozilla-aurora/rev/2f90ffb5ee45
status-firefox43: --- → fixed
Flags: needinfo?(cbook)
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+
(Assignee)

Updated

2 years ago
Keywords: feature
Depends on: 1276062
You need to log in before you can comment on or make changes to this bug.