Closed Bug 1274635 Opened 4 years ago Closed 4 years ago

Clean up the usage of "All" OS for graphics blocklists, downloadable and otherwise

Categories

(Core :: Graphics, defect)

Unspecified
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: milan, Assigned: milan)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

Today we use "All" (DRIVER_OS_ALL) in somewhat dangerous ways within blocklisting.  In the downloadable blocklist, we often specify "All" when we mean "All Windows", and count on a feature being Windows specific (g144), sometimes on devices being Windows specific (g1068.)  Since we only look at the actual driver versions on Windows and Android, we run into bug 1274152 if we try to make it driver version specific, Windows only, but specify "All".

In the compiled code, we use DRIVER_OS_ALL in the platform specific file - which ends up meaning "All Windows" or "All Mac", but if the code was to move to a non-platform specific file, there is a danger of wrong things getting blocked.

We have to consider the backward compatibility of the downloadable blocklist file, way beyond the currently supported version of Firefox.
"Unknown" OS entries are skipped, so we can't introduce new string values to the blocklist.
Assignee: nobody → milan
Blocks: 1274152
Whiteboard: [gfx-noted]
No longer blocks: 1274152
Depends on: 1274152
Attached patch WIPSplinter Review
Attachment #8755004 - Flags: feedback?(jmuizelaar)
Comment on attachment 8755004 [details] [diff] [review]
WIP

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

I like. Let's keep the old OS X versions in though just to be conservative.
Attachment #8755004 - Flags: feedback?(jmuizelaar) → feedback+
Comment on attachment 8758021 [details]
Bug 1274635: Have OperatingSystem an enum class, change All to mean All Windows.

https://reviewboard.mozilla.org/r/56382/#review52982

