Bug 1797580 - Fix broken assumptions with allowlists and the downloadable blocklist. r=jrmuizel

Ensure that when we evaluate the downloadable blocklist, we actually
only use the downloadable blocklist. We should not include any platform
specific checks in these prefs, as it causes confusion about why the
prefs were set in the first place. Allowlisted features should be
ignored when evaluating the downloadable blocklist; if we wish to
override the ALLOW/ALLOW_QUALIFIED/DENIED statuses, we should use OK or
BLOCKED_DEVICE or similar instead.

This caused allowlisted features (like WebRender) to be taken away from
users in the most recent nightly.

Differential Revision: https://phabricator.services.mozilla.com/D160408

toolkit/mozapps/extensions/test/xpcshell/rs-blocklist/test_gfxBlacklist_Version.js

@@ -84,13 +84,7 @@ async function run_test() {
     Assert.equal(failureId.value, "");
 
     status = gfxInfo.getFeatureStatus(Ci.nsIGfxInfo.FEATURE_OPENGL_LAYERS);
-    if (OS == "Android") {
-      // OpenGL layers are never blocklisted on Android, despite an entry in test_gfxBlacklist.json.
-      // https://searchfox.org/mozilla-central/rev/c1ec9ecbbc7eac698923ffd18c8594aa3e2e9da0/widget/android/GfxInfo.cpp#431-437
-      Assert.equal(status, Ci.nsIGfxInfo.FEATURE_STATUS_OK);
-    } else {
-      Assert.equal(status, Ci.nsIGfxInfo.FEATURE_BLOCKED_DRIVER_VERSION);
-    }
+    Assert.equal(status, Ci.nsIGfxInfo.FEATURE_BLOCKED_DRIVER_VERSION);
 
     status = gfxInfo.getFeatureStatus(
       Ci.nsIGfxInfo.FEATURE_WEBGL_OPENGL,

widget/GfxInfoBase.cpp

@@ -280,6 +280,11 @@ static bool GetPrefValueForFeature(int32_t aFeature, int32_t& aValue,
     return false;
   }
 
+  if (aValue == nsIGfxInfo::FEATURE_DENIED) {
+    // We should never see the DENIED status with the downloadable blocklist.
+    return false;
+  }
+
   nsCString failureprefname(prefname);
   failureprefname += ".failureid";
   nsAutoCString failureValue;
@@ -303,7 +308,7 @@ static void SetPrefValueForFeature(int32_t aFeature, int32_t aValue,
 
   Preferences::SetInt(prefname, aValue);
   if (!aFailureId.IsEmpty()) {
-    nsCString failureprefname(prefname);
+    nsAutoCString failureprefname(prefname);
     failureprefname += ".failureid";
     Preferences::SetCString(failureprefname.get(), aFailureId);
   }
@@ -810,6 +815,8 @@ GfxInfoBase::GetFeatureStatus(int32_t aFeature, nsACString& aFailureId,
     return NS_OK;
   }
 
+  // This is how we evaluate the downloadable blocklist. If there is no pref,
+  // then we will fallback to checking the static blocklist.
   if (GetPrefValueForFeature(aFeature, *aStatus, aFailureId)) {
     return NS_OK;
   }
@@ -1308,10 +1315,7 @@ nsresult GfxInfoBase::GetFeatureStatusImpl(
     return NS_OK;
   }
 
-  // Check if the device is blocked from the downloaded blocklist. If not, check
-  // the static list after that. This order is used so that we can later escape
-  // out of static blocks (i.e. if we were wrong or something was patched, we
-  // can back out our static block without doing a release).
+  // We only check either the given blocklist, or the static list, as given.
   int32_t status;
   if (aDriverInfo.Length()) {
     status =
@@ -1374,44 +1378,63 @@ GfxInfoBase::GetFeatureSuggestedDriverVersion(int32_t aFeature,
 
 void GfxInfoBase::EvaluateDownloadedBlocklist(
     nsTArray<GfxDriverInfo>& aDriverInfo) {
+  // If the list is empty, then we don't actually want to call
+  // GetFeatureStatusImpl since we will use the static list instead. In that
+  // case, all we want to do is make sure the pref is removed.
+  if (aDriverInfo.IsEmpty()) {
+    gfxCriticalNoteOnce << "Evaluate empty downloaded blocklist";
+    return;
+  }
+
+  OperatingSystem os = GetOperatingSystem();
+
   // For every feature we know about, we evaluate whether this blocklist has a
   // non-STATUS_OK status. If it does, we set the pref we evaluate in
   // GetFeatureStatus above, so we don't need to hold on to this blocklist
   // anywhere permanent.
   for (int feature = 1; feature <= nsIGfxInfo::FEATURE_MAX_VALUE; ++feature) {
-    int32_t status;
+    int32_t status = nsIGfxInfo::FEATURE_STATUS_UNKNOWN;
     nsCString failureId;
     nsAutoString suggestedVersion;
-    if (NS_SUCCEEDED(GetFeatureStatusImpl(feature, &status, suggestedVersion,
-                                          aDriverInfo, failureId))) {
-      switch (status) {
-        default:
-          MOZ_FALLTHROUGH_ASSERT("Unhandled feature status!");
-        case nsIGfxInfo::FEATURE_STATUS_UNKNOWN:
-          // UNKNOWN may be returned during shutdown or for invalid features.
-        case nsIGfxInfo::FEATURE_STATUS_OK:
-        case nsIGfxInfo::FEATURE_ALLOW_ALWAYS:
-        case nsIGfxInfo::FEATURE_ALLOW_QUALIFIED:
-          RemovePrefForFeature(feature);
-          break;
 
-        case nsIGfxInfo::FEATURE_BLOCKED_DRIVER_VERSION:
-          if (!suggestedVersion.IsEmpty()) {
-            SetPrefValueForDriverVersion(suggestedVersion);
-          } else {
-            RemovePrefForDriverVersion();
-          }
-          [[fallthrough]];
+    // Note that we are careful to call the base class method since we only want
+    // to evaluate the downloadable blocklist for these prefs.
+    MOZ_ALWAYS_TRUE(NS_SUCCEEDED(GfxInfoBase::GetFeatureStatusImpl(
+        feature, &status, suggestedVersion, aDriverInfo, failureId, &os)));
 
-        case nsIGfxInfo::FEATURE_BLOCKED_MISMATCHED_VERSION:
-        case nsIGfxInfo::FEATURE_BLOCKED_DEVICE:
-        case nsIGfxInfo::FEATURE_DISCOURAGED:
-        case nsIGfxInfo::FEATURE_BLOCKED_OS_VERSION:
-        case nsIGfxInfo::FEATURE_DENIED:
-        case nsIGfxInfo::FEATURE_BLOCKED_PLATFORM_TEST:
-          SetPrefValueForFeature(feature, status, failureId);
-          break;
-      }
+    switch (status) {
+      default:
+        MOZ_FALLTHROUGH_ASSERT("Unhandled feature status!");
+      case nsIGfxInfo::FEATURE_STATUS_UNKNOWN:
+        // This may be returned during shutdown or for invalid features.
+      case nsIGfxInfo::FEATURE_ALLOW_ALWAYS:
+      case nsIGfxInfo::FEATURE_ALLOW_QUALIFIED:
+      case nsIGfxInfo::FEATURE_DENIED:
+        // We cannot use the downloadable blocklist to control the allowlist.
+        // If a feature is allowlisted, then we should also ignore DENIED
+        // statuses from GetFeatureStatusImpl because we don't check the
+        // static list when and this is an expected value. If we wish to
+        // override the allowlist, it is as simple as creating a normal
+        // blocklist rule with a BLOCKED* status code.
+      case nsIGfxInfo::FEATURE_STATUS_OK:
+        RemovePrefForFeature(feature);
+        break;
+
+      case nsIGfxInfo::FEATURE_BLOCKED_DRIVER_VERSION:
+        if (!suggestedVersion.IsEmpty()) {
+          SetPrefValueForDriverVersion(suggestedVersion);
+        } else {
+          RemovePrefForDriverVersion();
+        }
+        [[fallthrough]];
+
+      case nsIGfxInfo::FEATURE_BLOCKED_MISMATCHED_VERSION:
+      case nsIGfxInfo::FEATURE_BLOCKED_DEVICE:
+      case nsIGfxInfo::FEATURE_DISCOURAGED:
+      case nsIGfxInfo::FEATURE_BLOCKED_OS_VERSION:
+      case nsIGfxInfo::FEATURE_BLOCKED_PLATFORM_TEST:
+        SetPrefValueForFeature(feature, status, failureId);
+        break;
     }
   }
 }

widget/GfxInfoBase.h

@@ -115,6 +115,8 @@ class GfxInfoBase : public nsIGfxInfo,
  protected:
   virtual ~GfxInfoBase();
 
+  virtual OperatingSystem GetOperatingSystem() = 0;
+
   virtual nsresult GetFeatureStatusImpl(
       int32_t aFeature, int32_t* aStatus, nsAString& aSuggestedDriverVersion,
       const nsTArray<GfxDriverInfo>& aDriverInfo, nsACString& aFailureId,

widget/android/GfxInfo.h

@@ -76,6 +76,9 @@ class GfxInfo : public GfxInfoBase {
   virtual uint32_t OperatingSystemVersion() override;
 
  protected:
+  OperatingSystem GetOperatingSystem() override {
+    return OperatingSystem::Android;
+  }
   virtual nsresult GetFeatureStatusImpl(
       int32_t aFeature, int32_t* aStatus, nsAString& aSuggestedDriverVersion,
       const nsTArray<GfxDriverInfo>& aDriverInfo, nsACString& aFailureId,

widget/cocoa/GfxInfo.h

@@ -67,6 +67,7 @@ class GfxInfo : public GfxInfoBase {
  protected:
   virtual ~GfxInfo() {}
 
+  OperatingSystem GetOperatingSystem() override;
   virtual nsresult GetFeatureStatusImpl(
       int32_t aFeature, int32_t* aStatus, nsAString& aSuggestedDriverVersion,
       const nsTArray<GfxDriverInfo>& aDriverInfo, nsACString& aFailureId,

widget/cocoa/GfxInfo.mm

@@ -426,6 +426,8 @@ const nsTArray<GfxDriverInfo>& GfxInfo::GetGfxDriverInfo() {
   return *sDriverInfo;
 }
 
+OperatingSystem GfxInfo::GetOperatingSystem() { return OSXVersionToOperatingSystem(mOSXVersion); }
+
 nsresult GfxInfo::GetFeatureStatusImpl(int32_t aFeature, int32_t* aStatus,
                                        nsAString& aSuggestedDriverVersion,
                                        const nsTArray<GfxDriverInfo>& aDriverInfo,

widget/gtk/GfxInfo.h

@@ -64,6 +64,9 @@ class GfxInfo final : public GfxInfoBase {
  protected:
   ~GfxInfo() = default;
 
+  OperatingSystem GetOperatingSystem() override {
+    return OperatingSystem::Linux;
+  }
   virtual nsresult GetFeatureStatusImpl(
       int32_t aFeature, int32_t* aStatus, nsAString& aSuggestedDriverVersion,
       const nsTArray<GfxDriverInfo>& aDriverInfo, nsACString& aFailureId,

widget/windows/GfxInfo.cpp

@@ -1927,6 +1927,10 @@ const nsTArray<GfxDriverInfo>& GfxInfo::GetGfxDriverInfo() {
   return *sDriverInfo;
 }
 
+OperatingSystem GfxInfo::GetOperatingSystem() {
+  return WindowsVersionToOperatingSystem(mWindowsVersion);
+}
+
 nsresult GfxInfo::GetFeatureStatusImpl(
     int32_t aFeature, int32_t* aStatus, nsAString& aSuggestedDriverVersion,
     const nsTArray<GfxDriverInfo>& aDriverInfo, nsACString& aFailureId,

widget/windows/GfxInfo.h

@@ -71,6 +71,8 @@ class GfxInfo : public GfxInfoBase {
   GfxInfo(GfxInfo&&) = delete;
   GfxInfo& operator=(GfxInfo&&) = delete;
 
+  OperatingSystem GetOperatingSystem() override;
+
   nsresult GetFeatureStatusImpl(int32_t aFeature, int32_t* aStatus,
                                 nsAString& aSuggestedDriverVersion,
                                 const nsTArray<GfxDriverInfo>& aDriverInfo,