Bug 1112828 - Have GfxInfo::LogFailure use gfxCriticalLog and entries from gfxCriticalLog be available in about:support graphics section. r=jmuizelaar

gfx/2d/Logging.h

@@ -9,6 +9,7 @@
 #include <string>
 #include <sstream>
 #include <stdio.h>
+#include <vector>
 
 #ifdef MOZ_LOGGING
 #include <prlog.h>
@@ -206,6 +207,12 @@ class LogForwarder {
 public:
   virtual ~LogForwarder() {}
   virtual void Log(const std::string &aString) = 0;
+
+  // Provide a copy of the logs to the caller.  The int is the index
+  // of the Log call, if the number of logs exceeds some preset capacity
+  // we may not get all of them, so the indices help figure out which
+  // ones we did save.
+  virtual std::vector<std::pair<int32_t,std::string> > StringsVectorCopy() = 0;
 };
 
 class NoLog

gfx/thebes/gfxPlatform.cpp

@@ -169,6 +169,8 @@ public:
   explicit CrashStatsLogForwarder(const char* aKey);
   virtual void Log(const std::string& aString) MOZ_OVERRIDE;
 
+  virtual std::vector<std::pair<int32_t,std::string> > StringsVectorCopy();
+
   void SetCircularBufferSize(uint32_t aCapacity);
 
 private:
@@ -201,6 +203,13 @@ void CrashStatsLogForwarder::SetCircularBufferSize(uint32_t aCapacity)
   mBuffer.reserve(static_cast<size_t>(aCapacity));
 }
 
+std::vector<std::pair<int32_t,std::string> >
+CrashStatsLogForwarder::StringsVectorCopy()
+{
+  MutexAutoLock lock(mMutex);
+  return mBuffer;
+}
+
 bool
 CrashStatsLogForwarder::UpdateStringsVector(const std::string& aString)
 {

toolkit/content/aboutSupport.js

@@ -177,10 +177,32 @@ let snapshotFormatters = {
 
     // graphics-failures-tbody tbody
     if ("failures" in data) {
-      $.append($("graphics-failures-tbody"), data.failures.map(function (val) {
-        return $.new("tr", [$.new("td", val)]);
-      }));
-      delete data.failures;
+      // If indices is there, it should be the same length as failures,
+      // (see Troubleshoot.jsm) but we check anyway:
+      if ("indices" in data && data.failures.length == data.indices.length) {
+        let combined = [];
+        for (let i = 0; i < data.failures.length; i++) {
+          let assembled = assembleFromGraphicsFailure(i, data);
+          combined.push(assembled);
+        }
+        combined.sort(function(a,b) {
+            if (a.index < b.index) return -1;
+            if (a.index > b.index) return 1;
+            return 0;});
+        $.append($("graphics-failures-tbody"),
+                 combined.map(function(val) {
+                   return $.new("tr", [$.new("th", val.header, "column"),
+                                       $.new("td", val.message)]);
+                 }));
+      } else {
+        $.append($("graphics-failures-tbody"),
+          [$.new("tr", [$.new("th", "LogFailure", "column"),
+                        $.new("td", data.failures.map(function (val) {
+                          return $.new("p", val);
+                       }))])]);
+      }
+
+	delete data.failures;
     }
 
     // graphics-tbody tbody
@@ -357,6 +379,32 @@ function stringBundle() {
            "chrome://global/locale/aboutSupport.properties");
 }
 
+function assembleFromGraphicsFailure(i, data)
+{
+  // Only cover the cases we have today; for example, we do not have
+  // log failures that assert and we assume the log level is 1/error.
+  let message = data.failures[i];
+  let index = data.indices[i];
+  let what = "";
+  if (message.search(/\[GFX1-\]: \(LF\)/) == 0) {
+    // Non-asserting log failure - the message is substring(14)
+    what = "LogFailure";
+    message = message.substring(14);
+  } else if (message.search(/\[GFX1-\]: /) == 0) {
+    // Non-asserting - the message is substring(9)
+    what = "Error";
+    message = message.substring(9);
+  } else if (message.search(/\[GFX1\]: /) == 0) {
+    // Asserting - the message is substring(8)
+    what = "Assert";
+    message = message.substring(8);
+  }
+  let assembled = {"index" : index,
+                   "header" : ("(#" + index + ") " + what),
+                   "message" : message};
+  return assembled;
+}
+
 function sortedArrayFromObject(obj) {
   let tuples = [];
   for (let prop in obj)

toolkit/modules/Troubleshoot.jsm

@@ -410,9 +410,16 @@ let dataProviders = {
     if (infoInfo)
       data.info = infoInfo;
 
-    let failures = gfxInfo.getFailures();
-    if (failures.length)
+    let failureCount = {};
+    let failureIndices = {};
+
+    let failures = gfxInfo.getFailures(failureCount, failureIndices);
+    if (failures.length) {
       data.failures = failures;
+      if (failureIndices.value.length == failures.length) {
+        data.indices = failureIndices.value;
+      }
+    }
 
     done(data);
   },

widget/GfxInfoBase.cpp

@@ -28,6 +28,8 @@
 #include "nsXULAppAPI.h"
 #include "mozilla/Preferences.h"
 #include "mozilla/dom/ContentChild.h"
+#include "mozilla/gfx/2D.h"
+#include "mozilla/gfx/Logging.h"
 
 #if defined(MOZ_CRASHREPORTER)
 #include "nsExceptionHandler.h"
@@ -88,6 +90,7 @@ void InitGfxDriverInfoShutdownObserver()
 }
 
 using namespace mozilla::widget;
+using namespace mozilla::gfx;
 using namespace mozilla;
 
 #ifdef XP_MACOSX
@@ -550,8 +553,7 @@ GfxInfoBase::Observe(nsISupports* aSubject, const char* aTopic,
 }
 
 GfxInfoBase::GfxInfoBase()
-    : mFailureCount(0)
-    , mMutex("GfxInfoBase")
+    : mMutex("GfxInfoBase")
 {
 }
 
@@ -888,43 +890,73 @@ GfxInfoBase::EvaluateDownloadedBlacklist(nsTArray<GfxDriverInfo>& aDriverInfo)
 NS_IMETHODIMP_(void)
 GfxInfoBase::LogFailure(const nsACString &failure)
 {
+  // gfxCriticalError has a mutex lock of its own, so we may not actually
+  // need this lock. ::GetFailures() accesses the data but the LogForwarder
+  // will not return the copy of the logs unless it can get the same lock
+  // that gfxCriticalError uses.  Still, that is so much of an implementation
+  // detail that it's nicer to just add an extra lock here and in ::GetFailures()
   MutexAutoLock lock(mMutex);
-  /* We only keep the first 9 failures */
-  if (mFailureCount < ArrayLength(mFailures)) {
-    mFailures[mFailureCount++] = failure;
-
-    /* record it in the crash notes too */
-    #if defined(MOZ_CRASHREPORTER)
-      CrashReporter::AppendAppNotesToCrashReport(failure);
-    #endif
-  }
 
+  // By default, gfxCriticalError asserts; make it not assert in this case.
+  gfxCriticalError(CriticalLog::DefaultOptions(false)) << "(LF) " << failure.BeginReading();
 }
 
-/* void getFailures ([optional] out unsigned long failureCount, [array, size_is (failureCount), retval] out string failures); */
+/* void getFailures (out unsigned long failureCount, [optional, array, size_is (failureCount)] out long indices, [array, size_is (failureCount), retval] out string failures); */
 /* XPConnect method of returning arrays is very ugly. Would not recommend. Fallable nsMemory::Alloc makes things worse */
-NS_IMETHODIMP GfxInfoBase::GetFailures(uint32_t *failureCount, char ***failures)
+NS_IMETHODIMP GfxInfoBase::GetFailures(uint32_t* failureCount,
+				       int32_t** indices,
+				       char ***failures)
 {
+  MutexAutoLock lock(mMutex);
 
   NS_ENSURE_ARG_POINTER(failureCount);
   NS_ENSURE_ARG_POINTER(failures);
 
   *failures = nullptr;
-  *failureCount = mFailureCount;
+  *failureCount = 0;
+
+  // indices is "allowed" to be null, the caller may not care about them,
+  // although calling from JS doesn't seem to get us there.
+  if (indices) *indices = nullptr;
+
+  LogForwarder* logForwarder = Factory::GetLogForwarder();
+  if (!logForwarder) {
+    return NS_ERROR_UNEXPECTED;
+  }
+
+  // There are two stirng copies in this method, starting with this one. We are
+  // assuming this is not a big deal, as the size of the array should be small
+  // and the strings in it should be small as well (the error messages in the
+  // code.)  The second copy happens with the Clone() calls.  Technically,
+  // we don't need the mutex lock after the StringVectorCopy() call.
+  std::vector<std::pair<int32_t,std::string> > loggedStrings = logForwarder->StringsVectorCopy();
+  *failureCount = loggedStrings.size();
 
   if (*failureCount != 0) {
     *failures = (char**)nsMemory::Alloc(*failureCount * sizeof(char*));
-    if (!failures)
+    if (!(*failures)) {
       return NS_ERROR_OUT_OF_MEMORY;
+    }
+    if (indices) {
+      *indices = (int32_t*)nsMemory::Alloc(*failureCount * sizeof(int32_t));
+      if (!(*indices)) {
+        nsMemory::Free(*failures);
+        *failures = nullptr;
+        return NS_ERROR_OUT_OF_MEMORY;
+      }
+    }
 
     /* copy over the failure messages into the array we just allocated */
-    for (uint32_t i = 0; i < *failureCount; i++) {
-      nsCString& flattenedFailureMessage(mFailures[i]);
-      (*failures)[i] = (char*)nsMemory::Clone(flattenedFailureMessage.get(), flattenedFailureMessage.Length() + 1);
+    std::vector<std::pair<int32_t, std::string> >::const_iterator it;
+    uint32_t i=0;
+    for(it = loggedStrings.begin() ; it != loggedStrings.end(); ++it, i++) {
+      (*failures)[i] = (char*)nsMemory::Clone((*it).second.c_str(), (*it).second.size() + 1);
+      if (indices) (*indices)[i] = (*it).first;
 
       if (!(*failures)[i]) {
         /* <sarcasm> I'm too afraid to use an inline function... </sarcasm> */
         NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(i, (*failures));
+	*failureCount = i;
         return NS_ERROR_OUT_OF_MEMORY;
       }
     }

widget/GfxInfoBase.h

@@ -54,7 +54,7 @@ public:
   NS_IMETHOD GetFeatureSuggestedDriverVersion(int32_t aFeature, nsAString & _retval) MOZ_OVERRIDE;
   NS_IMETHOD GetWebGLParameter(const nsAString & aParam, nsAString & _retval) MOZ_OVERRIDE;
 
-  NS_IMETHOD GetFailures(uint32_t *failureCount, char ***failures) MOZ_OVERRIDE;
+    NS_IMETHOD GetFailures(uint32_t *failureCount, int32_t** indices, char ***failures) MOZ_OVERRIDE;
   NS_IMETHOD_(void) LogFailure(const nsACString &failure) MOZ_OVERRIDE;
   NS_IMETHOD GetInfo(JSContext*, JS::MutableHandle<JS::Value>) MOZ_OVERRIDE;
 
@@ -103,8 +103,6 @@ private:
 
   void EvaluateDownloadedBlacklist(nsTArray<GfxDriverInfo>& aDriverInfo);
 
-  nsCString mFailures[9]; // The choice of 9 is Ehsan's
-  uint32_t mFailureCount;
   Mutex mMutex;
 
 };

widget/nsIGfxInfo.idl

@@ -55,7 +55,8 @@ interface nsIGfxInfo : nsISupports
   readonly attribute boolean isGPU2Active;
 
   void getFailures(
-               [optional] out unsigned long failureCount,
+               out unsigned long failureCount,
+               [optional, array, size_is(failureCount)] out long indices,
                [retval, array, size_is(failureCount)] out string failures);
 
   [noscript, notxpcom] void logFailure(in ACString failure);