Closed Bug 1257547 Opened 8 years ago Closed 8 years ago

Remove coupling of Gfx code to XML file blocklist.xml

Categories

(Toolkit :: Blocklist Policy Requests, defect)

Product:
Toolkit
Component:
Blocklist Policy Requests
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: leplatrem, Assigned: leplatrem)

References

Details

Attachments

(4 files, 2 obsolete files)

Remove coupling of Gfx code to XML file blocklist.xml
26.02 KB, patch
milan
: feedback+
Details | Diff | Splinter Review
MozReview Request: Bug 1257547 - Remove coupling of Gfx code to XML file blocklist.xml
58 bytes, text/x-review-board-request
milan
: review+
Details
Bug1257547.failing - modify the test to fail after the implementation patch
3.09 KB, patch
Details | Diff | Splinter Review
Bug1257547.passing - this keeps the modified tests passing
3.77 KB, patch
Details | Diff | Splinter Review 
Currently, the blocklist of Gfx hardware is loaded when the XML content is parsed in nsBlocklistService. 

This happens synchronously on startup, probably because blacklisting must happen before the graphics are instantiated.

The current implementation shares a reference on the XML DOM element via a message. The C++ code in GfxInfoBase.cpp parses somes nodes using this reference.

In the Cloud services team we are working on moving blocklists to Kinto [1], and for that we'll need to remove the coupling with XML in the GfxInfoBase code.

[1] https://mail.mozilla.org/pipermail/firefox-dev/2016-March/004067.html
Assignee: nobody → mathieu
Attachment #8731715 - Attachment is obsolete: true
Comment on attachment 8731719 [details] [diff] [review]
Remove coupling of Gfx code to XML file blocklist.xml

The tests weren't updated but that's a proof of concept.

Is that a good idea to have a custom (simple though) serialization of values into a string?
Attachment #8731719 - Flags: feedback?(milan)
Attachment #8731719 - Flags: feedback?(joe)
Blocks: 1257565
As discussed with Mathieu, I was wondering if it would not be simpler to let the C++ side access to the data that's already in memory via an interface, rather than adding an ad-hoc serializing/deserializing step
Comment on attachment 8731719 [details] [diff] [review]
Remove coupling of Gfx code to XML file blocklist.xml

Joe doesn't work at Mozilla anymore
Attachment #8731719 - Flags: feedback?(joe)
Comment on attachment 8731719 [details] [diff] [review]
Remove coupling of Gfx code to XML file blocklist.xml

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

There is some duplicated code (driverVersionMax), but otherwise having the simplification on the graphics side is appreciated.  One thing that I'd like to see if we can get rid of - the graphics specific keys showing up in nsBlocklistService.js file, together with those showing up in GfxInfoBase.cpp (e.g., driverVersionMax, devices, etc.)  We could fix that with strong and clear documentation, but if there is a way to have those show up only in a single place (they used to be only in GfxInfoBase), it would be better.

