Last Comment Bug 791742 - Alter blacklisting logic to consider substrings 'decimals'
: Alter blacklisting logic to consider substrings 'decimals'
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla18
Assigned To: Bas Schouten (:bas.schouten)
:
Mentors:
: 744672 (view as bug list)
Depends on: 822804
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-17 10:23 PDT by Bas Schouten (:bas.schouten)
Modified: 2012-12-18 12:47 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
+
fixed
fixed


Attachments
Consider driver version substrings decimals (2.36 KB, patch)
2012-09-17 10:23 PDT, Bas Schouten (:bas.schouten)
joe: review+
Details | Diff | Splinter Review
Consider driver version substrings decimals v2 (4.88 KB, patch)
2012-09-18 11:31 PDT, Bas Schouten (:bas.schouten)
joe: review+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Bas Schouten (:bas.schouten) 2012-09-17 10:23:57 PDT
Created attachment 661840 [details] [diff] [review]
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) 2012-09-17 11:42:37 PDT
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) 2012-09-18 11:31:52 PDT
Created attachment 662244 [details] [diff] [review]
Consider driver version substrings decimals v2

Updated to properly interpret the hardcoded blacklist.
Comment 3 Joe Drew (not getting mail) 2012-09-18 12:16:36 PDT
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 4 Joe Drew (not getting mail) 2012-09-18 18:04:01 PDT
*** Bug 744672 has been marked as a duplicate of this bug. ***
Comment 5 Graeme McCutcheon [:graememcc] 2012-09-19 07:56:25 PDT
https://hg.mozilla.org/mozilla-central/rev/dbd4f12bebc0
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-09-20 02:37:52 PDT
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] 2012-09-20 06:03:29 PDT
Copying the tracking flag from the dupe.
Comment 8 Alex Keybl [:akeybl] 2012-09-20 15:15:16 PDT
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 2012-09-25 13:03:54 PDT
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) 2012-09-25 13:19:05 PDT
(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 2012-10-16 15:39:51 PDT
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) 2012-10-30 09:29:30 PDT
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] 2012-10-30 10:35:53 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/2584cac54ea7

Note You need to log in before you can comment on or make changes to this bug.