Bug 1112828 - Have GfxInfo::LogFailure use gfxCriticalLog and entries from gfxCriticalLog be available in about:support graphics section. r=jmuizelaar
authorMilan Sreckovic <milan@mozilla.com>
Tue, 13 Jan 2015 21:19:25 -0500
changeset 251112 09c23f8dea868b7b93d7c23a3e1d1bc589b565cf
parent 251111 b035d2c566ad7552410eff92ea9acac1aede59a0
child 251113 e936da5c4ceebabd0d785d0470a4d2ed819d0022
push id4610
push userjlund@mozilla.com
push dateMon, 30 Mar 2015 18:32:55 +0000
treeherdermozilla-beta@4df54044d9ef [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjmuizelaar
bugs1112828
milestone38.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1112828 - Have GfxInfo::LogFailure use gfxCriticalLog and entries from gfxCriticalLog be available in about:support graphics section. r=jmuizelaar
gfx/2d/Logging.h
gfx/thebes/gfxPlatform.cpp
toolkit/content/aboutSupport.js
toolkit/modules/Troubleshoot.jsm
widget/GfxInfoBase.cpp
widget/GfxInfoBase.h
widget/nsIGfxInfo.idl
--- a/gfx/2d/Logging.h
+++ b/gfx/2d/Logging.h
@@ -4,16 +4,17 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef MOZILLA_GFX_LOGGING_H_
 #define MOZILLA_GFX_LOGGING_H_
 
 #include <string>
 #include <sstream>
 #include <stdio.h>
+#include <vector>
 
 #ifdef MOZ_LOGGING
 #include <prlog.h>
 #endif
 
 #if defined(MOZ_WIDGET_GONK) || defined(MOZ_WIDGET_ANDROID)
 #include "nsDebug.h"
 #endif
@@ -201,16 +202,22 @@ struct CriticalLogger {
 };
 
 // Implement this interface and init the Factory with an instance to
 // forward critical logs.
 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
 {
 public:
   NoLog() {}
   ~NoLog() {}
 
--- a/gfx/thebes/gfxPlatform.cpp
+++ b/gfx/thebes/gfxPlatform.cpp
@@ -164,16 +164,18 @@ public:
 /// 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);
   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:
   // Helpers for the Log()
   bool UpdateStringsVector(const std::string& aString);
   void UpdateCrashReport();
 
 private:
@@ -196,16 +198,23 @@ CrashStatsLogForwarder::CrashStatsLogFor
 void CrashStatsLogForwarder::SetCircularBufferSize(uint32_t aCapacity)
 {
   MutexAutoLock lock(mMutex);
 
   mMaxCapacity = 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)
 {
   // We want at least the first one and the last one.  Otherwise, no point.
   if (mMaxCapacity < 2) {
     return false;
   }
 
--- a/toolkit/content/aboutSupport.js
+++ b/toolkit/content/aboutSupport.js
@@ -172,20 +172,42 @@ let snapshotFormatters = {
         ]);
       });
       $.append($("graphics-info-properties"), trs);
       delete data.info;
     }
 
     // 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
 
     function localizedMsg(msgArray) {
       let nameOrMsg = msgArray.shift();
       if (msgArray.length) {
         // formatStringFromName logs an NS_ASSERTION failure otherwise that says
@@ -352,16 +374,42 @@ let $ = document.getElementById.bind(doc
   children.forEach(c => parent.appendChild(c));
 };
 
 function stringBundle() {
   return Services.strings.createBundle(
            "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)
     tuples.push([prop, obj[prop]]);
   tuples.sort(([prop1, v1], [prop2, v2]) => prop1.localeCompare(prop2));
   return tuples;
 }
 
--- a/toolkit/modules/Troubleshoot.jsm
+++ b/toolkit/modules/Troubleshoot.jsm
@@ -405,19 +405,26 @@ let dataProviders = {
 #endif
       data.webglRendererMessage = statusMsgForFeature(feature);
     }
 
     let infoInfo = gfxInfo.getInfo();
     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);
   },
 
   javaScript: function javaScript(done) {
     let data = {};
     let winEnumer = Services.ww.getWindowEnumerator();
     if (winEnumer.hasMoreElements())
--- a/widget/GfxInfoBase.cpp
+++ b/widget/GfxInfoBase.cpp
@@ -23,16 +23,18 @@
 #include "nsIDOMElement.h"
 #include "nsIDOMHTMLCollection.h"
 #include "nsIDOMNode.h"
 #include "nsIDOMNodeList.h"
 #include "nsTArray.h"
 #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"
 #endif
 
 using namespace mozilla::widget;
 using namespace mozilla;
 using mozilla::MutexAutoLock;
@@ -83,16 +85,17 @@ void InitGfxDriverInfoShutdownObserver()
     return;
   }
 
   ShutdownObserver *obs = new ShutdownObserver();
   observerService->AddObserver(obs, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false);
 }
 
 using namespace mozilla::widget;
