Closed Bug 1452740 Opened 6 years ago Closed 6 years ago

Policy: Disable Hardware Acceleration

Categories

(Firefox :: Enterprise Policies, defect, P1)

Product:
Firefox
Component:
Enterprise Policies
Version:
60 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox-esr60 --- verified
firefox62 --- verified

People

(Reporter: jhauger, Assigned: kanika16047)

References

Details

Attachments

(1 file)

Bug 1452740 - Added Policy: Disable Hardware Acceleration
59 bytes, text/x-review-board-request
Felipe
: review+
RyanVM
: approval-mozilla-esr60+
Details 
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36

Steps to reproduce:

The option to "Disable Hardware Acceleration" is not available in the .admx files for Windows group policy.


Actual results:

The option does not exist.


Expected results:

The option should exist.
This does seem like somethigng that comes up reasonable often and it's just a pref switch.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 59 Branch → 60 Branch
Component: Untriaged → Enterprise Policies
Summary: Add "Disable Hardware Acceleration" to .admx Options → Policy: Disable Hardware Acceleration
Priority: -- → P3
Assignee: nobody → kanika16047
Status: NEW → ASSIGNED
Priority: P3 → P1
This looks great. You should also add a test.

Felipe built a framework to make it easy to test for simple preferences.

Check out:

https://searchfox.org/mozilla-central/source/browser/components/enterprisepolicies/tests/browser/browser_policies_simple_pref_policies.js
One more thought. Google has a policy for naming policies. We never really had a policy when we created our initial set and I'm worried the Disabled section is going to get overloaded :).

We should check out:

https://www.chromium.org/developers/how-tos/enterprise/adding-new-policies

And see if there's any good advice on naming.
This pref layers.acceleration.disabled is ready by the graphics code very early on and can't be changed afterwards, so I think onBeforeUIStartup here is too late to read it. This will probably need to be onBeforeAddons.

To verify that this is working, you should test this policy and look in about:support, under the Graphics section, to see if acceleration is enabled.
(In reply to :Felipe Gomes (needinfo me!) from comment #5)
> This pref layers.acceleration.disabled is ready by the graphics code very

is *read
Comment on attachment 8975824 [details]
Bug 1452740 - Added Policy: Disable Hardware Acceleration

https://reviewboard.mozilla.org/r/244022/#review250026


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/enterprisepolicies/tests/browser/browser_policies_simple_pref_policies.js:66
(Diff revision 2)
>  
> +  // POLICY: DisableHardwareAcceleration
> +  {
> +    policies: { "DisableHardwareAcceleration": true },
> +    lockedPrefs: { "layers.acceleration.disabled": true },
> +  },  

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]
(In reply to Mike Kaply [:mkaply] from comment #4)
> One more thought. Google has a policy for naming policies. We never really
> had a policy when we created our initial set and I'm worried the Disabled
> section is going to get overloaded :).
> 
> We should check out:
> 
> https://www.chromium.org/developers/how-tos/enterprise/adding-new-policies
> 
> And see if there's any good advice on naming.

Oh, that's right! I will surely go over the link and come up with a better name for this. :)
(In reply to :Felipe Gomes (needinfo me!) from comment #5)
> This pref layers.acceleration.disabled is ready by the graphics code very
> early on and can't be changed afterwards, so I think onBeforeUIStartup here
> is too late to read it. This will probably need to be onBeforeAddons.
> 
> To verify that this is working, you should test this policy and look in
> about:support, under the Graphics section, to see if acceleration is enabled.

I wanted to test the current implementation, that is, how it performs with onBeforeUIStartup and explored about:support.
I could see layers.acceleration.disabled to be true in the Important Locked Preferences section but couldn't understand what to look for in the Graphics section. Can you please help?
Ah sorry, I should have given more details. So the Graphics section has a lot going on, and it varies by platform. I tested on Mac, and one certain indication is that here, when hardware acceleration is on, I get:

 Compositing: OpenGL

and if it's off, it's 

 Compositing: Basic


There's also some useful information in the sub-section "Decision Log" which tells you if it was disabled by the preference.

You should disable hardware acceleration through normal means (i.e, change it in the about:preferences and restart) and compare to see if the policy code has the same effect
According to the Chromium naming policy, naming this policy HardwareAccelerationBlocked would be better than naming it HardwareAccelerationDisabled as it says "Negative policies (*Disable*, *Disallow*) are verboten because setting something to "true" to disable it confuses people."

Please let me know if that works for us.


Also, on my system, I'm not able to find any difference in the Graphics Section. In both the cases (when the policy is disabled or enabled), Compositing value is Basic and in the Decision Log, the value remains as the following.

HW_COMPOSITING - blocked by default: Acceleration blocked by platform
OPENGL_COMPOSITING - unavailable by default: Hardware compositing is disabled
What about using HardwareAccelerationEnabled and you set it to false if you don't want hardware acceleration?

And it defaults to true?
(In reply to kanika16047 from comment #13)
> According to the Chromium naming policy, naming this policy
> HardwareAccelerationBlocked would be better than naming it
> HardwareAccelerationDisabled as it says "Negative policies (*Disable*,
> *Disallow*) are verboten because setting something to "true" to disable it
> confuses people."

I like the HardwareAccelerationBlocked proposal.  We have a bunch of policies already using "Block", but we started them with Block instead of putting it in the end.. so they all ended grouped together.

Maybe moving forward we should start using this proposal

> Also, on my system, I'm not able to find any difference in the Graphics
> Section. In both the cases (when the policy is disabled or enabled),
> Compositing value is Basic and in the Decision Log, the value remains as the
> following.

Ok, let me test the patch on my computer
(In reply to :Felipe Gomes (needinfo me!) from comment #15)
> Maybe moving forward we should start using this proposal
> 
> > Also, on my system, I'm not able to find any difference in the Graphics
> > Section. In both the cases (when the policy is disabled or enabled),
> > Compositing value is Basic and in the Decision Log, the value remains as the
> > following.
> 
> Ok, let me test the patch on my computer

I tested and onBeforeAddons is indeed necesssary to make this work.

Since the timing is important for this feature, we should have a real functionality test instead of a simple pref test in browser_policies_simple_pref_policies.js

Since layers.acceleration.disabled does not respond dynamically (only the first value after startup is used), you'll need to make a test like the ones in the sub-folders of https://searchfox.org/mozilla-central/source/browser/components/enterprisepolicies/tests/browser  (for example, the disable_forget_button folder). 

And then you set up the policy and check that the layerManagerType obtained from nsIDOMWindowUtils is "Basic".

For reference, this is the code used in about:support:
https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/toolkit/modules/Troubleshoot.jsm#370
For the naming, actually let's attempt to stop using Disabled, Enabled, Blocked when possible, and just use the feature name, like so:

"HardwareAcceleration": true   ->  enabled, do nothing

"HardwareAcceleration": false  ->  disabled, set the pref to disable it
Comment on attachment 8975824 [details]
Bug 1452740 - Added Policy: Disable Hardware Acceleration

https://reviewboard.mozilla.org/r/244022/#review250680
Attachment #8975824 - Flags: review?(felipc) → review+
Ah actually, I noticed one small prob: with the renaming to HardwareAcceleration, the alphabetical position in policies-schema.json got wrong (I see that in Policies.jsm it's already correct)..

There's the test browser_policies_sorted_alphabetically.js which will fail in this case..  So r+ with that fixed!
Comment on attachment 8975824 [details]
Bug 1452740 - Added Policy: Disable Hardware Acceleration

https://reviewboard.mozilla.org/r/244022/#review250692

Great work on this patch, Kanika! One note - see below.

::: commit-message-646c8:1
(Diff revision 5)
> +Bug 1452740 - Added Policy: Disable Hardware Acceleration r?Felipe Gomes

Hi Kanika, one last thing - you don't need to put felipe's full name in as the reviewer flag. MozReview expects the reviewer ID to match their IRC nick ("felipe", in this case).

So I think it'd more match convention for the commit message to be:

"Bug 1452740 - Added Policy: Disable Hardware Acceleration r?felipe"
(In reply to :Felipe Gomes (needinfo me!) from comment #21)
> Ah actually, I noticed one small prob: with the renaming to
> HardwareAcceleration, the alphabetical position in policies-schema.json got
> wrong (I see that in Policies.jsm it's already correct)..
> 
> There's the test browser_policies_sorted_alphabetically.js which will fail
> in this case..  So r+ with that fixed!

Fixed! Thank you. :)
(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos / reviews) from comment #22)
> Comment on attachment 8975824 [details]
> Bug 1452740 - Added Policy: Disable Hardware Acceleration
> 
> https://reviewboard.mozilla.org/r/244022/#review250692
> 
> Great work on this patch, Kanika! One note - see below.
> 
> ::: commit-message-646c8:1
> (Diff revision 5)
> > +Bug 1452740 - Added Policy: Disable Hardware Acceleration r?Felipe Gomes
> 
> Hi Kanika, one last thing - you don't need to put felipe's full name in as
> the reviewer flag. MozReview expects the reviewer ID to match their IRC nick
> ("felipe", in this case).
> 
> So I think it'd more match convention for the commit message to be:
> 
> "Bug 1452740 - Added Policy: Disable Hardware Acceleration r?felipe"

Oh, thank you! Will remember that.
(In reply to kanika16047 from comment #25)
> Oh, thank you! Will remember that.

No problem! :D
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3da8ba2cf90e
Added Policy: Disable Hardware Acceleration r=Felipe
https://hg.mozilla.org/mozilla-central/rev/3da8ba2cf90e
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Comment on attachment 8975824 [details]
Bug 1452740 - Added Policy: Disable Hardware Acceleration

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Policy Changes
Fix Landed on Version: Firefox 62
Risk to taking this patch (and alternatives if risky): Low - has tests, policy only.
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8975824 - Flags: approval-mozilla-esr60?
Comment on attachment 8975824 [details]
Bug 1452740 - Added Policy: Disable Hardware Acceleration

Policy engine fix needed on ESR60. Approved for ESR 60.2.
Attachment #8975824 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Flags: qe-verify+
Blocks: 1479870
This bug was covered by the overall testing efforts invested in the New Enterprise Policies feature.

Marking this as verified fixed using Firefox 62.0b16 (BuildId:20180809104529) and the ESR try build (provided in comment 31) on Windows 10 64bit and macOS 10.13.4
Status: RESOLVED → VERIFIED
status-firefox62: fixed → verified
status-firefox-esr60: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: