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 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.

Updated to properly interpret the hardcoded blacklist.

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 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?

Copying the tracking flag from the dupe.

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.

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

(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?

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 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