From 2a5ffb8d23e8ce05718a81956909e6cf82e99e78 Mon Sep 17 00:00:00 2001 From: Milan Sreckovic Date: Tue, 13 Jan 2015 21:19:25 -0500 Subject: [PATCH] Bug 1112828 - Have GfxInfo::LogFailure use gfxCriticalLog and entries from gfxCriticalLog be available in about:support graphics section. r=jmuizelaar --- gfx/2d/Logging.h | 7 ++++ gfx/thebes/gfxPlatform.cpp | 9 +++++ toolkit/content/aboutSupport.js | 56 ++++++++++++++++++++++++-- toolkit/modules/Troubleshoot.jsm | 11 +++++- widget/GfxInfoBase.cpp | 68 +++++++++++++++++++++++--------- widget/GfxInfoBase.h | 4 +- widget/nsIGfxInfo.idl | 3 +- 7 files changed, 130 insertions(+), 28 deletions(-) diff --git a/gfx/2d/Logging.h b/gfx/2d/Logging.h index 9c1ef644f24..e72ac66ff3a 100644 --- a/gfx/2d/Logging.h +++ b/gfx/2d/Logging.h @@ -9,6 +9,7 @@ #include #include #include +#include #ifdef MOZ_LOGGING #include @@ -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 > StringsVectorCopy() = 0; }; class NoLog diff --git a/gfx/thebes/gfxPlatform.cpp b/gfx/thebes/gfxPlatform.cpp index de72a240315..2454341c688 100644 --- a/gfx/thebes/gfxPlatform.cpp +++ b/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 > StringsVectorCopy(); + void SetCircularBufferSize(uint32_t aCapacity); private: @@ -201,6 +203,13 @@ void CrashStatsLogForwarder::SetCircularBufferSize(uint32_t aCapacity) mBuffer.reserve(static_cast(aCapacity)); } +std::vector > +CrashStatsLogForwarder::StringsVectorCopy() +{ + MutexAutoLock lock(mMutex); + return mBuffer; +} + bool CrashStatsLogForwarder::UpdateStringsVector(const std::string& aString) { diff --git a/toolkit/content/aboutSupport.js b/toolkit/content/aboutSupport.js index 25838d60e8d..5af615e3861 100644 --- a/toolkit/content/aboutSupport.js +++ b/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) diff --git a/toolkit/modules/Troubleshoot.jsm b/toolkit/modules/Troubleshoot.jsm index f6e142a9682..0ab4403d3cf 100644 --- a/toolkit/modules/Troubleshoot.jsm +++ b/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); }, diff --git a/widget/GfxInfoBase.cpp b/widget/GfxInfoBase.cpp index 6ffa7a75152..c04894ab9a3 100644 --- a/widget/GfxInfoBase.cpp +++ b/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& 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 > 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 >::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]) { /* I'm too afraid to use an inline function... */ NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(i, (*failures)); + *failureCount = i; return NS_ERROR_OUT_OF_MEMORY; } } diff --git a/widget/GfxInfoBase.h b/widget/GfxInfoBase.h index df21779d7d9..70576c45ce0 100644 --- a/widget/GfxInfoBase.h +++ b/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) MOZ_OVERRIDE; @@ -103,8 +103,6 @@ private: void EvaluateDownloadedBlacklist(nsTArray& aDriverInfo); - nsCString mFailures[9]; // The choice of 9 is Ehsan's - uint32_t mFailureCount; Mutex mMutex; }; diff --git a/widget/nsIGfxInfo.idl b/widget/nsIGfxInfo.idl index 8df4813d2e3..8b248d80ab7 100644 --- a/widget/nsIGfxInfo.idl +++ b/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);