791742 - Alter blacklisting logic to consider substrings 'decimals'

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bas Schouten (:bas.schouten)]

Attachment: Consider driver version substrings decimals

So we've found out ATI treats the substring as decimals. I've created a patch which deals with this and seems to work well for all other vendor's driver strings. We should keep a close eye on if this stays valid in the future, but for now everything looks good.

This patch is pretty terrible, but it does the trick.

Comment 1

[Joe Drew (not getting mail)]

Comment on attachment 661840 [details] [diff] [review]
Consider driver version substrings decimals

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

*barf*

::: widget/xpwidgets/GfxDriverInfo.h
@@ +131,5 @@
> +  int destIdx = 0;
> +  int destPos = 0;
> +
> +  for (int i = 0; i < len; i++) {
> +    if (destIdx > 3) {

ArrayLength(dest) instead of hard-coding.

@@ +139,5 @@
> +
> +    if (aSource[i] == '.') {
> +      dest[destIdx][destPos] = 0;
> +      destIdx++;
> +      destPos = 0;

uggghhhhhhh

@@ +142,5 @@
> +      destIdx++;
> +      destPos = 0;
> +      continue;
> +    }
> +    

kill the spaces on this line

@@ +152,5 @@
> +    dest[destIdx][destPos++] = aSource[i];
> +  }
> +
> +  // Add last terminator.
> +  dest[destIdx][destPos] = 0;

Can you please refactor this loop so it doesn't need this special case?

@@ +154,5 @@
> +
> +  // Add last terminator.
> +  dest[destIdx][destPos] = 0;
> +
> +  if (destIdx != 3) {

ArrayLength(dest) instead of hard-coding.

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Consider driver version substrings decimals v2

Updated to properly interpret the hardcoded blacklist.

Comment 3

[Joe Drew (not getting mail)]

Comment on attachment 662244 [details] [diff] [review]
Consider driver version substrings decimals v2

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

::: widget/xpwidgets/GfxDriverInfo.h
@@ +124,5 @@
>  
> +static uint64_t
> +V(uint32_t a, uint32_t b, uint32_t c, uint32_t d)
> +{
> +  while (b > 0 && b < 1000) {

You should add a comment as to why we do this

Comment 5

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/dbd4f12bebc0

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 662244 [details] [diff] [review]
Consider driver version substrings decimals v2

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

::: widget/xpwidgets/GfxDriverInfo.h
@@ +210,5 @@
> +
> +  a = atoi(aStr);
> +  b = atoi(bStr);
> +  c = atoi(cStr);
> +  d = atoi(dStr);

We want the locale-dependent behaviour of atoi here?

Comment 7

[JP Rosevear [:jpr]]

Copying the tracking flag from the dupe.

Comment 8

[Alex Keybl [:akeybl]]

We think we've got Win8 driver blocklisting in an OK state for FF16, but given https://bugzilla.mozilla.org/show_bug.cgi?id=744672#c14, we may still want to uplift for FF17 rather than waiting another 6 weeks.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Nomination for uplift would be great so we can assess the risk to uplifting now while we're on Aurora.

Comment 10

[Bas Schouten (:bas.schouten)]

(In reply to :Ms2ger from comment #6)
> Comment on attachment 662244 [details] [diff] [review]
> Consider driver version substrings decimals v2
> 
> Review of attachment 662244 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/xpwidgets/GfxDriverInfo.h
> @@ +210,5 @@
> > +
> > +  a = atoi(aStr);
> > +  b = atoi(bStr);
> > +  c = atoi(cStr);
> > +  d = atoi(dStr);
> 
> We want the locale-dependent behaviour of atoi here?

Probably not. Fed plain numerics will this ever matter though?

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Bas - what's the status of this right now? We've only got two more Betas to consider taking more speculative fixes on if this is going to ship in FF 17.  Please nominate for uplift if that's an option here with a risk assessment.

Comment 12

[Joe Drew (not getting mail)]

Comment on attachment 662244 [details] [diff] [review]
Consider driver version substrings decimals v2

[Approval Request Comment]
User impact if declined: inability to correctly block AMD driver versions
Testing completed (on m-c, etc.): on m-c and aurora
Risk to taking this patch (and alternatives if risky): could potentially break driver version comparison, but likely not
String or UUID changes made by this patch: none

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2584cac54ea7