Closed Bug 1283601 Opened 8 years ago Closed 8 years ago

Relax blacklisting rule g1057 from bug 951422

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jrmuizel, Assigned: BenWa)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

The in-tree rule should be much more precise than the rule in the downloadable list.

This is the most hit blacklisting rule today. 34% of our accel failures are due to this.
Assignee: nobody → bgirard
Flags: needinfo?(bgirard)
We're blocklist Intel < 6.14.10.5218 in our downloadable blacklist but our hardcoded blocklist is only checking for V(6,14,10,5076) to V(6,14,10,5218). And based on the data we look at we're rejecting a lot of drivers before V(6,14,10,5076).

We need to re-enable Intel < V(6,14,10,5076)
Blocks: 1257692
Ideally I'd like to land something to relax the blocklist starting in FF50 (or 49 if we want). What's the best way to do that without lifting the blocklist rule for older version that might not support the FF version field in the blocklist?

Should we perhaps have two rules?
Block Intel < 6.14.10.5218 if FF < 50
Block V(6,14,10,5076) to V(6,14,10,5218) if FF >= 50.

Versions that ignore the FF version field which parse both rules and will continue to block the whole range. Versions that understand it should block only the restricted range.

Is this correct?
Flags: needinfo?(bgirard) → needinfo?(milan)
So, today we block WEBGL_ANGLE and D3D9 layers on XP for all Intel drivers < 6.14.10.5218:

<gfxBlacklistEntry  blockID="g1057">
  <os>WINNT 5.1</os>
  <vendor>0x8086</vendor>
  <feature>WEBGL_ANGLE</feature>
  <featureStatus>BLOCKED_DRIVER_VERSION</featureStatus>
  <driverVersion>6.14.10.5218</driverVersion>
  <driverVersionComparator>LESS_THAN</driverVersionComparator>
</gfxBlacklistEntry>

<gfxBlacklistEntry  blockID="g511">
  <os>WINNT 5.1</os>
  <vendor>0x8086</vendor>
  <feature>DIRECT3D_9_LAYERS</feature>
  <featureStatus>BLOCKED_DRIVER_VERSION</featureStatus>
  <driverVersion>6.14.10.5218</driverVersion>
  <driverVersionComparator>LESS_THAN</driverVersionComparator>
</gfxBlacklistEntry>


We want to relax the blocking, starting with 50, for both layers and WebGL, or just for layers?  Either way, we want to let versions before 6.14.10.5076 back in.

For WebGL scenario, we would replace g1057 with two entries (as suggested):

<gfxBlacklistEntry>
  <os>WINNT 5.1</os>
  <vendor>0x8086</vendor>
  <versionRange maxVersion="49" />
  <feature>WEBGL_ANGLE</feature>
  <featureStatus>BLOCKED_DRIVER_VERSION</featureStatus>
  <driverVersion>6.14.10.5076</driverVersion>
  <driverVersionComparator>LESS_THAN_OR_EQUAL</driverVersionComparator>
</gfxBlacklistEntry>

<gfxBlacklistEntry>
  <os>WINNT 5.1</os>
  <vendor>0x8086</vendor>
  <feature>WEBGL_ANGLE</feature>
  <featureStatus>BLOCKED_DRIVER_VERSION</featureStatus>
  <driverVersion>6.14.10.5076</driverVersion>
  <driverVersionMax>6.14.10.5218</driverVersionMax>
  <driverVersionComparator>DRIVER_BETWEEN_EXCLUSIVE</driverVersionComparator>
</gfxBlacklistEntry>

Firefox up to 49, including those that don't understand "versionRange" will take both of these, effectively doing what we were doing before, but with two entries (V <= 6.14.10.5076 and 6.14.10.5076 < V < 6.14.10.5218.)  Version 50 and higher would only take the second one, which only blocks versions > 6.14.10.5076 and < 6.14.10.5218, and is exactly the one we have in code.
Flags: needinfo?(milan)
Summary: Relax blacklisting rule g1057 from bug 951422 → Relax WEBGL_ANGLE blacklisting rule g1057 from bug 951422
Once we decide if this is just WEBGL_ANGLE or just DIRECT3D_9_LAYERS, or both, please change the title to reflect it.
Summary: Relax WEBGL_ANGLE blacklisting rule g1057 from bug 951422 → Relax blacklisting rule g1057 from bug 951422
Doing both should be fine. The underlying issue that caused us to block was the same in both situations.
To complicate things somewhat:
driverVersionMax wasn't supported in the downloadable blocklist until 44 (bug 1234385.)

