Need to load graphics blocklist entries from blocklist service

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: Joe Drew (not getting mail))

Tracking

(Depends on: 3 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [hardblocker])

Attachments

(9 attachments, 14 obsolete attachments)

19.74 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
18.66 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
4.76 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
33.90 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
11.16 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
5.69 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
8.67 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
22.58 KB, patch
jrmuizel
: review+
mossop
: review+
Details | Diff | Splinter Review
1.98 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Updated

7 years ago
blocking2.0: --- → betaN+
(Assignee)

Comment 1

7 years ago
Created attachment 504015 [details] [diff] [review]
part 1: Split out the Windows DriverInfo structure into a separate file
Attachment #504015 - Flags: review?(jmuizelaar)
(Assignee)

Comment 2

7 years ago
Created attachment 504016 [details] [diff] [review]
part 2: Create a base class of Windows' GfxInfo that will let us override GetFeatureStatus and GetFeatureSuggestedDriverVersion
Attachment #504016 - Flags: review?(jmuizelaar)
(Assignee)

Comment 3

7 years ago
Created attachment 504017 [details] [diff] [review]
part 3: Let prefs, one per feature, override the return value of GetFeatureStatus and GetFeatureSuggestedDriverVersion
(Assignee)

Updated

7 years ago
Attachment #504017 - Attachment is patch: true
Attachment #504017 - Attachment mime type: application/octet-stream → text/plain
Attachment #504017 - Flags: review?(jmuizelaar)
(Assignee)

Comment 4

7 years ago
Created attachment 504019 [details] [diff] [review]
part 4: Register for the blocklist services' gfxInfo section, and parse that XML into a GfxDriverInfo
Attachment #504019 - Flags: review?(jmuizelaar)
Attachment #504019 - Flags: review?(dtownsend)
(Assignee)

Updated

7 years ago
Attachment #504019 - Attachment is patch: true
Attachment #504019 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 5

7 years ago
Created attachment 504021 [details] [diff] [review]
part 5: Evaluate the downloaded and parsed blacklist, and set the override prefs based on it
Attachment #504021 - Flags: review?(jmuizelaar)
(Assignee)

Comment 6

7 years ago
This works on Windows. Still to do: Get it compiling on Mac and Android (our other GfxInfo implementations), and write tests.
drive-by note: from talking to Mossop, I think this data needs to be cached -- we'll only get the blocklist data once per day, which means that the code will only trigger once per day.  If someone quits and restarts firefox, they still need to have up to date blocklist info.

It might be enough for 4.0.0 to just have a "blocked by blocklist" pref, which we'd always flip on/off based on the blocklist status (to let us remove things from the blocklist if they were added wrongly).  That would cover most use cases, but wouldn't cover "I quit firefox and turned off my PC and installed a new video card, then fired up firefox again and now it's crashing".  But it should be ok to get to that case in a followup...
(Assignee)

Comment 8

7 years ago
Drive-by reply: already handled!
(Assignee)

Comment 9

7 years ago
I've made a bunch of changes (getting stuff to compile on Mac, Linux, etc), and I honestly don't remember which patches got changed, so I'm going to reupload all of them. Sorry for the bugspam!
(Assignee)

Comment 10

7 years ago
Created attachment 504359 [details] [diff] [review]
part 1: Split out the Windows DriverInfo structure into a separate file
Attachment #504015 - Attachment is obsolete: true
Attachment #504359 - Flags: review?(jmuizelaar)
Attachment #504015 - Flags: review?(jmuizelaar)
(Assignee)

Comment 11

7 years ago
Created attachment 504360 [details] [diff] [review]
part 2: Create a base class for all GfxInfo implementations that will let us override GetFeatureStatus and GetFeatureSuggestedDriverVersion
Attachment #504016 - Attachment is obsolete: true
Attachment #504360 - Flags: review?(jmuizelaar)
Attachment #504016 - Flags: review?(jmuizelaar)
(Assignee)

Comment 12

7 years ago
Created attachment 504361 [details] [diff] [review]
part 3: Let prefs, one per feature, override the return value of GetFeatureStatus and GetFeatureSuggestedDriverVersion
Attachment #504017 - Attachment is obsolete: true
Attachment #504361 - Flags: review?(jmuizelaar)
Attachment #504017 - Flags: review?(jmuizelaar)
(Assignee)

Comment 13

7 years ago
Created attachment 504362 [details] [diff] [review]
part 4: Register for the blocklist services' gfxInfo section, and parse that XML into a GfxDriverInfo
Attachment #504019 - Attachment is obsolete: true
Attachment #504362 - Flags: review?(jmuizelaar)
Attachment #504362 - Flags: review?(dtownsend)
Attachment #504019 - Flags: review?(jmuizelaar)
Attachment #504019 - Flags: review?(dtownsend)
(Assignee)

Comment 14

7 years ago
Created attachment 504363 [details] [diff] [review]
part 5: Evaluate the downloaded and parsed blacklist, and set the override prefs based on it
Attachment #504021 - Attachment is obsolete: true
Attachment #504363 - Flags: review?(jmuizelaar)
Attachment #504021 - Flags: review?(jmuizelaar)
(Assignee)

Comment 15

7 years ago
Created attachment 504364 [details] [diff] [review]
part 6: move all driver/device/etc spoofing initialization to Init(), to allow it to be overridden in future patches
Attachment #504364 - Flags: review?(bjacob)
(Assignee)

Comment 16

7 years ago
Created attachment 504365 [details] [diff] [review]
part 7: create a debug-only nsIGfxInfoDebug interface that lets us programmatically spoof OS version, device, etc
Attachment #504365 - Flags: review?(jmuizelaar)
Attachment #504365 - Flags: review?(bjacob)
(Assignee)

Comment 17

7 years ago
Created attachment 504366 [details] [diff] [review]
part 8: add gfx blacklist tests

These are not exhaustive, obviously, but they're a good first step.
Attachment #504366 - Flags: review?(jmuizelaar)
Attachment #504366 - Flags: review?(dtownsend)
(Assignee)

Comment 18

7 years ago
A very important note: These patches implement the downloaded blacklist ONLY ON WINDOWS. Mac/Android support can come later if it's necessary.
Comment on attachment 504359 [details] [diff] [review]
part 1: Split out the Windows DriverInfo structure into a separate file

I'd prefer to have the enums namespaced and then we can "using" it to avoid have to scatter the prefix all around.
Attachment #504360 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 20

7 years ago
Created attachment 504499 [details] [diff] [review]
part 4: Register for the blocklist services' gfxInfo section, and parse that XML into a GfxDriverInfo

Accidentally uploaded the wrong patch last time.
Attachment #504362 - Attachment is obsolete: true
Attachment #504499 - Flags: review?(jmuizelaar)
Attachment #504499 - Flags: review?(dtownsend)
Attachment #504362 - Flags: review?(jmuizelaar)
Attachment #504362 - Flags: review?(dtownsend)
Depends on: 626493
Comment on attachment 504499 [details] [diff] [review]
part 4: Register for the blocklist services' gfxInfo section, and parse that XML into a GfxDriverInfo

[snip]

>+static PRUint32
>+BlacklistHexToInt(const nsAString& aHex)
>+{
>+  PRInt32 err;
>+  nsCAutoString hex = NS_LossyConvertUTF16toASCII(aHex);
>+  PRInt32 value = hex.ToInteger(&err, 16);
>+  if (NS_FAILED(err))
>+    return 0;
>+  return (PRUint32) value;
>+}

Let's avoid the conversion to ASCII if we can.

>+
>+static PRUint32*
>+BlacklistDevicesToDeviceFamily(nsIDOMNodeList* aDevices)
>+{
>+  // Collect the <device> nodes.
>+  nsCOMArray<nsIDOMNode> deviceNodes;
>+
>+  PRUint32 length;
>+  if (NS_FAILED(aDevices->GetLength(&length)))
>+    return nsnull;
>+
>+  for (PRUint32 i = 0; i < length; ++i) {
>+    nsCOMPtr<nsIDOMNode> device;
>+    if (NS_FAILED(aDevices->Item(i, getter_AddRefs(device))))
>+      continue;
>+
>+    nsAutoString deviceNodeName;
>+    if (NS_FAILED(device->GetNodeName(deviceNodeName)) ||
>+        deviceNodeName != NS_LITERAL_STRING("device"))
>+      continue;
>+
>+    deviceNodes.AppendObject(device);
>+  }
>+
>+  // For each <device>, get its device ID, and return a freshly-allocated array
>+  // with the contents of that array.
>+  nsAutoArrayPtr<PRUint32> deviceIds(new PRUint32[deviceNodes.Count() + 1]);
>+  memset(deviceIds, 0, sizeof(PRUint32) * (deviceNodes.Count() + 1));
>+
>+  for (PRInt32 i = 0; i < deviceNodes.Count(); ++i) {
>+    nsAutoString deviceValue;
>+    if (!BlacklistNodeToTextValue(deviceNodes[i], deviceValue))
>+      continue;
>+
>+    PRUint32 deviceId = BlacklistHexToInt(deviceValue);
>+    if (!deviceId)
>+      continue;
>+
>+    deviceIds[i] = deviceId;
>+  }
>+
>+  return deviceIds.forget();
>+}
>+

[snip]

>+    return GfxDriverInfo::DRIVER_BETWEEN_INCLUSIVE;
>+  else if (op == NS_LITERAL_STRING("BETWEEN_INCLUSIVE_START"))
>+    return GfxDriverInfo::DRIVER_BETWEEN_INCLUSIVE_START;
>+
>+  return GfxDriverInfo::DRIVER_LESS_THAN;
>+}

These comparison functions should handle errors better.
Have them return an invalid id.

>+
>+/*
>+
>+<gfxBlacklistEntry>
>+  <os>WINNT 6.0</os>
>+  <vendor>0x8086</vendor>
>+  <devices>
>+    <device>0x2582</device>
>+    <device>0x2782</device>
>+  </devices>
>+  <feature> DIRECT3D_10_LAYERS </feature>
>+  <featureStatus> BLOCKED_DRIVER_VERSION </featureStatus>
>+  <driverVersion> 8.52.322.2202 </driverVersion>
>+  <driverVersionComparator> LESS_THAN_OR_EQUAL </driverVersionComparator>
>+</gfxBlacklistEntry>
>+
>+*/
>+static bool
>+BlacklistEntryToDriverInfo(nsIDOMNode* aBlacklistEntry,
>+                           GfxDriverInfo& aDriverInfo)
>+{

[snip]

>+}

Please redo BlacklistEntryToDriverInfo() this to use something like getElementByTagName()


[snip]

>+GfxInfoBase::GfxInfoBase()
>+{
>+  mBlacklistObserver = new BlacklistObserver(this);
>+  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();

I'd rather the observer have a reference to GfxInfo then vice-versa.
Attachment #504499 - Flags: review?(jmuizelaar) → review-
Attachment #504361 - Flags: review?(jmuizelaar) → review+
Attachment #504359 - Flags: review?(jmuizelaar) → review-
Comment on attachment 504365 [details] [diff] [review]
part 7: create a debug-only nsIGfxInfoDebug interface that lets us programmatically spoof OS version, device, etc

I'm not sure it's worth spoofing the windows version is it?
(In reply to comment #22)
> Comment on attachment 504365 [details] [diff] [review]
> part 7: create a debug-only nsIGfxInfoDebug interface that lets us
> programmatically spoof OS version, device, etc
> 
> I'm not sure it's worth spoofing the windows version is it?

I guess you want this 'cause the xml contains OS.
Comment on attachment 504366 [details] [diff] [review]
part 8: add gfx blacklist tests

Shouldn't this spoof different things and see how the blacklist interacts?
Comment on attachment 504365 [details] [diff] [review]
part 7: create a debug-only nsIGfxInfoDebug interface that lets us programmatically spoof OS version, device, etc

I can't think of a better way to do this.
Attachment #504365 - Flags: review?(jmuizelaar) → review+
Comment on attachment 504499 [details] [diff] [review]
part 4: Register for the blocklist services' gfxInfo section, and parse that XML into a GfxDriverInfo

Will just defer to Jeff on subsequent reviews of this. Aside from the few minor issues below this all looks fine to me.

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

>+// <foo>Hello</foo> - "Hello" is stored as a child text node of the foo node.
>+static bool
>+BlacklistNodeToTextValue(nsIDOMNode *aBlacklistNode, nsAString& aValue)
>+{
>+  nsCOMPtr<nsIDOMNode> child;
>+  if (NS_FAILED(aBlacklistNode->GetFirstChild(getter_AddRefs(child))) || !child)
>+    return false;
>+
>+  PRUint16 type = 0;
>+  if (NS_FAILED(child->GetNodeType(&type)) || type != nsIDOMNode::TEXT_NODE)
>+    return false;
>+
>+  if (NS_FAILED(child->GetNodeValue(aValue)))
>+    return false;
>+
>+  return true;

This isn't quite right as it ignores the (albeit unlikely) possibility of multiple child text nodes. Best way to go is probably to just QI to nsIDOMElement and use GetTextContent.

>+static PRUint32*
>+BlacklistDevicesToDeviceFamily(nsIDOMNodeList* aDevices)
>+{
>+  // Collect the <device> nodes.
>+  nsCOMArray<nsIDOMNode> deviceNodes;
>+
>+  PRUint32 length;
>+  if (NS_FAILED(aDevices->GetLength(&length)))
>+    return nsnull;
>+
>+  for (PRUint32 i = 0; i < length; ++i) {
>+    nsCOMPtr<nsIDOMNode> device;
>+    if (NS_FAILED(aDevices->Item(i, getter_AddRefs(device))))
>+      continue;
>+
>+    nsAutoString deviceNodeName;
>+    if (NS_FAILED(device->GetNodeName(deviceNodeName)) ||
>+        deviceNodeName != NS_LITERAL_STRING("device"))
>+      continue;
>+
>+    deviceNodes.AppendObject(device);
>+  }
>+
>+  // For each <device>, get its device ID, and return a freshly-allocated array
>+  // with the contents of that array.
>+  nsAutoArrayPtr<PRUint32> deviceIds(new PRUint32[deviceNodes.Count() + 1]);
>+  memset(deviceIds, 0, sizeof(PRUint32) * (deviceNodes.Count() + 1));
>+
>+  for (PRInt32 i = 0; i < deviceNodes.Count(); ++i) {
>+    nsAutoString deviceValue;
>+    if (!BlacklistNodeToTextValue(deviceNodes[i], deviceValue))
>+      continue;

Probably should trim this string for whitespace same as elsewhere.

>+NS_IMETHODIMP
>+BlacklistObserver::Observe(nsISupports* aSubject, const char* aTopic,
>+                           const PRUnichar* aData)
>+{
>+  if (strcmp(aTopic, "blocklist-data-gfxItems") == 0) {
>+    nsCOMPtr<nsIDOMNode> gfxItems = do_QueryInterface(aSubject);
>+    if (gfxItems) {
>+      nsCOMPtr<nsIDOMNodeList> blacklistEntries;
>+      gfxItems->GetChildNodes(getter_AddRefs(blacklistEntries));
>+      if (blacklistEntries) {
>+        nsTArray<GfxDriverInfo> driverInfo;
>+        BlacklistEntriesToDriverInfo(blacklistEntries, driverInfo);

I presume you do something with this in a later patch.

>+GfxInfoBase::GfxInfoBase()
>+{
>+  mBlacklistObserver = new BlacklistObserver(this);
>+  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>+  if (os) {
>+    os->AddObserver(mBlacklistObserver, "blocklist-data-gfxItems", PR_FALSE);
>+  }

Personally I would probably just make GfxInfoBase implement nsIObserver to save having the extra class for this. Not terribly bothered either way though.
Attachment #504499 - Flags: review?(dtownsend) → review+
Comment on attachment 504366 [details] [diff] [review]
part 8: add gfx blacklist tests

There is certainly more that could be tested here (different comparisons f.e.) however to start with could we get some comments at the top of each test saying what it is actually testing. My brain is getting fuzzy just trying to figure it out for myself.
Attachment #504366 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 28

7 years ago
(In reply to comment #21)
> Comment on attachment 504499 [details] [diff] [review]
> part 4: Register for the blocklist services' gfxInfo section, and parse that
> XML into a GfxDriverInfo
> 
> [snip]
> 
> >+static PRUint32
> >+BlacklistHexToInt(const nsAString& aHex)
> >+{
> >+  PRInt32 err;
> >+  nsCAutoString hex = NS_LossyConvertUTF16toASCII(aHex);
> >+  PRInt32 value = hex.ToInteger(&err, 16);
> >+  if (NS_FAILED(err))
> >+    return 0;
> >+  return (PRUint32) value;
> >+}
> 
> Let's avoid the conversion to ASCII if we can.

We can't :(
(In reply to comment #28)
> (In reply to comment #21)
> > Comment on attachment 504499 [details] [diff] [review]
> > 
> > Let's avoid the conversion to ASCII if we can.
> 
> We can't :(

Why not? nsAutoString seems to have ToInteger()
(Assignee)

Comment 30

7 years ago
nsAString is not the same as nsAutoString, sadly.
(Assignee)

Comment 31

7 years ago
Oh - OK. I can avoid the conversion, but not the copy.
Another drive-by suggestion, but perhaps too late: implement this all in JS for simplicity, use jsctypes for getting system info?
(Assignee)

Comment 33

7 years ago
(In reply to comment #24)
> Comment on attachment 504366 [details] [diff] [review]
> part 8: add gfx blacklist tests
> 
> Shouldn't this spoof different things and see how the blacklist interacts?

It does do this, but unfortunately the tests weren't labelled correctly. New tests coming up.
(Assignee)

Comment 34

7 years ago
(In reply to comment #32)
> Another drive-by suggestion, but perhaps too late: implement this all in JS for
> simplicity, use jsctypes for getting system info?

I thought about it, but didn't do it, maybe for no good reason. It's too late now, yes.
(Assignee)

Comment 35

7 years ago
Created attachment 504970 [details] [diff] [review]
part 1: Split out the Windows DriverInfo structure into a separate file, v2
Attachment #504359 - Attachment is obsolete: true
Attachment #504970 - Flags: review?(jmuizelaar)
(Assignee)

Comment 36

7 years ago
Created attachment 504971 [details] [diff] [review]
part 2: create GfxInfoBase, v2

Carrying forward r+
Attachment #504971 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #504360 - Attachment is obsolete: true
(Assignee)

Comment 37

7 years ago
Created attachment 504972 [details] [diff] [review]
part 3: add prefs, v2

carrying forward r+
Attachment #504361 - Attachment is obsolete: true
Attachment #504972 - Flags: review+
(Assignee)

Comment 38

7 years ago
Created attachment 504974 [details] [diff] [review]
part 4: parse the downloaded XML/DOM tree, v2

Addressed review comments.
Attachment #504499 - Attachment is obsolete: true
Attachment #504974 - Flags: review?(jmuizelaar)
(Assignee)

Comment 39

7 years ago
Created attachment 504975 [details] [diff] [review]
part 5: evaluate the downloaded and parsed blacklist, and set the override prefs based on it
Attachment #504363 - Attachment is obsolete: true
Attachment #504975 - Flags: review?(jmuizelaar)
Attachment #504363 - Flags: review?(jmuizelaar)
(Assignee)

Comment 40

7 years ago
Created attachment 504977 [details] [diff] [review]
part 6: move all spoofing to Init
Attachment #504364 - Attachment is obsolete: true
Attachment #504977 - Flags: review?(bjacob)
Attachment #504364 - Flags: review?(bjacob)
(Assignee)

Comment 41

7 years ago
Created attachment 504978 [details] [diff] [review]
part 7: create nsIGfxInfoDebug

Jeff reviewed this patch earlier; Benoit still needs to, though.
Attachment #504365 - Attachment is obsolete: true
Attachment #504978 - Flags: review?(bjacob)
Attachment #504365 - Flags: review?(bjacob)
(Assignee)

Comment 42

7 years ago
Created attachment 504979 [details] [diff] [review]
part 8: add tests v2

This patch adds comments to every test, and adds a test that makes sure we're setting and removing the prefs that save the blocking decisions for startup.
Attachment #504366 - Attachment is obsolete: true
Attachment #504979 - Flags: review?(jmuizelaar)
Attachment #504979 - Flags: review?(dtownsend)
Attachment #504366 - Flags: review?(jmuizelaar)
(Assignee)

Updated

7 years ago
Whiteboard: [hardblocker][january 18] → [hardblocker][january 18][new eta january 20][needs review]
Attachment #504970 - Flags: review?(jmuizelaar) → review+
Attachment #504977 - Flags: review?(bjacob) → review+
Attachment #504978 - Flags: review?(bjacob) → review+
Comment on attachment 504979 [details] [diff] [review]
part 8: add tests v2

Looks good apart from a few comment errors. I think even for this first landing you should also add a test for the case where only the vendor ID differs and a couple of different comparison cases, I would hazard a guess that BETWEEN_INCLUSIVE_START, EQUAL and GREATER_THAN_OR_EQUAL are liable to be the most common other cases that we block on.

One thing I suddenly realised is that in all your code you've used the term "blacklist" when we generally refer to the service as a "blocklist". Maybe not a big deal and perhaps a bit late to switch it everywhere but it does mean we're a little inconsistent.

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_Device.js b/toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_Device.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_Device.js
>@@ -0,0 +1,71 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ */
>+
>+// Test whether a machine which differs only on device ID, but otherwise
>+// exactly matches the blacklist entry, is successfully blocked.
>+// Uses test_gfxBlacklist.xml

Seems to actually check that a device with a different device ID is not blocked.

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_DriverNew.js b/toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_DriverNew.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_DriverNew.js
>@@ -0,0 +1,72 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ */
>+
>+// Test whether a new-enough driver bypasses the blacklist, even if the rest of
>+// the attributes match the blacklist entry.
>+// Uses test_gfxBlacklist.xml
>+
>+do_load_httpd_js();
>+
>+var gTestserver = null;
>+
>+function load_blocklist(file) {
>+  Services.prefs.setCharPref("extensions.blocklist.url", "http://localhost:4444/data/" + file);
>+  var blocklist = Cc["@mozilla.org/extensions/blocklist;1"].
>+                  getService(Ci.nsITimerCallback);
>+  blocklist.notify(null);
>+}
>+
>+// Performs the initial setup
>+function run_test() {
>+  try {
>+    var gfxInfo = Cc["@mozilla.org/gfx/info;1"].getService(Ci.nsIGfxInfo);
>+  } catch (e) {
>+    do_test_finished();
>+    return;
>+  }
>+
>+  // We can't do anything if we can't spoof the stuff we need.
>+  // We can't do anything if we can't spoof the stuff we need.

Probably only need this comment once (also in test_gfxBlacklist_OK.js)

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_OS.js b/toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_OS.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_OS.js
>@@ -0,0 +1,72 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ */
>+
>+// Test whether a machine which differs only on OS version, but otherwise
>+// exactly matches the blacklist entry, is successfully blocked.
>+// Uses test_gfxBlacklist.xml

Actually checks that it isn't blocked
Attachment #504979 - Flags: review?(dtownsend) → review+
Comment on attachment 504974 [details] [diff] [review]
part 4: parse the downloaded XML/DOM tree, v2

I wouldn't complain about the separating out the observer business from the parsing, but I expect that's not all that pleasant to do.

>-void
>+nsresult
> GfxInfo::Init()
> {
>   NS_TIME_FUNCTION;
> 
>+  nsresult rv = GfxInfoBase::Init();
>+

These return values make me sad, but perhaps there's not much that can be done. Please add a comment about the fact that they're needed for the construction stuff.

>+GfxDriverInfo::GfxDriverInfo()
>+  : mOperatingSystem(DRIVER_OS_UNKNOWN),
>+    mAdapterVendor(allAdapterVendors),
>+    mDevices(allDevices),
>+    mDeleteDevices(false),
>+    mFeature(allFeatures),
>+    mFeatureStatus(nsIGfxInfo::FEATURE_NO_INFO),
>+    mComparisonOp(DRIVER_LESS_THAN),

Wrong comparison op.

>+static PRUint32
>+BlacklistHexToInt(const nsAString& aHex)
>+{
>+  PRInt32 err;
>+  nsAutoString hex(aHex);
>+  PRInt32 value = hex.ToInteger(&err, 16);

Might as well add a comment about nsAString not supporting ToInteger.

>+  if (NS_FAILED(err))
>+    return 0;
>+  return (PRUint32) value;
>+}
>+
>+static PRUint32*
>+BlacklistDevicesToDeviceFamily(nsIDOMNodeList* aDevices)
>+{
>+  // Collect and count the <device> nodes.

Fix this comment.

>+  nsCOMArray<nsIDOMNode> deviceNodes;
>+
>+  PRUint32 length;
>+  if (NS_FAILED(aDevices->GetLength(&length)))
>+    return nsnull;
>+


>+static bool
>+BlacklistNodeGetFirstChildWithTagName(nsIDOMElement *element,
>+                                      const nsAString& tagname,
>+                                      nsIDOMNode** firstchild)
>+{

This name has more information than necessary. I'd trim it down to:
BlacklistNodeGetChildByName. You can add a comment in the implementation that
it arbitrarily chooses the first one.

>+  nsCOMPtr<nsIDOMNodeList> nodelist;
>+  if (NS_FAILED(element->GetElementsByTagName(tagname,
>+                                              getter_AddRefs(nodelist))) ||
>+      !nodelist) {
>+    return false;
>+  }
>+
>+  PRUint32 length = 0;
>+  if (NS_FAILED(nodelist->GetLength(&length)) || length == 0)
>+    return false;
>+

Do we really need to check the length here? I'd expect the next call to fail if length == 0
Attachment #504974 - Flags: review?(jmuizelaar) → review+
Attachment #504975 - Flags: review?(jmuizelaar) → review+
Attachment #504979 - Flags: review?(jmuizelaar) → review+
(Assignee)

Updated

7 years ago
Whiteboard: [hardblocker][january 18][new eta january 20][needs review] → [hardblocker][missed january 18][new eta january 19][needs landing]
(Assignee)

Comment 45

7 years ago
http://hg.mozilla.org/mozilla-central/rev/3706c8ac95b7
http://hg.mozilla.org/mozilla-central/rev/cce78c43034a
http://hg.mozilla.org/mozilla-central/rev/4e348632d5e4
http://hg.mozilla.org/mozilla-central/rev/839ddef92d3f
http://hg.mozilla.org/mozilla-central/rev/aa85b04aa9ef
http://hg.mozilla.org/mozilla-central/rev/ef843567f03a
http://hg.mozilla.org/mozilla-central/rev/e1b6a5426f31
http://hg.mozilla.org/mozilla-central/rev/80c4d7317c29
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [hardblocker][missed january 18][new eta january 19][needs landing] → [hardblocker]
bustage on Android

http://tinderbox.mozilla.org/showlog.cgi?log=Mobile/1295487973.1295488202.19222.gz

/builds/slave/cen-mb-br-andrd-r7-bld/build/mozilla-central/widget/src/android/GfxInfo.cpp:57: error: no 'nsrefcnt mozilla::widget::GfxInfo::AddRef()' member function declared in class 'mozilla::widget::GfxInfo'
/builds/slave/cen-mb-br-andrd-r7-bld/build/mozilla-central/widget/src/android/GfxInfo.cpp:57: error: no 'nsrefcnt mozilla::widget::GfxInfo::Release()' member function declared in class 'mozilla::widget::GfxInfo'
/builds/slave/cen-mb-br-andrd-r7-bld/build/mozilla-central/widget/src/android/GfxInfo.cpp:57: error: no 'nsresult mozilla::widget::GfxInfo::QueryInterface(const nsIID&, void**)' member function declared in class 'mozilla::widget::GfxInfo'
make[6]: *** [GfxInfo.o] Error 1
Created attachment 505457 [details] [diff] [review]
Remove old allFeatures=-1
Attachment #505457 - Flags: review?
Attachment #505457 - Flags: review? → review?(joe)
(Assignee)

Updated

7 years ago
Attachment #505457 - Flags: review?(joe) → review+
(Assignee)

Updated

7 years ago
Depends on: 627498
http://hg.mozilla.org/mozilla-central/rev/80b4174a2608
Depends on: 629957
Depends on: 645376
Depends on: 653102
Comment on attachment 504972 [details] [diff] [review]
part 3: add prefs, v2

>+static bool
>+GetPrefValueForDriverVersion(nsACString& aVersion)
...
>+static void
>+SetPrefValueForDriverVersion(const nsAString& aVersion)
This narrow/wide mismatch was a source for confusion. I'm filing a bug.

Updated

6 years ago
Depends on: 666381
You need to log in before you can comment on or make changes to this bug.