Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Alter blacklisting logic to consider substrings 'decimals'

RESOLVED FIXED in Firefox 17

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bas, Assigned: bas)

Tracking

unspecified
mozilla18
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(firefox16-, firefox17+ fixed, firefox18 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
Attachment #661840 - Flags: review?(joe)
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.
Attachment #661840 - Flags: review?(joe) → review+
(Assignee)

Comment 2

5 years ago
Created attachment 662244 [details] [diff] [review]
Consider driver version substrings decimals v2

Updated to properly interpret the hardcoded blacklist.
Attachment #661840 - Attachment is obsolete: true
Attachment #662244 - Flags: review?(joe)
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
Attachment #662244 - Flags: review?(joe) → review+
Duplicate of this bug: 744672
https://hg.mozilla.org/mozilla-central/rev/dbd4f12bebc0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
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

5 years ago
Copying the tracking flag from the dupe.
tracking-firefox16: --- → +

Comment 8

5 years ago
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.
tracking-firefox16: + → -
tracking-firefox17: --- → +
Nomination for uplift would be great so we can assess the risk to uplifting now while we're on Aurora.
status-firefox17: --- → affected
status-firefox18: --- → fixed
(Assignee)

Comment 10

5 years ago
(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
Attachment #662244 - Flags: approval-mozilla-beta?
Attachment #662244 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/2584cac54ea7
status-firefox17: affected → fixed
Depends on: 822804
You need to log in before you can comment on or make changes to this bug.