+using namespace mozilla::gfx;
 using namespace mozilla;
 
 #ifdef XP_MACOSX
 NS_IMPL_ISUPPORTS(GfxInfoBase, nsIGfxInfo, nsIGfxInfo2, nsIObserver, nsISupportsWeakReference)
 #else
 NS_IMPL_ISUPPORTS(GfxInfoBase, nsIGfxInfo, nsIObserver, nsISupportsWeakReference)
 #endif
 
@@ -545,18 +548,17 @@ GfxInfoBase::Observe(nsISupports* aSubje
       }
     }
   }
 
   return NS_OK;
 }
 
 GfxInfoBase::GfxInfoBase()
-    : mFailureCount(0)
-    , mMutex("GfxInfoBase")
+    : mMutex("GfxInfoBase")
 {
 }
 
 GfxInfoBase::~GfxInfoBase()
 {
 }
 
 nsresult
@@ -883,53 +885,83 @@ GfxInfoBase::EvaluateDownloadedBlacklist
 
     ++i;
   }
 }
 
 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;
       }
     }
   }
 
   return NS_OK;
 }
 
--- a/widget/GfxInfoBase.h
+++ b/widget/GfxInfoBase.h
@@ -49,17 +49,17 @@ public:
   // using GfxInfoBase::GetFeatureStatus;
   // using GfxInfoBase::GetFeatureSuggestedDriverVersion;
   // using GfxInfoBase::GetWebGLParameter;
   // to import the relevant methods into their namespace.
   NS_IMETHOD GetFeatureStatus(int32_t aFeature, int32_t *_retval) MOZ_OVERRIDE;
   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;
 
   // Initialization function. If you override this, you must call this class's
   // version of Init first.
   // We need Init to be called separately from the constructor so we can
   // register as an observer after all derived classes have been constructed
   // and we know we have a non-zero refcount.
@@ -98,18 +98,16 @@ protected:
 private:
   virtual int32_t FindBlocklistedDeviceInList(const nsTArray<GfxDriverInfo>& aDriverInfo,
                                               nsAString& aSuggestedVersion,
                                               int32_t aFeature,
                                               OperatingSystem os);
 
   void EvaluateDownloadedBlacklist(nsTArray<GfxDriverInfo>& aDriverInfo);
 
-  nsCString mFailures[9]; // The choice of 9 is Ehsan's
-  uint32_t mFailureCount;
   Mutex mMutex;
 
 };
 
 }
 }
 
 #endif /* __mozilla_widget_GfxInfoBase_h__ */
--- a/widget/nsIGfxInfo.idl
+++ b/widget/nsIGfxInfo.idl
@@ -50,17 +50,18 @@ interface nsIGfxInfo : nsISupports
   readonly attribute DOMString adapterDriverVersion2;
   
   readonly attribute DOMString adapterDriverDate;
   readonly attribute DOMString adapterDriverDate2;
 
   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);
   /*
    * A set of constants for features that we can ask this GfxInfo object
    * about via GetFeatureStatus
    */
   /* Don't assign 0 or -1 */