Closed Bug 586048 Opened 9 years ago Closed 9 years ago

Report graphics card and graphics driver version in crash dumps

Categories

(Core :: Graphics, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

Details

Attachments

(3 files, 3 obsolete files)

Ted, what is the best way to get data included in crash reports?
OS: Mac OS X → All
http://mxr.mozilla.org/mozilla-central/source/xpcom/system/nsICrashReporter.idl#90
Component: Breakpad Integration → Graphics
Product: Toolkit → Core
QA Contact: breakpad.integration → thebes
If you want to be able to search over the data in crash-stats, you will also need to file a bug to collect the data into the database and crash UI for it. If you just want it to be human-readable, use appendAppNotesToCrashReport.
(In reply to comment #2)
> If you want to be able to search over the data in crash-stats, you will also
> need to file a bug to collect the data into the database and crash UI for it.

Can we use AnnotateCrashReport and update the database later or do they need to be done in lock-step?

Also, it seems like AnnotateCrashReport would be called during startup. I'm worried about negatively impacting startup. What are the options if we want to do the detection at crash time?
I'm going to mark this as a blocker, just to make sure it's on people's radar.  I think we'll need this info in advance to make sure we consider all sources of crashes once we enable hardware acceleration.  Blocking betaN+.
blocking2.0: --- → betaN+
(In reply to comment #3)
> (In reply to comment #2)
> > If you want to be able to search over the data in crash-stats, you will also
> > need to file a bug to collect the data into the database and crash UI for it.
> 
> Can we use AnnotateCrashReport and update the database later or do they need to
> be done in lock-step?

You can send any data you want, crash-stats will simply ignore unknown keys.

> Also, it seems like AnnotateCrashReport would be called during startup. I'm
> worried about negatively impacting startup. What are the options if we want to
> do the detection at crash time?

We annotate a bunch of data during startup right now. Are you worried that actually gathering the data will be slow? I would recommend against doing anything in the exception handler, since it's unsafe to do almost anything at that point. If you only want to send this data for browser crashes (and not plugin or content crashes), then you could feasibly do it in the crash reporter client instead. We don't currently do anything like that, but there's no real reason you couldn't.
Blocks: 586396
Why does this block testpilot? It should be possible to design testpilot collection of this data using ctypes, and skip crash reporter altogether, if you wanted to get something together quickly.
If this is possibly slow, I would fork a thread to collect the data and annotate once you've collected it. Unless you also think that crashes are likely right at startup.
Summary: Report graphics card and graphics driver version → Report graphics card and graphics driver version in crash dumps
Duplicate of this bug: 584547
Depends on: 586046
The vendor and device ids should be enough for us to get a sense of what graphics hardware is out there. We can already get driver version from the module list provided D3D has been loaded. We can add more information as the need arises.
Assignee: nobody → jmuizelaar
Attachment #467845 - Flags: review?
Attachment #467845 - Flags: review? → review?(benjamin)
Comment on attachment 467845 [details] [diff] [review]
Add graphics card vendor and device ids to crash reports

You haven't arranged for anyone to call this method? Maybe missed a file in qref?

With AnnotateCrashReport, you'll need socorro database changes in order to see the results, which is not going to be quick. Assuming you want this data right away, I think you should use the comment method.
Attachment #467845 - Attachment is obsolete: true
Attachment #468857 - Flags: review?(benjamin)
Attachment #467845 - Flags: review?(benjamin)
Attachment #468857 - Flags: review?(benjamin) → review+
Attached patch Actually annotate at startup (obsolete) — Splinter Review
I assumed services were started at startup instead of at the first use. This patch initializes GfxInfo during gfxPlatform initialization.
Attachment #470146 - Flags: review?(gavin.sharp)
Comment on attachment 470146 [details] [diff] [review]
Actually annotate at startup

>diff --git a/gfx/thebes/gfxPlatform.cpp b/gfx/thebes/gfxPlatform.cpp

>+    /* Initialize the GfxInfo service */
>+    nsCOMPtr<nsIGfxInfo> gfxInfo;
>+    gfxInfo = do_GetService("@mozilla.org/gfx/info;1");
>+    if (gfxInfo)
>+        gfxInfo->AddCrashReportAnnotations();

Maybe you should put a comment about not calling GetD2DEnabled and GetDwriteEnabled here (and in AddCrashReportAnnotations?) just in case. And reference the followup.

>diff --git a/widget/src/windows/GfxInfo.cpp b/widget/src/windows/GfxInfo.cpp

>+/* GetD2DEnabled and GetDwriteEnabled shouldn't be called until after gfxPlatform initialization
>+ * has occured because they depend on it for information */

occurred

>@@ -232,9 +234,6 @@ void GfxInfo::Init()

>-  AddCrashReportAnnotations();

I think I would prefer leaving this here and just commenting about the fact that instantiation causes annotations to be added next to the getService().
Attached patch Actually annotate at startup v2 (obsolete) — Splinter Review
Address review comments.
Attachment #470146 - Attachment is obsolete: true
Attachment #470149 - Flags: review?(gavin.sharp)
Attachment #470146 - Flags: review?(gavin.sharp)
Comment on attachment 470149 [details] [diff] [review]
Actually annotate at startup v2

>diff --git a/widget/public/Makefile.in b/widget/public/Makefile.in

>+		nsIGfxInfo.idl \

>-		nsIGfxInfo.idl \

This doesn't seem worth doing until we actually have other implementations.

>diff --git a/widget/src/windows/GfxInfo.cpp b/widget/src/windows/GfxInfo.cpp

>@@ -232,9 +234,6 @@ void GfxInfo::Init()

>-  AddCrashReportAnnotations();

Need to revert this change too.
Attachment #470149 - Flags: review?(gavin.sharp) → review+
Finished version
Attachment #470149 - Attachment is obsolete: true
Depends on: 591576
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1283011877.1283012858.6022.gz
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1283011877.1283012651.5031.gz

make[7]: Entering directory `/e/builds/slave/comm-central-trunk-win32/build/objdir/mozilla/widget/src/build'
link -NOLOGO -DLL -OUT:gkwidget.dll -PDB:gkwidget.pdb -SUBSYSTEM:WINDOWS  nsWinWidgetFactory.obj    widget.res -NXCOMPAT -DYNAMICBASE -SAFESEH  -DEBUG -DEBUGTYPE:CV -MANIFEST:NO -LIBPATH:"e:/builds/slave/comm-central-trunk-win32/build/objdir/mozilla/memory/jemalloc/crtsrc/build/intel" -NODEFAULTLIB:msvcrt -NODEFAULTLIB:msvcrtd -NODEFAULTLIB:msvcprt -NODEFAULTLIB:msvcprtd -DEFAULTLIB:mozcrt19 -DEFAULTLIB:mozcpp19 -DEBUG -OPT:REF   -IMPLIB:fake.lib  @../windows/widget_windows.lib.fake @../xpwidgets/xpwidgets_s.lib.fake   ../../../dist/lib/gkgfx.lib ../../../dist/lib/thebes.lib e:/builds/slave/comm-central-trunk-win32/build/objdir/mozilla/dist/lib/xpcom.lib e:/builds/slave/comm-central-trunk-win32/build/objdir/mozilla/dist/lib/xpcom_core.lib e:/builds/slave/comm-central-trunk-win32/build/objdir/mozilla/dist/lib/mozalloc.lib e:/builds/slave/comm-central-trunk-win32/build/objdir/mozilla/dist/lib/nspr4.lib e:/builds/slave/comm-central-trunk-win32/build/objdir/mozilla/dist/lib/plc4.lib e:/builds/slave/comm-central-trunk-win32/build/objdir/mozilla/dist/lib/plds4.lib  e:/builds/slave/comm-central-trunk-win32/build/objdir/mozilla/dist/lib/unicharutil_s.lib ../../../gfx/qcms/mozqcms.lib   kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib uuid.lib ole32.lib oleaut32.lib comctl32.lib comdlg32.lib shell32.lib imm32.lib shlwapi.lib winspool.lib msimg32.lib  
   Creating library fake.lib and object fake.exp
GfxInfo.obj : error LNK2019: unresolved external symbol "unsigned int __cdecl CrashReporter::AppendAppNotesToCrashReport(class nsACString_internal const &)" (?AppendAppNotesToCrashReport@CrashReporter@@YAIABVnsACString_internal@@@Z) referenced in function "private: void __thiscall mozilla::widget::GfxInfo::AddCrashReportAnnotations(void)" (?AddCrashReportAnnotations@GfxInfo@widget@mozilla@@AAEXXZ)
GfxInfo.obj : error LNK2019: unresolved external symbol "unsigned int __cdecl CrashReporter::AnnotateCrashReport(class nsACString_internal const &,class nsACString_internal const &)" (?AnnotateCrashReport@CrashReporter@@YAIABVnsACString_internal@@0@Z) referenced in function "private: void __thiscall mozilla::widget::GfxInfo::AddCrashReportAnnotations(void)" (?AddCrashReportAnnotations@GfxInfo@widget@mozilla@@AAEXXZ)
gkwidget.dll : fatal error LNK1120: 2 unresolved externals
(In reply to comment #17)

Can you try out the patch in #591576?
I was checking the crash reporter results and noticed they seemed a little funny. It turns out we were printing the addresses of the ids instead of the values. This patch fixes that and improves the formatting of the app notes a little bit.
Attachment #470366 - Flags: review?(benjamin)
Comment on attachment 470366 [details] [diff] [review]
Fix reporting of device ID

Since we're here, can you call AppendAppNotesToCrashReport just once, and also add a newline at the end so that other notes don't collide?
Attachment #470366 - Flags: review?(benjamin) → review+
Blocks: 589265
http://hg.mozilla.org/mozilla-central/rev/c8aa01bfccb2
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
just wanted to reference another crashed bug that spiked in beta5 when testpilot tried to gather graphics card info.   

https://bugzilla.mozilla.org/show_bug.cgi?id=594608#c14

As a precaution we should be on the look out for any similar crashes or tickling of any similar bugs that test pilot might have stumbled upon.   Are there any test cases associated with this check-in, or any good ways to test?
Having a test that loads about:support would probably help.
Blocks: 596720
No longer blocks: 575738
You need to log in before you can comment on or make changes to this bug.