The versionRange was not supported until 40 (bug 1162530.)  So, we can't really do what I was suggesting in comment 3 - the DRIVER_BETWEEN_EXCLUSIVE, without the driverVersionMax would effectively block everything above 6.14.10.5076.

At a first glance, it looks like we'd have two choices:

1. Leave things as they are.

2. Block all Intel drivers on Firefox 39 and below (0.4%), block what we do today for versions 40 through 49, and block the smaller range on Firefox 50 and higher.
(In reply to Milan Sreckovic [:milan] from comment #6)
> ...
> 
> 2. Block all Intel drivers on Firefox 39 and below (0.4%), block what we do
> today for versions 40 through 49, and block the smaller range on Firefox 50
> and higher.

Sorry, this is wrong.  It is actually removing the block on 39 and below, rather than adding more:

2. Remove this block for Firefox 39 and below (0.4%), block what we do today for versions 40 through 49, and block the smaller range on Firefox 50 and higher.
Attached file rough notes
Attaching my notes but here's the summary from the previous bugs and testing this in the graphics labs:

bug is introduced between 5076-5157
bug is fixed between 5082-5160
bug is introduced again in bug 5189-5215
bug is fixed again between 5215-5218

We tested some previous versions and the bug was not there 5002, 5035, 5076.

This seems to indicate that we should be able to relax the blacklist without a huge risk. We could also unblacklist between 5160 and 5189 is we wanted.
FWIW Nightly works fine when forcing D3D9 on a broken driver. We could re-enable if we wanted.
Benoit pointed out that we can work around the limitations I mentioned in comment 6 (comment 7) by setting the current, as they are, downloadable blocklist entries to apply to version 49.9 and below, and moving/leaving the logic for the higher versions out of the downloadable blocklist and into the code itself.
Can we please modify blocklist rule g511 to:
1) Rename g511 to a new ruleid for tracking purposes
2) add:
<versionRange maxVersion="49.9" />

This will let stricter more accurate built-in blacklist code take over.
Flags: needinfo?(awilliamson)
Flags: needinfo?(jorge)
> Can we please modify blocklist rule g511 to:
> 1) Rename g511 to a new ruleid for tracking purposes

We can delete the old block and add a new one.

> 2) add:
> <versionRange maxVersion="49.9" />

We've never had application version ranges in graphics blocks, and it isn't supported in the existing admin tools. We'll need to add support for it before we can make any changes. Is there any documentation about this new field?
Flags: needinfo?(milan)
Flags: needinfo?(jorge)
Flags: needinfo?(awilliamson)
For the record I tested the versionRange manually so it appears to work as expected. I'm not sure if Milan has documentation but from what I got by looking at the code:
versionRange supports minVersion and maxVersion which are parsed by the mozilla::Version. The rule will only apply if the current version falls between the range.

Sadly the Version documentation is a bit lacking:
https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsVersionComparator.h
The work was done in bug 1162530, the automated tests in bug 1164036.  We're using nsVersionComparator, which takes care of "a" and "b" in the versions, and, as Benoit mentions, the comparison is inclusive.  Landed in 41, uplifted to 40.

I'll happily add documentation - is there an expected place for it, or can I just put it somewhere on graphics wiki?
Flags: needinfo?(milan)
The info on comment #13 and bug 1164036 are enough for us, thanks.
It looks like the two github issues have been fixed now. Can we make the change to the blocklist now?
Flags: needinfo?(jorge)
I don't see it in the form yet, so it's possible it didn't land before last week's push. cgrebs, can you confirm?
Flags: needinfo?(jorge) → needinfo?(cgrebs)
It isn't yet, it's on dev though and will be deployed this Thursday as it'll go into today's tag.
As per #c11 I've deleted g1057 (https://addons.mozilla.org/en-US/admin/models/blocklist/blocklistdetail/1057/) and replaced it with g1251 (https://addons.mozilla.org/en-US/admin/models/blocklist/blocklistdetail/1251/) which has a maxVersion of 49.9.

Some of the earlier comments talked about additional blocks or adjusting the driver version from 6.14.10.5218 so if any of that is needed too please ni.
Flags: needinfo?(cgrebs)
This looks fine and jeff confirmed that the updated data has started coming in to Telemetry.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: