Bug 1348273 - Convert the graphics annotations; r?jrmuizel draft
authorGabriele Svelto <gsvelto@mozilla.com>
Fri, 16 Mar 2018 23:57:43 +0100
changeset 781400 79ffbdccdf6e2ca5298dfcc33f9e089e9c9bc46b
parent 781399 c14726b2d1cf63b2a52f37de30682227742a659a
child 781401 fa7f940dadf3f5a68015ac64ef3f7e74ef5dfef2
push id106292
push usergsvelto@mozilla.com
push dateThu, 12 Apr 2018 22:06:23 +0000
reviewersjrmuizel
bugs1348273
milestone61.0a1
Bug 1348273 - Convert the graphics annotations; r?jrmuizel This patch also removes some crash notes whose contents are already gathered through annotations and surfaced via Socorro. MozReview-Commit-ID: L5tDNEgRLw0
gfx/ipc/GPUProcessManager.cpp
gfx/src/DriverCrashGuard.cpp
gfx/thebes/DeviceManagerDx.cpp
gfx/thebes/gfxPlatform.cpp
toolkit/components/gfx/SanityTest.js
toolkit/crashreporter/CrashAnnotations.yaml
widget/android/GfxInfo.cpp
widget/cocoa/GfxInfo.mm
widget/windows/GfxInfo.cpp
--- a/gfx/ipc/GPUProcessManager.cpp
+++ b/gfx/ipc/GPUProcessManager.cpp
@@ -369,22 +369,21 @@ GPUProcessManager::OnProcessLaunchComple
     DisableGPUProcess("Failed to create PVsyncBridge endpoints");
     return;
   }
 
   mVsyncBridge = VsyncBridgeChild::Create(mVsyncIOThread, mProcessToken, Move(vsyncChild));
   mGPUChild->SendInitVsyncBridge(Move(vsyncParent));
 
   CrashReporter::AnnotateCrashReport(
-    NS_LITERAL_CSTRING("GPUProcessStatus"),
-    NS_LITERAL_CSTRING("Running"));
+    CrashReporter::Annotation::GPUProcessStatus, NS_LITERAL_CSTRING("Running"));
 
   CrashReporter::AnnotateCrashReport(
-    NS_LITERAL_CSTRING("GPUProcessLaunchCount"),
-    nsPrintfCString("%d", mNumProcessAttempts));
+    CrashReporter::Annotation::GPUProcessLaunchCount,
+    static_cast<int>(mNumProcessAttempts));
 }
 
 static bool
 ShouldLimitDeviceResets(uint32_t count, int32_t deltaMilliseconds)
 {
   // We decide to limit by comparing the amount of resets that have happened
   // and time since the last reset to two prefs.
   int32_t timeLimit = gfxPrefs::DeviceResetThresholdMilliseconds();
@@ -716,17 +715,17 @@ GPUProcessManager::DestroyProcess()
   mProcess = nullptr;
   mGPUChild = nullptr;
   if (mVsyncBridge) {
     mVsyncBridge->Close();
     mVsyncBridge = nullptr;
   }
 
   CrashReporter::AnnotateCrashReport(
-    NS_LITERAL_CSTRING("GPUProcessStatus"),
+    CrashReporter::Annotation::GPUProcessStatus,
     NS_LITERAL_CSTRING("Destroyed"));
 }
 
 already_AddRefed<CompositorSession>
 GPUProcessManager::CreateTopLevelCompositor(nsBaseWidget* aWidget,
                                             LayerManager* aLayerManager,
                                             CSSToLayoutDeviceScale aScale,
                                             const CompositorOptions& aOptions,
--- a/gfx/src/DriverCrashGuard.cpp
+++ b/gfx/src/DriverCrashGuard.cpp
@@ -160,19 +160,18 @@ DriverCrashGuard::~DriverCrashGuard()
     // proceed to mark the status as okay.
     if (GetStatus() != DriverInitStatus::Crashed) {
       SetStatus(DriverInitStatus::Okay);
     }
   } else {
     dom::ContentChild::GetSingleton()->SendEndDriverCrashGuard(uint32_t(mType));
   }
 
-  // Remove the crash report annotation.
-  CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("GraphicsStartupTest"),
-                                     NS_LITERAL_CSTRING(""));
+  CrashReporter::RemoveCrashReportAnnotation(
+    CrashReporter::Annotation::GraphicsStartupTest);
 }
 
 bool
 DriverCrashGuard::Crashed()
 {
   InitializeIfNeeded();
 
   // Note, we read mCrashDetected instead of GetStatus(), since in child
@@ -205,18 +204,18 @@ void
 DriverCrashGuard::ActivateGuard()
 {
   mGuardActivated = true;
 
   // Anotate crash reports only if we're a real guard. Otherwise, we could
   // attribute a random parent process crash to a graphics problem in a child
   // process.
   if (mMode != Mode::Proxy) {
-    CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("GraphicsStartupTest"),
-                                       NS_LITERAL_CSTRING("1"));
+    CrashReporter::AnnotateCrashReport(
+      CrashReporter::Annotation::GraphicsStartupTest, true);
   }
 
   // If we're in the content process, the rest of the guarding is handled
   // in the parent.
   if (XRE_IsContentProcess()) {
     return;
   }
 
--- a/gfx/thebes/DeviceManagerDx.cpp
+++ b/gfx/thebes/DeviceManagerDx.cpp
@@ -879,20 +879,18 @@ DeviceManagerDx::MaybeResetAndReacquireD
   if (!HasDeviceReset(&resetReason)) {
     return false;
   }
 
   if (resetReason != DeviceResetReason::FORCED_RESET) {
     Telemetry::Accumulate(Telemetry::DEVICE_RESET_REASON, uint32_t(resetReason));
   }
 
-  nsPrintfCString reasonString("%d", int(resetReason));
   CrashReporter::AnnotateCrashReport(
-    NS_LITERAL_CSTRING("DeviceResetReason"),
-    reasonString);
+    CrashReporter::Annotation::DeviceResetReason, int(resetReason));
 
   bool createCompositorDevice = !!mCompositorDevice;
   bool createContentDevice = !!mContentDevice;
 
   ResetDevices();
 
   if (createCompositorDevice && !CreateCompositorDevices()) {
     // Just stop, don't try anything more
--- a/gfx/thebes/gfxPlatform.cpp
+++ b/gfx/thebes/gfxPlatform.cpp
@@ -190,38 +190,38 @@ public:
 /// if the capacity set with the method below is >= 2.  We always retain the
 /// very first critical message, and the latest capacity-1 messages are
 /// rotated through. Note that we don't expect the total number of times
 /// this gets called to be large - it is meant for critical errors only.
 
 class CrashStatsLogForwarder: public mozilla::gfx::LogForwarder
 {
 public:
-  explicit CrashStatsLogForwarder(const char* aKey);
+  explicit CrashStatsLogForwarder(CrashReporter::Annotation aKey);
   void Log(const std::string& aString) override;
   void CrashAction(LogReason aReason) override;
   bool UpdateStringsVector(const std::string& aString) override;
 
   LoggingRecord LoggingRecordCopy() override;
 
   void SetCircularBufferSize(uint32_t aCapacity);
 
 private:
   // Helper for the Log()
   void UpdateCrashReport();
 
 private:
   LoggingRecord mBuffer;
-  nsCString mCrashCriticalKey;
+  CrashReporter::Annotation mCrashCriticalKey;
   uint32_t mMaxCapacity;
   int32_t mIndex;
   Mutex mMutex;
 };
 
-CrashStatsLogForwarder::CrashStatsLogForwarder(const char* aKey)
+CrashStatsLogForwarder::CrashStatsLogForwarder(CrashReporter::Annotation aKey)
   : mBuffer()
   , mCrashCriticalKey(aKey)
   , mMaxCapacity(0)
   , mIndex(-1)
   , mMutex("CrashStatsLogForwarder")
 {
 }
 
@@ -289,21 +289,23 @@ void CrashStatsLogForwarder::UpdateCrash
     break;
   }
 
   for (auto& it : mBuffer) {
     message << logAnnotation << Get<0>(it) << "]" << Get<1>(it) << " (t=" << Get<2>(it) << ") ";
   }
 
   nsCString reportString(message.str().c_str());
-  nsresult annotated = CrashReporter::AnnotateCrashReport(mCrashCriticalKey, reportString);
+  nsresult annotated = CrashReporter::AnnotateCrashReport(mCrashCriticalKey,
+                                                          reportString);
 
   if (annotated != NS_OK) {
     printf("Crash Annotation %s: %s",
-           mCrashCriticalKey.get(), message.str().c_str());
+           CrashReporter::AnnotationToString(mCrashCriticalKey),
+           message.str().c_str());
   }
 }
 
 class LogForwarderEvent : public Runnable
 {
   ~LogForwarderEvent() override = default;
 
 public:
@@ -896,17 +898,18 @@ gfxPlatform::MaxAllocSize()
   // pref or whatnot.
   const int32_t kMinAllocPref = 10000000;
   return std::max(kMinAllocPref, gfxPrefs::MaxAllocSizeDoNotUseDirectly());
 }
 
 /* static */ void
 gfxPlatform::InitMoz2DLogging()
 {
-  auto fwd = new CrashStatsLogForwarder("GraphicsCriticalError");
+  auto fwd = new CrashStatsLogForwarder(
+    CrashReporter::Annotation::GraphicsCriticalError);
   fwd->SetCircularBufferSize(gfxPrefs::GfxLoggingCrashLength());
 
   mozilla::gfx::Config cfg;
   cfg.mLogForwarder = fwd;
   cfg.mMaxTextureSize = gfxPlatform::MaxTextureSize();
   cfg.mMaxAllocSize = gfxPlatform::MaxAllocSize();
 
   gfx::Factory::Init(cfg);
--- a/toolkit/components/gfx/SanityTest.js
+++ b/toolkit/components/gfx/SanityTest.js
@@ -80,17 +80,18 @@ function reportTestReason(val) {
   histogram.add(val);
 }
 
 function annotateCrashReport(value) {
   try {
     // "1" if we're annotating the crash report, "" to remove the annotation.
     var crashReporter = Cc["@mozilla.org/toolkit/crash-reporter;1"].
                           getService(Ci.nsICrashReporter);
-    crashReporter.annotateCrashReport("GraphicsSanityTest", value ? "1" : "");
+    crashReporter.annotateCrashReport(crashReporter.GraphicsSanityTest,
+                                      value ? "1" : "");
   } catch (e) {
   }
 }
 
 function setTimeout(aMs, aCallback) {
   var timer = Cc["@mozilla.org/timer;1"].
                 createInstance(Ci.nsITimer);
   timer.initWithCallback(aCallback, aMs, Ci.nsITimer.TYPE_ONE_SHOT);
--- a/toolkit/crashreporter/CrashAnnotations.yaml
+++ b/toolkit/crashreporter/CrashAnnotations.yaml
@@ -34,16 +34,36 @@ AccessibilityInProcClient:
     accessible/windows/msaa/Compatibility.h for the mappings.
   type: string
 
 ActualStreamLen:
   description: >
     Actual length of an IPC proxy stream.
   type: integer
 
+AdapterDeviceID:
+  description: >
+    Graphics adapter name.
+  type: string
+
+AdapterDriverVersion:
+  description: >
+    Graphics adapter driver version.
+  type: string
+
+AdapterSubsysID:
+  description: >
+    Graphics adapter subsystem ID.
+  type: string
+
+AdapterVendorID:
+  description: >
+    Graphics adapter vendor name.
+  type: string
+
 additional_minidumps:
   description: >
     Comma separated list of additional minidumps for this crash, each element
     in the list represent the suffix used in the dump filename. E.g. the
     "browser" entry for crash fa909194-737b-4b93-b8da-da110ac785e0 implies the
     existence of the fa909194-737b-4b93-b8da-da110ac785e0-browser.dmp file.
   type: string
 
@@ -163,16 +183,21 @@ CrashTime:
   ping: true
 
 CreateStreamOnHGlobalFailure:
   description: >
     Set when failing to obtain a global memory handle during the creation of an
     IPC proxy stream.
   type: string
 
+DeviceResetReason:
+  description: >
+    Reason why a DirectX device has been reset, Windows only.
+  type: string
+
 DOMIPCEnabled:
   description: >
     Set to 1 when a tab is running in a content process
   type: boolean
 
 EventLoopNestingLevel:
   description: >
     Present only if higher than 0, indicates that we're running in a nested
@@ -192,16 +217,41 @@ FlashProcessDump:
   type: string
 
 GetHGlobalFromStreamFailure:
   description: >
     Error returned when invoking GetHGlobalFromStreamFailure() during the
     creation of an IPC stream proxy.
   type: string
 
+GPUProcessLaunchCount:
+  description: >
+    Number of times the GPU process was launched.
+  type: integer
+
+GPUProcessStatus:
+  description: >
+    Status of the GPU process, can be set to "Running" or "Destroyed"
+  type: string
+
+GraphicsCriticalError:
+  description: >
+    Information of a critical error that occurred within the graphics code.
+  type: string
+
+GraphicsSanityTest:
+  description: >
+    Annotation used in tests.
+  type: string
+
+GraphicsStartupTest:
+  description: >
+    Set to 1 by the graphics driver crash guard when it's activated.
+  type: boolean
+
 HangMonitorDescription:
   description: >
     Name of the hang monitor that generated the crash.
   type: string
 
 IAccessibleConfig:
   description: >
     Set when something is seriously wrong with the IAccessible configuration in
@@ -301,17 +351,16 @@ IPCTransportFailureReason:
 
 IsGarbageCollecting:
   description: >
     If true then the JavaScript garbage collector was running when the crash
     occurred.
   type: boolean
   ping: true
 
-
 JavaStackTrace:
   description: >
     Java stack trace, only present on Firefox for Android if we encounter an
     uncaught Java exception.
   type: string
 
 MarshalActCtxManifestPath:
   description: >
--- a/widget/android/GfxInfo.cpp
+++ b/widget/android/GfxInfo.cpp
@@ -343,29 +343,22 @@ GfxInfo::GetIsGPU2Active(bool* aIsGPU2Ac
 {
   EnsureInitialized();
   return NS_ERROR_FAILURE;
 }
 
 void
 GfxInfo::AddCrashReportAnnotations()
 {
-  CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("AdapterVendorID"),
+  CrashReporter::AnnotateCrashReport(CrashReporter::Annotation::AdapterVendorID,
                                      mGLStrings->Vendor());
-  CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("AdapterDeviceID"),
+  CrashReporter::AnnotateCrashReport(CrashReporter::Annotation::AdapterDeviceID,
                                      mGLStrings->Renderer());
-  CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("AdapterDriverVersion"),
-                                     mGLStrings->Version());
-
-  /* Add an App Note for now so that we get the data immediately. These
-   * can go away after we store the above in the socorro db */
-  nsAutoCString note;
-  note.AppendPrintf("AdapterDescription: '%s'\n", mAdapterDescription.get());
-
-  CrashReporter::AppendAppNotesToCrashReport(note);
+  CrashReporter::AnnotateCrashReport(
+    CrashReporter::Annotation::AdapterDriverVersion, mGLStrings->Version());
 }
 
 const nsTArray<GfxDriverInfo>&
 GfxInfo::GetGfxDriverInfo()
 {
   if (mDriverInfo->IsEmpty()) {
     APPEND_TO_DRIVER_BLOCKLIST2(OperatingSystem::Android,
       (nsAString&) GfxDriverInfo::GetDeviceVendor(VendorAll), GfxDriverInfo::allDevices,
--- a/widget/cocoa/GfxInfo.mm
+++ b/widget/cocoa/GfxInfo.mm
@@ -278,31 +278,22 @@ GfxInfo::AddCrashReportAnnotations()
 
   GetAdapterDeviceID(deviceID);
   CopyUTF16toUTF8(deviceID, narrowDeviceID);
   GetAdapterVendorID(vendorID);
   CopyUTF16toUTF8(vendorID, narrowVendorID);
   GetAdapterDriverVersion(driverVersion);
   CopyUTF16toUTF8(driverVersion, narrowDriverVersion);
 
-  CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("AdapterVendorID"),
+  CrashReporter::AnnotateCrashReport(CrashReporter::Annotation::AdapterVendorID,
                                      narrowVendorID);
-  CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("AdapterDeviceID"),
+  CrashReporter::AnnotateCrashReport(CrashReporter::Annotation::AdapterDeviceID,
                                      narrowDeviceID);
-  CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("AdapterDriverVersion"),
-                                     narrowDriverVersion);
-  /* Add an App Note for now so that we get the data immediately. These
-   * can go away after we store the above in the socorro db */
-  nsAutoCString note;
-  /* AppendPrintf only supports 32 character strings, mrghh. */
-  note.AppendLiteral("AdapterVendorID: ");
-  note.Append(narrowVendorID);
-  note.AppendLiteral(", AdapterDeviceID: ");
-  note.Append(narrowDeviceID);
-  CrashReporter::AppendAppNotesToCrashReport(note);
+  CrashReporter::AnnotateCrashReport(
+    CrashReporter::Annotation::AdapterDriverVersion, narrowDriverVersion);
 }
 
 // We don't support checking driver versions on Mac.
 #define IMPLEMENT_MAC_DRIVER_BLOCKLIST(os, vendor, device, features, blockOn, ruleId) \
   APPEND_TO_DRIVER_BLOCKLIST(os, vendor, device, features, blockOn,           \
                              DRIVER_COMPARISON_IGNORED, V(0,0,0,0), ruleId, "")
 
 
--- a/widget/windows/GfxInfo.cpp
+++ b/widget/windows/GfxInfo.cpp
@@ -888,45 +888,34 @@ GfxInfo::AddCrashReportAnnotations()
   CopyUTF16toUTF8(deviceID, narrowDeviceID);
   GetAdapterVendorID(vendorID);
   CopyUTF16toUTF8(vendorID, narrowVendorID);
   GetAdapterDriverVersion(driverVersion);
   CopyUTF16toUTF8(driverVersion, narrowDriverVersion);
   GetAdapterSubsysID(subsysID);
   CopyUTF16toUTF8(subsysID, narrowSubsysID);
 
-  CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("AdapterVendorID"),
+  CrashReporter::AnnotateCrashReport(CrashReporter::Annotation::AdapterVendorID,
                                      narrowVendorID);
-  CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("AdapterDeviceID"),
+  CrashReporter::AnnotateCrashReport(CrashReporter::Annotation::AdapterDeviceID,
                                      narrowDeviceID);
-  CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("AdapterDriverVersion"),
-                                     narrowDriverVersion);
-  CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("AdapterSubsysID"),
+  CrashReporter::AnnotateCrashReport(
+    CrashReporter::Annotation::AdapterDriverVersion, narrowDriverVersion);
+  CrashReporter::AnnotateCrashReport(CrashReporter::Annotation::AdapterSubsysID,
                                      narrowSubsysID);
 
-  /* Add an App Note for now so that we get the data immediately. These
-   * can go away after we store the above in the socorro db */
+  /* Add an App Note, this contains extra information. */
   nsAutoCString note;
-  /* AppendPrintf only supports 32 character strings, mrghh. */
-  note.AppendLiteral("AdapterVendorID: ");
-  note.Append(narrowVendorID);
-  note.AppendLiteral(", AdapterDeviceID: ");
-  note.Append(narrowDeviceID);
-  note.AppendLiteral(", AdapterSubsysID: ");
-  note.Append(narrowSubsysID);
-  note.AppendLiteral(", AdapterDriverVersion: ");
-  note.Append(NS_LossyConvertUTF16toASCII(driverVersion));
 
+  // TODO: We should probably convert this into a proper annotation
   if (vendorID == GfxDriverInfo::GetDeviceVendor(VendorAll)) {
     /* if we didn't find a valid vendorID lets append the mDeviceID string to try to find out why */
-    note.AppendLiteral(", ");
     LossyAppendUTF16toASCII(mDeviceID[mActiveGPUIndex], note);
     note.AppendLiteral(", ");
     LossyAppendUTF16toASCII(mDeviceKeyDebug, note);
-    LossyAppendUTF16toASCII(mDeviceKeyDebug, note);
   }
   note.AppendLiteral("\n");
 
   if (mHasDualGPU) {
     nsString deviceID2, vendorID2, subsysID2;
     nsAutoString adapterDriverVersionString2;
     nsCString narrowDeviceID2, narrowVendorID2, narrowSubsysID2;