Last Comment Bug 627464 - Annotate crash reports about gfx features in use
: Annotate crash reports about gfx features in use
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 All
: -- normal with 1 vote (vote)
: ---
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
Mentors:
Depends on:
Blocks: 629265
  Show dependency treegraph
 
Reported: 2011-01-20 11:49 PST by Jeff Muizelaar [:jrmuizel]
Modified: 2011-03-02 12:56 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
report gfx features (23.70 KB, patch)
2011-02-17 12:51 PST, Benoit Jacob [:bjacob] (mostly away)
jmuizelaar: review-
Details | Diff | Splinter Review
report gfx features, updated (23.78 KB, patch)
2011-02-24 13:53 PST, Benoit Jacob [:bjacob] (mostly away)
jmuizelaar: review+
joe: approval2.0+
Details | Diff | Splinter Review
report gfx features + please LeakLog (25.68 KB, patch)
2011-03-01 14:20 PST, Benoit Jacob [:bjacob] (mostly away)
jmuizelaar: review+
joe: approval2.0+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2011-01-20 11:49:49 PST
We should do this so we can more easily ignore these crashes
Comment 1 Jeff Muizelaar [:jrmuizel] 2011-01-20 12:02:10 PST
This should perhaps softblock
Comment 2 Joe Drew (not getting mail) 2011-01-20 15:41:40 PST
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.
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2011-02-14 13:08:29 PST
This would indeed have saved me 2 days on bug 629265.
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2011-02-17 12:51:04 PST
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 -).
Comment 5 Jeff Muizelaar [:jrmuizel] 2011-02-22 12:55:02 PST
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.
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2011-02-24 13:53:31 PST
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.
Comment 7 Jeff Muizelaar [:jrmuizel] 2011-02-25 08:22:58 PST
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.
Comment 8 Joe Drew (not getting mail) 2011-02-25 10:53:44 PST
Comment on attachment 514912 [details] [diff] [review]
report gfx features, updated

Looks good.
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2011-03-01 14:20:12 PST
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
Comment 10 Benoit Jacob [:bjacob] (mostly away) 2011-03-02 08:46:26 PST
OK, the .forget() was bad here, removing it makes it all green.
Comment 11 Benoit Jacob [:bjacob] (mostly away) 2011-03-02 12:56:01 PST
http://hg.mozilla.org/mozilla-central/rev/b6ca6d65fafa

Note You need to log in before you can comment on or make changes to this bug.