Good direction though.
Attachment #8731719 - Flags: feedback?(milan) → feedback+
Thanks Milan, we'll work in this direction
Comment on attachment 8743026 [details]
MozReview Request: Bug 1257547 - Remove coupling of Gfx code to XML file blocklist.xml

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47141/diff/1-2/
Attachment #8743027 - Attachment is obsolete: true
Attachment #8743026 - Flags: review?(milan)
(In reply to Mathieu Leplatre (:leplatrem) from comment #11)
> Comment on attachment 8743026 [details]
> MozReview Request: Bug 1257547 - Remove coupling of Gfx code to XML file
> blocklist.xml
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/47141/diff/1-2/

Will look at it this week.
Comment on attachment 8743026 [details]
MozReview Request: Bug 1257547 - Remove coupling of Gfx code to XML file blocklist.xml

https://reviewboard.mozilla.org/r/47141/#review45187

::: toolkit/mozapps/extensions/nsBlocklistService.js:331
(Diff revision 2)
>     *                                 (default = 0)
>     *                   "maxVersion"  The maximum version in a version range
>     *                                 (default = *)
>     */
>    _addonEntries: null,
>    _pluginEntries: null,

Do we also need _gfxEntries: null here?

::: toolkit/mozapps/extensions/nsBlocklistService.js:667
(Diff revision 2)
>      this._addonEntries = [];
> +    this._gfxEntries = [];
>      this._pluginEntries = [];
>  
>      this._loadBlocklistFromString(request.responseText);
>      this._blocklistUpdated(oldAddonEntries, oldPluginEntries);

It may be worth a comment that we don't currently inform the users when the graphics blocklist changed, so that the reader knows this is on purpose?

::: toolkit/mozapps/extensions/nsBlocklistService.js:1122
(Diff revision 2)
> +  //   <hardware>foo</hardware>
> +  // </gfxBlacklistEntry>
> +  _handleGfxBlacklistNode: function (blocklistElement, result) {
> +    const blockEntry = {};
> +
> +    const strip = (s) => s.replace(/(^\s+)|(\s+$)/g, "");

Is this trim?  Generic recommendation seems to be [\s\uFEFF\xA0] instead of just \s, but I admit I know little of regex :)

::: toolkit/mozapps/extensions/nsBlocklistService.js:1130
(Diff revision 2)
> +      var matchElement = blocklistElement.childNodes.item(i);
> +      if (!(matchElement instanceof Ci.nsIDOMElement))
> +        continue;
> +
> +      let value;
> +      if (matchElement.localName === "devices") {

I know it should be fine, but still, === rather than == makes me nervous :)

::: toolkit/mozapps/extensions/nsBlocklistService.js:1140
(Diff revision 2)
> +          if (childValue) {
> +            value.push(childValue);
> +          }
> +        }
> +      } else if (matchElement.localName === "versionRange") {
> +        value = {minVersion: strip(matchElement.getAttribute("minVersion") || "*"),

This "*" causes trouble in GfxInfoBase, see comment there.  Setting it to "0" should work.

::: toolkit/mozapps/extensions/nsBlocklistService.js:1145
(Diff revision 2)
> +        value = {minVersion: strip(matchElement.getAttribute("minVersion") || "*"),
> +                 maxVersion: strip(matchElement.getAttribute("maxVersion") || "*")};
> +      } else {
> +        value = strip(matchElement.textContent);
> +      }
> +      blockEntry[matchElement.localName] = value;

Should this still be done if value is an empty string?  If somebody just sets <os></os>, we probably don't want blockEntry\\\["os"\\\] to be set to ""?

In fact, if you modify toolkit/mozapps/extensions/test/xpcshell/data/test_gfxBlacklist_AllOS.xml at line 98, to have <vendor></vendor> (e.g., remove 0xabcd value), we will get an assertion in the xpcshell test toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_Version.js

No assertion, and the test passes if we do:
if (value) {
  blocklistEntry[matchElement.localName] = value;
}
instead

::: toolkit/mozapps/extensions/nsBlocklistService.js:1299
(Diff revision 2)
> +    // This way we avoid spreading XML structure logics there.
> +    const payload = this._gfxEntries.map((r) => {
> +      return Object.keys(r).sort().filter((k) => !/id|last_modified/.test(k)).map((key) => {
> +        let value = r[key];
> +        if (Array.isArray(value)) {
> +          value = value.join(",");

This assumes there are no "," in the values in the arrays.  With the arrays currently only being devices, this is a valid assumption.  I'd like something to fail if that ever changes - can we have an assert or a failure reported, perhaps while we're building the arrays, that there are no "," allowed?

::: toolkit/mozapps/extensions/nsBlocklistService.js:1300
(Diff revision 2)
> +    const payload = this._gfxEntries.map((r) => {
> +      return Object.keys(r).sort().filter((k) => !/id|last_modified/.test(k)).map((key) => {
> +        let value = r[key];
> +        if (Array.isArray(value)) {
> +          value = value.join(",");
> +        } else if (value.hasOwnProperty("minVersion")) {

This works because we make sure above that we always have both minVersion and maxVersion, as long as versionRange is set.  Otherwise, we'd have to check for the "is there only one of the two that's been set".  Correct?

::: toolkit/mozapps/extensions/test/xpcshell/test_blocklist_gfx.js:1
(Diff revision 2)
> +const { classes: Cc, interfaces: Ci, utils: Cu } = Components;

This tests the map to string serialization.  Do we have a test for XML to map?  I guess that is implicitly tested through the existing blocklisting tests (which only run in debug, by the way.)

::: widget/GfxInfoBase.cpp:276
(Diff revision 2)
> -  // For each <device>, get its device ID, and return a freshly-allocated
> +  // For each device, get its device ID, and return a freshly-allocated
>    // GfxDeviceFamily with the contents of that array.
>    GfxDeviceFamily* deviceIds = new GfxDeviceFamily;
>  
> -  for (uint32_t i = 0; i < length; ++i) {
> -    nsCOMPtr<nsIDOMNode> node;
> +  for (uint32_t i = 0; i < devices.Length(); ++i) {
> +    deviceIds->AppendElement(NS_ConvertUTF8toUTF16(devices[i]));

We make sure we don't add any "empty" device entries to the array, so we don't need to check if devices[i] is empty.

::: widget/GfxInfoBase.cpp:373
(Diff revision 2)
> -
> -  node.forget(firstchild);
> -  return true;
> -}
>  
>  /*

Probably want to remove this whole commented section, and replace it with the string version that we're actually going to deal with.  E.g.,
/*
os:WINNT 6.0\tvendor:0x8086\tdevices:0x2582,0x2782\tfeature:DIRECT3D_10_LAYERS\tfeatureStatus:BLOCKED_DRIVER_VERSION\tdriverVersion:8.52.322.2202\tdriverVersionComparator:LESS_THAN_OR_EQUAL

::: widget/GfxInfoBase.cpp:410
(Diff revision 2)
> -  if (BlacklistNodeGetChildByName(element, NS_LITERAL_STRING("osversion"),
> -                                  getter_AddRefs(dataNode))) {
> -    BlacklistNodeToTextValue(dataNode, dataValue);
> -    aDriverInfo.mOperatingSystemVersion = strtoul(NS_LossyConvertUTF16toASCII(dataValue).get(),
> -                                                  nullptr, 10);
> -  }
> +    nsCString keyValue = keyValues[i];
> +    nsTArray<nsCString> splitted;
> +    ParseString(keyValue, ':', splitted);
> +    if (splitted.Length() != 2) {
> +      // If we don't recognize the input data, we do not want to proceed.
> +      gfxWarning() << "Unrecognized data " << keyValue.get();

We should make this stronger - a gfxCriticalError() instead, as it means something really nasty is going on.
And a "continue", instead of falling through.  We do not want to go into the code below, especially if the length of "splitted" is < 2.

::: widget/GfxInfoBase.cpp:413
(Diff revision 2)
> -    aDriverInfo.mOperatingSystemVersion = strtoul(NS_LossyConvertUTF16toASCII(dataValue).get(),
> -                                                  nullptr, 10);
> -  }
> +    if (splitted.Length() != 2) {
> +      // If we don't recognize the input data, we do not want to proceed.
> +      gfxWarning() << "Unrecognized data " << keyValue.get();
> +    }
> +    nsCString key = splitted[0];
> +    nsCString value = splitted[1];

It looks like the code below assumes the value is not empty, but nsBlocklistService.js may have let those through.  Can you confirm?

::: widget/GfxInfoBase.cpp:457
(Diff revision 2)
> -    aDriverInfo.mHardware = dataValue;
> +      aDriverInfo.mHardware = dataValue;
> +    } else if (key.EqualsLiteral("versionRange")) {
> +      nsTArray<nsCString> versionRange;
> +      ParseString(value, ',', versionRange);
> +      if (versionRange.Length() != 2) {
> +        gfxWarning() << "Unrecognized versionRange " << value.get();

As above, this is a string we produced, so we can go stronger, and gfxCriticalError() instead of gfxWarning() because there is a bug in our code if this happens.

::: widget/GfxInfoBase.cpp:463
(Diff revision 2)
> +        return false;
> +      }
> +      nsCString minValue = versionRange[0];
> +      nsCString maxValue = versionRange[1];
> +      mozilla::Version minV(minValue.get());
> +      if (minV > zeroV && appV < minV) {

This logic doesn't work anymore, we will get wrong results.  Before, if minVersion was not there, we never made a comparison, just allowing it to go through . We now send "*" through, and "*" compares higher than anything.  This means that not specifying minVersion will kick all application versions out.  I left a comment with nsBlocklistService.js - changing the minVersion default to "0" from "*" works.

::: widget/GfxInfoBase.cpp:518
(Diff revision 2)
> -          blacklistEntries)
> -      {
> -        nsTArray<GfxDriverInfo> driverInfo;
> +    nsTArray<GfxDriverInfo> driverInfo;
> +    nsTArray<nsCString> blacklistEntries;
> +    nsCString utf8Data = NS_ConvertUTF16toUTF8(aData);
> +    gfxWarning() << "Received data: " << utf8Data.get();

Leftover debugging?
Slightly modifying the xpcshell tests still has them pass before the patch that fixes this bug.  However, it makes them fail once the patch is applied.
Mathieu, I attached the modified tests that run fine today, fail after your patch (running ./mach test toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_Version.js), and a potential patch that makes them pass.  I wanted to illustrate my comments, it's sometimes difficult to parse what I'm talking about :)
Flags: needinfo?(mathieu)
Thanks a lot Milan for this thorough review, I'll merge those and resubmit a review!
Flags: needinfo?(mathieu)
Comment on attachment 8743026 [details]
MozReview Request: Bug 1257547 - Remove coupling of Gfx code to XML file blocklist.xml

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47141/diff/2-3/
Attachment #8743026 - Flags: review?(milan)
Comment on attachment 8743026 [details]
MozReview Request: Bug 1257547 - Remove coupling of Gfx code to XML file blocklist.xml

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47141/diff/3-4/
https://reviewboard.mozilla.org/r/47141/#review45187

> Is this trim?  Generic recommendation seems to be [\s\uFEFF\xA0] instead of just \s, but I admit I know little of regex :)

I added those no-break spaces, can't harm

> I know it should be fine, but still, === rather than == makes me nervous :)

I removed the ones I had added, but didn't touch existing ones (there hence may be inconsistency)

> This works because we make sure above that we always have both minVersion and maxVersion, as long as versionRange is set.  Otherwise, we'd have to check for the "is there only one of the two that's been set".  Correct?

Exactly. I added a comment.

> This tests the map to string serialization.  Do we have a test for XML to map?  I guess that is implicitly tested through the existing blocklisting tests (which only run in debug, by the way.)

I added some for corner cases. Let me know if they seem sufficient.
Comment on attachment 8743026 [details]
MozReview Request: Bug 1257547 - Remove coupling of Gfx code to XML file blocklist.xml

https://reviewboard.mozilla.org/r/47141/#review47015

::: widget/GfxInfoBase.cpp:466
(Diff revision 4)
> +      nsCString maxValue = versionRange[1];
>  
> +      mozilla::Version minV(minValue.get());
> +      mozilla::Version maxV(maxValue.get());
> +
> +      fprintf(stderr,"***************************************************\n");

These are the leftover debugging prints, and should be removed, right?
Attachment #8743026 - Flags: review?(milan) → review+
Comment on attachment 8743026 [details]
MozReview Request: Bug 1257547 - Remove coupling of Gfx code to XML file blocklist.xml

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47141/diff/4-5/
I realize that my patch currently conflicts with Bug 1263249

I'll rebase and make sure that the newly expected blockId is passed along.
Comment on attachment 8743026 [details]
MozReview Request: Bug 1257547 - Remove coupling of Gfx code to XML file blocklist.xml

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47141/diff/5-6/
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/899a0bc9d72b
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: