1162530 - Version support in downloadable graphics blocklist

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

Right now, once it's in the blocklist, it blocks all versions.  Add the capability to specify minimum, maximum (or both) version(s) in the XML, so that we can blocklist things for a specific version or range.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Other places in the blocklist use <versionRange minVersion="..." maxVersion="..."></versionRange>, we can probably use the same.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

[Tracking Requested - why for this release]:

If we fix this for 40, it will let us use downloadable blocklist for release 39 and earlier as well; if we uplift to 39, it means we can handle 38 (and earlier) as well, which bodes well for an ESR release.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Allow min and max app versions for the downloadable blocklist. r=jmuizelaar

This will only work properly for non-e10s until bug 1162700 is fixed.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8603742 [details] [diff] [review]
Allow min and max app versions for the downloadable blocklist. r=jmuizelaar

There's a typo in the patch, let me fix that.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Allow min and max app versions for the downloadable blocklist. r=jmuizelaar

Use nsVersionComparator for version comparisons.  Add some unit tests for that, as well as the split version from GfxDriverInfo, to better describe what it is that happens in the version comparison (and the split was just a, turns out, unrelated bonus.)
Manual testing was done as described in bug 1164036, which will be the automated test for the same thing, but we have longer dependencies in order to take care of that.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8604715 [details] [diff] [review]
Allow min and max app versions for the downloadable blocklist. r=jmuizelaar

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

::: widget/GfxInfoBase.cpp
@@ +1047,1 @@
>          return NS_ERROR_OUT_OF_MEMORY;

It looks like there's a couple of spurious changes here and at the GetFailures() definition.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> Comment on attachment 8604715 [details] [diff] [review]
> Allow min and max app versions for the downloadable blocklist. r=jmuizelaar
> 
> Review of attachment 8604715 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/GfxInfoBase.cpp
> @@ +1047,1 @@
> >          return NS_ERROR_OUT_OF_MEMORY;
> 
> It looks like there's a couple of spurious changes here and at the
> GetFailures() definition.

Yeah, I sneaked in a couple of "we're using tabs instead of spaces" changes.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 1: Allow min and max app versions for the downloadable blocklist. Carry r=jmuizelaar

Get rid of the whitespace changes (will add in another changeset.)

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 2: Some cleanup in downloadable blocklist - we were not actually checking for WebRTC, some whitespace changes, watch for unknown OS. r=kats

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=358d6c8df74c + https://treeherder.mozilla.org/#/jobs?repo=try&revision=077998c3c692 combined try runs for a few bugs in the series.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8606369 [details] [diff] [review]
Part 2: Some cleanup in downloadable blocklist - we were not actually checking for WebRTC, some whitespace changes, watch for unknown OS. r=kats

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

Looks ok to me but again I'm hesitant to r+ since I don't know this code much.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Adding tracking flags for firefox40 and firefox 39 based on comment #2.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> ...
> Looks ok to me but again I'm hesitant to r+ since I don't know this code
> much.

You're becoming an expert just by looking at it :)

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93fe551e8a55
https://hg.mozilla.org/integration/mozilla-inbound/rev/edb5b81dd9e7

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/93fe551e8a55
https://hg.mozilla.org/mozilla-central/rev/edb5b81dd9e7

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8606368 [details] [diff] [review]
Part 1: Allow min and max app versions for the downloadable blocklist. Carry r=jmuizelaar

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: This gives us ability to blocklist things we wouldn't before because it would affect too many releases.  Uplifting to 40 means we could do specific blocklisting for release 39 and earlier.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8606369 [details] [diff] [review]
Part 2: Some cleanup in downloadable blocklist - we were not actually checking for WebRTC, some whitespace changes, watch for unknown OS. r=kats

Same as above.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8606368 [details] [diff] [review]
Part 1: Allow min and max app versions for the downloadable blocklist. Carry r=jmuizelaar

We do want that in 40!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d643717c69be
https://hg.mozilla.org/releases/mozilla-aurora/rev/70af32d331a0

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes with "Graphic blocklist mechanism improved: Firefox version ranges can be specified, limiting the number of devices blocked" as wording.