Closed
Bug 1213007
Opened 10 years ago
Closed 10 years ago
gfxCrash - a combination of gfxCriticalError, MOZ_CRASH and telemetry
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla44
People
(Reporter: milan, Assigned: milan)
References
Details
(Keywords: feature, Whiteboard: [gfx-noted])
Attachments
(2 files, 5 obsolete files)
12.98 KB,
patch
|
milan
:
review+
lizzard
:
approval-mozilla-aurora+
cbook
:
checkin+
|
Details | Diff | Splinter Review |
12.97 KB,
patch
|
milan
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•10 years ago
|
Whiteboard: [gfx-noted]
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → milan
Assignee | ||
Comment 1•10 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•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
compiles, envvar to force telemetry behaviour in non-release builds.
Attachment #8671623 -
Attachment is obsolete: true
Assignee | ||
Comment 4•10 years ago
|
||
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•10 years ago
|
Attachment #8671951 -
Attachment description: gfxCrash implementation. r=dvander → Part 1. gfxCrash implementation. r=dvander
Assignee | ||
Comment 5•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
Attachment #8671951 -
Attachment is obsolete: true
Attachment #8673276 -
Attachment is obsolete: true
Attachment #8673702 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8673702 -
Flags: checkin?
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 9•10 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
Comment 10•10 years ago
|
||
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Updated•10 years ago
|
Attachment #8673702 -
Flags: checkin? → checkin+
Assignee | ||
Comment 12•10 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 13•10 years ago
|
||
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+
Comment 14•10 years ago
|
||
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•10 years ago
|
||
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?
status-firefox43:
--- → fixed
Flags: needinfo?(cbook)
Comment 17•10 years ago
|
||
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+
You need to log in
before you can comment on or make changes to this bug.
Description
•