Ted, what is the best way to get data included in crash reports?

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+.

(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.

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.

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.

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.

I assumed services were started at startup instead of at the first use. This patch initializes GfxInfo during gfxPlatform initialization.

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

Address review comments.

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.

Finished version

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.

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?

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.