617028 - Log gfx problems in about:support (Report Failure)

Status: NEW

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

We should be able to see when things go wrong without needing to crash.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

There's a front end to this interface in LayerManagerD3D9::ReportFailure(const nsACString &aMsg, HRESULT aCode).

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: Initial version of the C++ side

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: The about:support side

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add example call to LogFailure when binding a buffer fails

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Attachment: C++ side v2

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Attachment: about:support v2

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Attachment: C++ side v3

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 509569 [details] [diff] [review]
C++ side v3

Perhaps logFailure should be [noscript]...

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 509526 [details] [diff] [review]
Add example call to LogFailure when binding a buffer fails

"Mapping pixel buffer failed\n" maybe? It will be the fMapBuffer call that is failing.

It might also be useful to log the value of mPixelBufferSize since that seems like the most likely cause of failure for this.

Looks good though, who do we have to bribe to get approval for this bug?

Comment 10

[Joe Drew (not getting mail)]

Comment on attachment 509569 [details] [diff] [review]
C++ side v3


>diff --git a/gfx/thebes/gfxFailure.h b/gfx/thebes/gfxFailure.h

>@@ -0,0 +1,11 @@

add copyright header

>+#include "nsString.h"
>+#include "nsIGfxInfo.h"

add whitespace

>diff --git a/widget/public/nsIGfxInfo.idl b/widget/public/nsIGfxInfo.idl

>+  void getFailures(
>+               [optional] out unsigned long failureCount,
>+               [retval, array, size_is(failureCount)] out string failures);

>+  void logFailure(in ACString failure);

[noscript, noxpcom]

or prove that noxpcom ⇒ noscript

>diff --git a/widget/src/xpwidgets/GfxInfoBase.cpp b/widget/src/xpwidgets/GfxInfoBase.cpp

>-GfxInfoBase::GfxInfoBase()
>+GfxInfoBase::GfxInfoBase() : mFailureCount(0)

newline before :

>+/* void getFailures ([optional] out unsigned long failureCount, [array, size_is (failureCount), retval] out string failures); */
>+/* XPConnect method of returning arrays is very ugly. Would not recommend. Failable nsMemory::Alloc makes things worse */

s/Failable/Fallible/

>+NS_IMETHODIMP GfxInfoBase::GetFailures(PRUint32 *failureCount NS_OUTPARAM, char ***failures NS_OUTPARAM)
>+{
>+

remove newline

>diff --git a/widget/src/xpwidgets/GfxInfoBase.h b/widget/src/xpwidgets/GfxInfoBase.h

>@@ -67,16 +68,20 @@ public:
>   // using GfxInfoBase::GetFeatureStatus;
>   // using GfxInfoBase::GetFeatureSuggestedDriverVersion;
>   // using GfxInfoBase::GetWebGLParameter;
>   // to import the relevant methods into their namespace.
>   NS_SCRIPTABLE NS_IMETHOD GetFeatureStatus(PRInt32 aFeature, PRInt32 *_retval NS_OUTPARAM);
>   NS_SCRIPTABLE NS_IMETHOD GetFeatureSuggestedDriverVersion(PRInt32 aFeature, nsAString & _retval NS_OUTPARAM);
>   NS_SCRIPTABLE NS_IMETHOD GetWebGLParameter(const nsAString & aParam, nsAString & _retval NS_OUTPARAM);
> 
>+  NS_SCRIPTABLE NS_IMETHOD GetFailures(PRUint32 *failureCount NS_OUTPARAM, char ***failures NS_OUTPARAM);
>+
>+  NS_SCRIPTABLE NS_IMETHOD LogFailure(const nsACString &failure);
>+

No need for newlines between GetWebGLParameter and others.

Do we need to add "using GfxInfoBase::GetFailures" and "using GfxInfoBase::LogFailure" to all the subclasses?

>+  PRUint32 mFailureCount;
>+  nsCString mFailures[9]; // The choice of 9 is Ehsan's

Put the nsCString first for optimal packing

Blame Ehsan more by putting his email address in

Comment 11

[(no longer active)]

Comment on attachment 509568 [details] [diff] [review]
about:support v2

>+    let trGraphicsFailures = [];
>+    gfxInfo.getFailures().forEach(function (value) {
>+        trGraphicsFailures.push(createParentElement("tr", [
>+            createElement("td", value)
>+        ]));
>+    });

FWIW, this could be written as:

      let trGraphicsFailures = gfxInfo.getFailures().map(
        function(value)
          createParentElement("tr", [
            createElement("td", value)
          ])
      );

r=me on either your js-ey version or my pythonic version.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

> >+  PRUint32 mFailureCount;
> >+  nsCString mFailures[9]; // The choice of 9 is Ehsan's
> 
> Put the nsCString first for optimal packing

Why do you think the alignment will be > 32 bits?

Comment 13

[Ed Morley [:emorley]]

This landed in the latest nightly as:
http://hg.mozilla.org/mozilla-central/rev/165b354e3ec9

One slightly minor cosmetic issue: 
If there are no failures shown, the additional table (<tbody id="graphics-failures-tbody">) shows empty, adding a double border to the bottom of the table above it (<tbody id="graphics-tbody">).

Comment 14

[Rain [:rain1] (inactive)]

What's left to fix here?

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:bhood, could you have a look please?
For more information, please visit auto_nag documentation.