::: widget/GfxDriverInfo.h:67
(Diff revision 1)
>  };
>  
> +inline bool
> +MatchingOperatingSystems(OperatingSystem aBlockedOS, OperatingSystem aSystemOS)
> +{
> +  MOZ_ASSERT(aSystemOS != OperatingSystem::Unknown &&

It's not clear to me why we want this to assert is I call MatchingOperatingSystems(OperationgSystem::Android, mySystem). Feels like 'aSystemOS == aBlockedOS' would just work. I feel like people reading this code will also wonder this so it should be obvious from reading the code why this is important. A comment is probably a good idea.

::: widget/GfxDriverInfo.h:78
(Diff revision 1)
> +    return false;
> +  }
> +
> +#if defined (XP_WIN)
> +  if (aBlockedOS == OperatingSystem::Windows) {
> +    // We do want even "unknown" aSystemOS to fall under "all windows"

Perhaps we should handle Unknown outside of this function. We're not well equipped to know how this return value is used and thus it's unclear if we if the safe thing is to return true, or to return false.

I'd say maybe it's better to assert that the OS is not unknown here and have the caller check. We probably want to blacklist just in case if we can't determine the OS to prevent missing a blacklist rule and crashing.

::: widget/GfxInfoBase.cpp:245
(Diff revision 1)
>    Preferences::ClearUser(SUGGESTED_VERSION_PREF);
>  }
>  
>  
>  static OperatingSystem
>  BlacklistOSToOperatingSystem(const nsAString& os)

GTest could be good candidate if we want unit coverage for this. But it's fairly simple so probably not worth it.

::: widget/cocoa/GfxInfo.mm:358
(Diff revision 1)
>        // See bug 1249659
> -      if (os > DRIVER_OS_OS_X_10_7) {
> -        *aStatus = nsIGfxInfo::FEATURE_STATUS_OK;
> -      } else {
> +      switch(os) {
> +        case OperatingSystem::OSX10_5:
> +        case OperatingSystem::OSX10_6:
> +        case OperatingSystem::OSX10_7:
> +        case OperatingSystem::OSX10_8:

This is now blocking 10.8. Feels wrong to have this change ride-along this patch. But we did just deprecrate 10.8 so it's probably ok if this was intentional.
Attachment #8758021 - Flags: review?(bgirard)
Thanks for the quick review!  I'll post a follow up patch, here's some comments.

(In reply to Benoit Girard (:BenWa) from comment #5)
> Comment on attachment 8758021 [details]
> MozReview Request: Bug 1274635: Have OperatingSystem an enum class, change
> All to mean All Windows. r?benwa
> 
> https://reviewboard.mozilla.org/r/56382/#review52982
> 
> ::: widget/GfxDriverInfo.h:67
> (Diff revision 1)
> >  };
> >  
> > +inline bool
> > +MatchingOperatingSystems(OperatingSystem aBlockedOS, OperatingSystem aSystemOS)
> > +{
> > +  MOZ_ASSERT(aSystemOS != OperatingSystem::Unknown &&
> 
> It's not clear to me why we want this to assert is I call
> MatchingOperatingSystems(OperationgSystem::Android, mySystem). Feels like
> 'aSystemOS == aBlockedOS' would just work. I feel like people reading this
> code will also wonder this so it should be obvious from reading the code why
> this is important. A comment is probably a good idea.

Adding the comment, and I'll remove the "aSystemOS is not unknown" - I think the code actually allows for that and handles.  The other two in the assert stay, but with a better explanation as to why.

> 
> ::: widget/GfxDriverInfo.h:78
> (Diff revision 1)
> > +    return false;
> > +  }
> > +
> > +#if defined (XP_WIN)
> > +  if (aBlockedOS == OperatingSystem::Windows) {
> > +    // We do want even "unknown" aSystemOS to fall under "all windows"
> 
> Perhaps we should handle Unknown outside of this function. We're not well
> equipped to know how this return value is used and thus it's unclear if we
> if the safe thing is to return true, or to return false.
> 
> I'd say maybe it's better to assert that the OS is not unknown here and have
> the caller check. We probably want to blacklist just in case if we can't
> determine the OS to prevent missing a blacklist rule and crashing.

Keeping the original logic, which is "unknown OS in the blocklist blocks nothing".  However, with this logic extracted to a separate function, this is not as obvious, and it could be dangerous, as the function is freely available.  I will leave it as a function, but I'll put it back in the CPP file, only to be called in one place, making it clear what the caller expects.

> 
> ::: widget/GfxInfoBase.cpp:245
> (Diff revision 1)
> >    Preferences::ClearUser(SUGGESTED_VERSION_PREF);
> >  }
> >  
> >  
> >  static OperatingSystem
> >  BlacklistOSToOperatingSystem(const nsAString& os)
> 
> GTest could be good candidate if we want unit coverage for this. But it's
> fairly simple so probably not worth it.

Good idea, I may do a followup bug.

> 
> ::: widget/cocoa/GfxInfo.mm:358
> (Diff revision 1)
> >        // See bug 1249659
> > -      if (os > DRIVER_OS_OS_X_10_7) {
> > -        *aStatus = nsIGfxInfo::FEATURE_STATUS_OK;
> > -      } else {
> > +      switch(os) {
> > +        case OperatingSystem::OSX10_5:
> > +        case OperatingSystem::OSX10_6:
> > +        case OperatingSystem::OSX10_7:
> > +        case OperatingSystem::OSX10_8:
> 
> This is now blocking 10.8. Feels wrong to have this change ride-along this
> patch. But we did just deprecrate 10.8 so it's probably ok if this was
> intentional.

Not intentional.  Nice catch.
(In reply to Milan Sreckovic [:milan] from comment #6)
> Keeping the original logic, which is "unknown OS in the blocklist blocks
> nothing". 

Knowing the blocklist code I'm fine with minimizing functional changes. So that's fine.
(In reply to Benoit Girard (:BenWa) from comment #7)
> (In reply to Milan Sreckovic [:milan] from comment #6)
> > Keeping the original logic, which is "unknown OS in the blocklist blocks
> > nothing". 
> 
> Knowing the blocklist code I'm fine with minimizing functional changes. So
> that's fine.

Yeah, we don't really have much of a choice because the downloadable blocklists are for all versions.  So, when OS X 10.12 shows up, and we have to block something on it in the downloadable list, and we run Firefox 49 that doesn't know about 10.12, and would tag it as "unknown", we'd end up blocking things in 49 we didn't mean to.
Comment on attachment 8758021 [details]
Bug 1274635: Have OperatingSystem an enum class, change All to mean All Windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56382/diff/1-2/
Attachment #8758021 - Flags: review?(bgirard)
Comment on attachment 8758021 [details]
Bug 1274635: Have OperatingSystem an enum class, change All to mean All Windows.

https://reviewboard.mozilla.org/r/56382/#review52988

::: widget/GfxInfoBase.cpp:638
(Diff revisions 1 - 2)
> +    return false;
> +  }
> +
> +#if defined (XP_WIN)
> +  if (aBlockedOS == OperatingSystem::Windows) {
> +    // We do want even "unknown" aSystemOS to fall under "all windows"

This seems reasonable to me. But wont it be odd that win/osx are the only platform where this holds? Feels like a gotcha that might be missed and cause a bug later.
Attachment #8758021 - Flags: review?(bgirard) → review+
(In reply to Benoit Girard (:BenWa) from comment #10)
> > +#if defined (XP_WIN)
> > +  if (aBlockedOS == OperatingSystem::Windows) {
> > +    // We do want even "unknown" aSystemOS to fall under "all windows"
> 
> This seems reasonable to me. But wont it be odd that win/osx are the only
> platform where this holds? Feels like a gotcha that might be missed and
> cause a bug later.

Only in the context that those are the only two platforms that have the "all" versions available - there is only one Linux and one Android.  But, you're right; if we were to introduce, say, BSD, and then have "*nix" we'd have to add an entry for Linux & BSD to fall under it.  Android, I suppose as well.
Comment on attachment 8758021 [details]
Bug 1274635: Have OperatingSystem an enum class, change All to mean All Windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56382/diff/2-3/
Attachment #8758021 - Attachment description: MozReview Request: Bug 1274635: Have OperatingSystem an enum class, change All to mean All Windows. r?benwa → Bug 1274635: Have OperatingSystem an enum class, change All to mean All Windows.
Pushed by msreckovic@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af65533093de
Have OperatingSystem an enum class, change All to mean All Windows. r=BenWa
https://hg.mozilla.org/mozilla-central/rev/af65533093de
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.