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...

Drive-by reply: already handled!

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!

These are not exhaustive, obviously, but they're a good first step.

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.

Accidentally uploaded the wrong patch last time.

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.

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.

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.

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.

(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()

nsAString is not the same as nsAutoString, sadly.

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?

(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.

(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.

Carrying forward r+

carrying forward r+

Addressed review comments.

Jeff reviewed this patch earlier; Benoit still needs to, though.

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.

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

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

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

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

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.