The default bug view has changed. See this FAQ.

Annotate crash reports about gfx features in use

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
7 months ago

People

(Reporter: jrmuizel, Assigned: bjacob)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
We should do this so we can more easily ignore these crashes
(Reporter)

Comment 1

6 years ago
This should perhaps softblock
blocking2.0: --- → ?
Maybe you can convince me this should block, Jeff, but it seems like it'd be purely gravy, not necessarily required for 4.0

I would a+ a patch SO HARD.
blocking2.0: ? → -
(Assignee)

Comment 3

6 years ago
This would indeed have saved me 2 days on bug 629265.
Blocks: 629265
(Assignee)

Comment 4

6 years ago
Created attachment 513225 [details] [diff] [review]
report gfx features

This patch adds App Notes for actually tried/successful/failed Gfx features, like this:

D2D? D2D+
DWrite? DWrite+
D3D10 Layers? D3D10 Layers+
WebGL? EGL? EGL+
GL Context? GL Context+
WebGL+

Indeed, we discussed that knowing what had actually happened was more useful than knowing the prefs, and that info, together with info we already get in crash reports, allows us to know when users have force-enabled stuff anyway.

The Xxx? part is added to AppNotes before the feature is tried. That will help debugging crashes that occurred during initialization of said feature.

The feature name is repeated (Xxx? Xxx+) for the following reason. Each of these strings is printed at most once to avoid flooding the AppNotes. So if Xxx succeeded, then Yyy succeeded, then Xxx failed, we get:
Xxx? Xxx+
Yyy? Yyy+
Xxx-

since the Xxx? is not repeated. That's why it's useful to repeat the feature name (Xxx- instead of just -).
Attachment #513225 - Flags: review?(jmuizelaar)
(Reporter)

Comment 5

6 years ago
Comment on attachment 513225 [details] [diff] [review]
report gfx features


>+
>+void
>+ScopedGfxFeatureReporter::WriteAppNote(char statusChar)
>+{
>+#ifdef MOZ_GFXFEATUREREPORTER
>+  static nsCString featuresAlreadyReported;
>+
>+  nsACString::const_iterator start, end;
>+  featuresAlreadyReported.BeginReading(start);
>+  featuresAlreadyReported.EndReading(end);
>+

Please use a list of strings instead of a string of strings.

> 
>     // if initialization fails, ensure all symbols are zero, to avoid hard-to-understand bugs
>     if (!mInitialized)
>-      mSymbols.Zero();
>+        mSymbols.Zero();
>+
>+    if (mInitialized)
>+        reporter.SetSuccessful();

maybe use an else here.
Attachment #513225 - Flags: review?(jmuizelaar) → review-
(Assignee)

Comment 6

6 years ago
Created attachment 514912 [details] [diff] [review]
report gfx features, updated

applied your comments. amazing how the array-of-strings idea turns out to be more concise.
Attachment #513225 - Attachment is obsolete: true
Attachment #514912 - Flags: review?(jmuizelaar)
(Reporter)

Comment 7

6 years ago
Comment on attachment 514912 [details] [diff] [review]
report gfx features, updated


>+
>+void
>+ScopedGfxFeatureReporter::WriteAppNote(char statusChar)
>+{
>+#ifdef MOZ_GFXFEATUREREPORTER
>+  static nsTArray<nsCString> featuresAlreadyReported;
>+
>+  nsCAutoString featureString;
>+  featureString.AppendPrintf("%s%c%c",
>+                             mFeature,
>+                             statusChar,
>+                             statusChar == '?' ? ' ' : '\n');
>+
>+  if (!featuresAlreadyReported.Contains(featureString)) {
>+    featuresAlreadyReported.AppendElement(featureString);
>+    CrashReporter::AppendAppNotesToCrashReport(featureString);
>+  }
>+#else
>+  (void) statusChar;
>+#endif
>+}
>+

It would be cleaner to have a single #ifdef and do a completely separate stub implementation.
Attachment #514912 - Flags: review?(jmuizelaar) → review+
Comment on attachment 514912 [details] [diff] [review]
report gfx features, updated

Looks good.
Attachment #514912 - Flags: approval2.0+
Assignee: nobody → bjacob

Updated

6 years ago
Version: unspecified → Trunk
(Assignee)

Comment 9

6 years ago
Created attachment 516039 [details] [diff] [review]
report gfx features + please LeakLog

Here is a version that hopefully will please tryserver...
http://tbpl.mozilla.org/?tree=MozillaTry&rev=661e198bd49d
Attachment #514912 - Attachment is obsolete: true
Attachment #516039 - Flags: review?(jmuizelaar)
(Reporter)

Updated

6 years ago
Attachment #516039 - Flags: review?(jmuizelaar) → review+
OK, the .forget() was bad here, removing it makes it all green.
Attachment #516039 - Flags: approval2.0+
(Assignee)

Updated

6 years ago
Summary: Annotate crash reports if the forced-enabled prefs are set → Annotate crash reports about gfx features in use
http://hg.mozilla.org/mozilla-central/rev/b6ca6d65fafa
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.