668004 - Generalize the downloaded GPU blacklist to non-Windows platforms

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Since bug 625160, we use a downloaded GPU blacklist (downloaded as part of the addons blacklist, blocklist.xml) in addition to the built-in GPU blacklist.

But we only do that on Windows.

We need the same on Mac and on X11.

Comment 1

[Benoit Girard (:BenWa)]

Blocking mobile OGL layers.

Comment 2

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.0, generalized blocklisting.

Initial patch to generalize blocklisting. Confirmed to work on OSX and Linux. Might not build on Android. If this compiles correctly on Android, it should almost certainly work first shot.

Comment 3

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.0, generalized blocklisting.

Whoops, left something in the other one.

Comment 4

[Benoit Girard (:BenWa)]

We're going to need to implement the following for Android:
http://mxr.mozilla.org/mozilla-central/source/widget/src/android/GfxInfo.cpp#154
http://mxr.mozilla.org/mozilla-central/source/widget/src/android/GfxInfo.cpp#184
http://mxr.mozilla.org/mozilla-central/source/widget/src/android/GfxInfo.cpp#199

Since VendorID/DeviceID are strings on android we discussed hashing them. Sound good to everyone?

Comment 5

[Doug Sherk (:drs) (inactive)]

Sounds good to me.

Comment 6

[Benoit Girard (:BenWa)]

Attachment: Hash model/HW for vendor/deviceid

Feel free to fold this patch into yours

Comment 7

[Doug Sherk (:drs) (inactive)]

Have you tested to see if these two patches together work on Android?

Comment 8

[Benoit Girard (:BenWa)]

No I have not. How do I test this feature?

Comment 9

[Doug Sherk (:drs) (inactive)]

This wiki article gives instructions that work for Windows, and some of what you need for Linux/Mac:
https://wiki.mozilla.org/Blocklisting/Testing

It seems like on Android the instructions should be pretty similar. You can change the blocklist URL to a local file to make it easier. For this, you can use browser/app/blocklist.xml. Add an entry that matches up with your device and check if the feature gets disabled.

The only thing I don't know about is how to get to the error console on Android, and whether or not downloading the blocklist is implemented on it at all.

Comment 10

[Martijn Wargers (dead)]

In about:config, you can set the devtools.errorconsole.enabled pref to true to let the error console show up in the preferences.

Whether or not downloading the blocklist is implemented on it at all, perhaps Mark or Matt can answer that question.

Comment 11

[Benoit Girard (:BenWa)]

Attachment: Android fix, qfold into Patch v1.0

Simple fix to compile on android

Comment 12

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.2, generalized blocklisting.

Folded BenWa's patches in and cleared up some bitrot.

Comment 13

[Doug Sherk (:drs) (inactive)]

Running on Try: https://tbpl.mozilla.org/?tree=Try&rev=20a50709f71c

Comment 14

[Benoit Girard (:BenWa)]

I worked on testing this out on Android. I wrote a patch to give proper Device/Driver version in GfxInfo.cpp but I couldn't get the blocklist to update by changing the blocklist URI to the sdcard or changing the default blocklist.xml and clearing the profile.

Comment 15

[Doug Sherk (:drs) (inactive)]

Ok, I'll get myself setup with an Android dev env tomorrow so I can try and fix it.

Comment 16

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.3, generalized blocklisting.

Fixed driver suggestion to only happen on Windows.

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 570354 [details] [diff] [review]
Patch v1.3, generalized blocklisting.

Review of attachment 570354 [details] [diff] [review]:
-----------------------------------------------------------------

The design of all this seems to be made more complex by the fact that NO_INFO has acquired a new second meaning, in addition to its original meaning.

The original meaning of NO_INFO is "no bad news", "no reason to blacklist, so go ahead, use the feature".

The new meaning of NO_INFO in this patch seems to be "we don't know yet".

I won't r- the patch for that but we need a follow-up bug for solving this problem. I think we should split NO_INFO into two separate enum values, one of each of these two meanings: "don't know yet" and "do know that the feature is OK to use".

::: widget/src/android/GfxInfo.cpp
@@ +188,5 @@
> +  if (mozilla::AndroidBridge::Bridge()->GetStaticStringField("android/os/Build", "HARDWARE", str)) {
> +    *aAdapterVendorID = HashString(str);
> +    return NS_OK;
> +  }
> +

We don't want to coerce Android to return PCI-vendor-id-style values. Different OSes need to have different fields.

Leave GetAdapterVendorID unchanged and add/edit a different function that returns a string.

@@ +193,2 @@
>    *aAdapterVendorID = 0;
> +  return NS_ERROR_FAILURE;

Return NS_OK here like before. NS_ERROR_FAILURE would throw an exception in the about:support page, breaking it.

@@ +209,5 @@
> +  if (mozilla::AndroidBridge::Bridge()->GetStaticStringField("android/os/Build", "MODEL", str)) {
> +    *aAdapterDeviceID = HashString(str);
> +    return NS_OK;
> +  }
> +

same.

@@ +214,2 @@
>    *aAdapterDeviceID = 0;
> +  return NS_ERROR_FAILURE;

same.

::: widget/src/cocoa/GfxInfo.mm
@@ +55,5 @@
>  #include "nsICrashReporter.h"
>  #define NS_CRASHREPORTER_CONTRACTID "@mozilla.org/toolkit/crash-reporter;1"
>  #endif
>  
> +#define MAC_OS_X_VERSION_MASK       0x0000FFFF // Not supported

what does not supported mean here?

::: widget/src/xpwidgets/GfxDriverInfo.cpp
@@ +41,5 @@
>  using namespace mozilla::widget;
>  
>  PRUint32 GfxDriverInfo::allAdapterVendors = 0;
>  PRInt32 GfxDriverInfo::allFeatures = 0;
> +PRUint64 GfxDriverInfo::allDriverVersions = 0xffffffffffffffffULL;

~(PRUint64(0)) ?
~(PRUint64)0 ?

(long bikeshed discussion ensued)

::: widget/src/xpwidgets/GfxInfoBase.cpp
@@ +585,5 @@
> +  OperatingSystem os = DRIVER_OS_UNKNOWN;
> +  if (aOS)
> +    os = *aOS;
> +
> +  PRUint32 adapterVendor = 0;

ID

Comment 18

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.4, generalized blocklisting.

Dealt with code review comments. We decided to stall dealing with Android because it'll be an expensive refactor and we want to get Mac blocklisting out ASAP. I have instead disabled downloaded blocklisting on Android, although BenWa reports that it doesn't work at all to begin with. 

I don't like avoiding suggestions like the NO_INFO comment, which I agree with, but I'll include changes for that in a follow-up patch so that we can land this patch alone without worrying about big refactors.

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 570919 [details] [diff] [review]
Patch v1.4, generalized blocklisting.

Review of attachment 570919 [details] [diff] [review]:
-----------------------------------------------------------------

One of my earlier review comments is not addressed:

::: widget/src/android/GfxInfo.cpp
@@ +193,2 @@
>    *aAdapterVendorID = 0;
> +  return NS_ERROR_FAILURE;

In my previous review I asked for this to remain NS_OK

Comment 20

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.5, generalized blocklisting.

Fixed missed code review comment.

Comment 21

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.5, generalized blocklisting.

Comment 22

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f1d6394148c

Comment 23

[Jeff Muizelaar [:jrmuizel]]

The stuff that landed still has Android "pretend to be PCI" code. Was that intentional?

Comment 24

[Doug Sherk (:drs) (inactive)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #23)
> The stuff that landed still has Android "pretend to be PCI" code. Was that
> intentional?

It's disabled right now. I suppose it would have been better to just remove it, but it should have no impact.

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1f1d6394148c

Comment 26

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: dont return ERROR_FAILURE in GetFeatureStatusImpl

WebGL anti-aliasing is broken on linux, and can't even be force-enabled, because GetFeatureStatusImpl returns NS_ERROR_FAILURE. Looking at the code, it's trying to parse a driver version, which I believe only works on Windows, then returns NS_ERROR_FAILURE except on Mac, which I believe is the only reason why it's not broken on Mac.

GetFeatureStatusImpl should never return NS_ERROR_FAILURE, I believe. Regardless of how we structure C++ code in WebGLContext.cpp, GetFeatureStatus returning NS_ERROR_FAILURE would mean an exception thrown in about:support, breaking it. We already discussed that we don't want it to throw: bug 651981, bug 642502 comment 13.

Comment 27

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: change false to NO_INFO and add FIXME comment

I really don't know what that aValue = false; was doing here, especially as aValue is a PRInt32 and its possible values are the FEATURE symbolic constants.

GetFeatureStatusImpl relies on aValue getting set to FEATURE_NO_INFO if we don't know anything. That's a new second meaning of FEATURE_NO_INFO so this patch adds a big FIXME comment about that.

Moreover, if GetPrefNameForFeature fails, or if Preferences::GetInt fails, aValue didn't get correctly set to FEATURE_NO_INFO, which would have broken GetFeatureStatusImpl. This patch fixes that.

Comment 28

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf8b201e8980

Comment 29

[Doug Sherk (:drs) (inactive)]

Comment on attachment 572222 [details] [diff] [review]
change false to NO_INFO and add FIXME comment

Review of attachment 572222 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not really comfortable with landing this, especially if it doesn't fix anything that is known to be broken. I'm thinking that the functionality of this patch would be useful in the case where you want to assume something is blocked, then run GetPrefValueForFeature to confirm it. 

If we want to land this, we should at least fix the comment above the function.

::: widget/src/xpwidgets/GfxInfoBase.cpp
@@ +178,5 @@
> +  // GetFeatureStatusImpl.
> +  // TODO: dissociate these two meanings into two different symbolic values; clean up / rationaliz
> +  // this logic
> +  aValue = nsIGfxInfo::FEATURE_NO_INFO;
> +

So the line directly above this function reads "// If the pref doesn't exist, aValue is not touched, and returns false." This new functionality breaks this.

Comment 30

[Ed Morley [:emorley]]

(In reply to Benoit Jacob [:bjacob] from comment #28)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/cf8b201e8980

https://hg.mozilla.org/mozilla-central/rev/cf8b201e8980