Closed Bug 627464 Opened 13 years ago Closed 13 years ago

Annotate crash reports about gfx features in use

Categories

(Core :: Graphics, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- -

People

(Reporter: jrmuizel, Assigned: bjacob)

References

Details

Attachments

(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+
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)
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-
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


>+
>+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
Version: unspecified → Trunk
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)
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
http://hg.mozilla.org/mozilla-central/rev/b6ca6d65fafa
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: