Closed Bug 627464 Opened 9 years ago Closed 9 years ago

Annotate crash reports about gfx features in use


(Core :: Graphics, defect)

Not set



Tracking Status
blocking2.0 --- -


(Reporter: jrmuizel, Assigned: bjacob)




(1 file, 2 obsolete files)

We should do this so we can more easily ignore these crashes
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: ? → -
This would indeed have saved me 2 days on bug 629265.
Blocks: 629265
Attached patch report gfx features (obsolete) — Splinter Review
This patch adds App Notes for actually tried/successful/failed Gfx features, like this:

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

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+

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)
Comment on attachment 513225 [details] [diff] [review]
report gfx features

>+ScopedGfxFeatureReporter::WriteAppNote(char statusChar)
>+  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-
Attached patch report gfx features, updated (obsolete) — Splinter Review
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)
Comment on attachment 514912 [details] [diff] [review]
report gfx features, updated

>+ScopedGfxFeatureReporter::WriteAppNote(char statusChar)
>+  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);
>+  }
>+  (void) statusChar;

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
Version: unspecified → Trunk
Here is a version that hopefully will please tryserver...
Attachment #514912 - Attachment is obsolete: true
Attachment #516039 - Flags: review?(jmuizelaar)
Attachment #516039 - Flags: review?(jmuizelaar) → review+
OK, the .forget() was bad here, removing it makes it all green.
Attachment #516039 - Flags: approval2.0+
Summary: Annotate crash reports if the forced-enabled prefs are set → Annotate crash reports about gfx features in use
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.