1245719 - Set HKCU\SOFTWARE\Microsoft\Windows\CurrentVersion\Explorer\Advanced\HideIcons to true on Windows test slaves

Status: RESOLVED FIXED

(Infrastructure & Operations :: RelOps: General, task)

Description

[Matthew N. [:MattN]]

mochitest-browser-screenshots test suite takes screenshots of the application in order to detect UI regressions in Firefox (among other things). Detecting regressions requires that screenshots not have unrelated changes in them but currently the desktop icons on Window test machines are causing false-positives.

For various reasons, capturing only the Fx window is non-trivial and sometimes undesired (when we want to capture multiple overlapping HWND) so the simplest solution is to toggle the registry key for the "Hide Desktop Icons" feature in the desktop context menu.

Could the following registry change please be deployed to all Windows test machines? Please ensure that this is loaded in the user that is actually used to run the tests (e.g. cltbld).

[HKEY_CURRENT_USER\SOFTWARE\Microsoft\Windows\CurrentVersion\Explorer\Advanced]
"HideIcons"=dword:00000001

I believe this should work for all our supported Windows versions.

Comment 1

[Phil Ringnalda (:philor)]

Crazy as this sounds, that may very well have an impact on Talos numbers and need to be coordinated with the Talos team: back before the desktop got completely and hopelessly littered with all those pointless icons from tests spewing, we were yelling at people whose tests spewed icons there because we felt that the presence or absence of icons affected Talos numbers.

Comment 2

[Matthew N. [:MattN]]

Perhaps we can differentiate between unit test and talos slaves and only deploy this to unit test slaves?

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

with our current setup it is all the same pool.  In the future we might be able to do that, in fact I think that would really help.

As it stands for this current bug, we could easily do this and allow for the numbers to re-baseline.

Comment 4

[Dustin J. Mitchell [:dustin] (he/him)]

Implicit in this is the promise that no pixel on the screen will ever change.  That is a promise we should not make lightly, as it may make it impossible to perform other changes on these systems.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

good point!  right now it is almost guaranteed to be different per machine, but for the small number of changes we make to the core OS/desktop it seems as though we could live with that.  I assume it is only a few times/year.

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

Maybe - we'll see how often it changes, and rebase the regression tests when it does -- and if it's too often, consider means of ignoring areas of the secreen, or something like that.

Comment 7

[Matthew N. [:MattN]]

(In reply to Dustin J. Mitchell [:dustin] from comment #4)
> Implicit in this is the promise that no pixel on the screen will ever
> change.  That is a promise we should not make lightly, as it may make it
> impossible to perform other changes on these systems.

I don't think this change implies that at all… it's just making it much easier for me to do comparisons on a Tier 3 job. Tier 3 means you're not going to have a mob of people mad if this tool breaks. Note that automation simply does the capturing at the moment, not the comparison, so even if some visual elements change on the machine it's not going to make some job go orange/red on TreeHerder, it's just going to make comparison harder. If there is an intentional change in the future, we can re-push jobs on the new configurations to use as a baseline for comparison.

Also, I already handle cropping out non-desktop parts of the images like the taskbar on Windows and the menu bar on OS X as that is fairly straightforward: cut X pixels off one side of the image based upon the OS version. I also agree with Joel that the number of changes is likely going to be small, especially when we're not counting the taskbars, menubars, desktop icons. The few variables left are probably the desktop wallpaper, dppx, OS theme, and screen resolution all of which I don't think will be changed without reason and wouldn't be a big deal for the tool if it's applied consistently for machines of the same job. 

(Quoting Matthew N. [:MattN] from comment #0)
> For various reasons, capturing only the Fx window is non-trivial and
> sometimes undesired (when we want to capture multiple overlapping HWND) so
> the simplest solution is to toggle the registry key for the "Hide Desktop
> Icons" feature in the desktop context menu.

Examples of why it's non-trivial:
* we want to capture what a user would actually see and some more targeted methods of capturing don't accurately capture the window manager portion (e.g. titlebar and window frame) which are things that we mess with in Firefox.
* there is transparency with Aero Glass (e.g. Win7) which means that desktop icons show through the window frame and titlebar meaning that simply cropping isn't sufficient to eliminate false positives

If in the future there are no other ways around differences in the test machines then I can invest a lot of time to implement workarounds but for now there is a straightforward solution which doesn't need to be committing to any future restrictions (though avoiding unnecessary changes would obviously be appreciated) so we should go ahead with it IMO, especially since I was told this was a trivial change.

If we start actually doing the comparisons (not just capture) in automation, and move this up from Tier 3 then we may want a promise regarding changes to the OS but that's premature for now IMO.

Comment 8

[Dustin J. Mitchell [:dustin] (he/him)]

Thanks for the detailed response!  I didn't realize this was Tier-3.

Comment 9

[Marco Castelluccio [:marco]]

Other than the web runtime tests, are there any other tests that create icons on the desktop?

The web runtime tests have been disabled in bug 1238576:
https://hg.mozilla.org/mozilla-central/rev/7f5e4c6bfb07#l3.1
https://hg.mozilla.org/mozilla-central/rev/7f5e4c6bfb07#l4.1
https://hg.mozilla.org/mozilla-central/rev/7f5e4c6bfb07#l33.1

So maybe this isn't necessary anymore?

Comment 10

[Dustin J. Mitchell [:dustin] (he/him)]

If it's Firefox creating the icons, then it might be good to continue to display them so that we can catch any new tests that don't clean up after themselves.  This is less important in the world of spot instances and frequent re-imaging, but even so, clean tests are good..

Comment 11

[Matthew N. [:MattN]]

(In reply to Marco Castelluccio [:marco] from comment #9)
> The web runtime tests have been disabled in bug 1238576:
> …
> So maybe this isn't necessary anymore?

I've seen files that aren't from the Web Runtime such as ones that seem manually created and .part files possibly from a download test that failed before cleanup.

(In reply to Dustin J. Mitchell [:dustin] from comment #10)
> If it's Firefox creating the icons, then it might be good to continue to
> display them so that we can catch any new tests that don't clean up after
> themselves.

The files/shortcuts will still be there on the desktop just won't be visible unless you look through explorer. Also, you will still see the icons on developer machines and on OS X (assuming the test is cross-platform) where we can capture only Firefox windows and so don't need to hide them hidden.

I still think we should do this by default, also because we think it can affect talos performance tests where consistency would be better. If we don't deploy this by default I will have to do the registry key change at runtime which may cause more problems such as not reverting it in the case of a failure.

Comment 12

[Dustin J. Mitchell [:dustin] (he/him)]

OK -- sorry I keep coming up with reasons not to do this :)

Comment 13

[Matthew N. [:MattN]]

Any update on this? I've been patiently waiting but this is still getting in the way of detecting regressions on Windows.

Comment 14

[Amy Rich [:arr] [:arich]]

From my reading, this is for tier3, which means it's in the large backlog of stuff we're working on and behind everything that's are impacting tier1. :jmaher: is this something we need to prioritize doing for tier1 (is it even desirable, based on the above conversation)?

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

Thanks for checkin in with this :arr.

I still believe we should get this done- these tests bring a lot of value to the dev teams, it is just expensive to run, and hard to make 99%+ reliable (this bug helps that) which is why we are at tier-3, possibly this could be tier-2 and run on nightly builds.

To answer the question about priorities- I am not aware of how much work is involved here to do this.  I was under the impression our windows machines were managed under active directory and we could make a change and it would be deployed.  I know things are not always that simple, but it would be nice to know if this is a LOT of manual work vs some work.  Also given the fact that we are working on AWS windows in the cloud, I would think when we bring those online they could have this reg key set properly.  Overall, I think finishing up the windows machines in the cloud work seems higher priority than this specific bug.

if we could understand more of what it takes to do this, it would help make the decision on if we should invest in having the harness set the reg key.

Comment 16

[Amy Rich [:arr] [:arich]]

:markco: is this something you could easily add?

Comment 17

[Q]

This is something I would need to do if it is to be successful in HCU

Comment 18

[Q]

Let me be more explicit. CLTBLD is a local user with varying sids and not controlled by domain policy. That being the case, it would require a script or a small bit of wmi expertise to accomplish this for just cltbld. We had a long standing goal to have a local hive iteration script to use generically however, that was abandoned since we are "moving to puppet very soon" and the domain is going away. I can make these changes today if we want to make this a priority. After looking at our xp platform we also want to cleanup the desktop more regularly in addition to this change.

Comment 19

[Amy Rich [:arr] [:arich]]

This isn't a priority over AWS, which is why I was wondering if markco could do it. If it requires you, we'll put it on the backlog until w7 is in production.

Comment 20

[Q]

The fast oses are done. Changes in for win 7 and win 8 xp will require more work.

Comment 21

[Matthew N. [:MattN]]

(In reply to Q from comment #20)
> The fast oses are done. Changes in for win 7 and win 8 xp will require more
> work.

Windows 7 (buildbot jobs) is where I still see the issue. I'm having a hard time parsing your last sentence to know where it should be fixed.

Comment 22

[Matthew N. [:MattN]]

Sorry, I meant "when", not "where"

Comment 23

[Q]

This was fixed on hardware on windows 7/8/10. Needs to be corrected via puppett in cloud slaves.

Comment 24

[Amy Rich [:arr] [:arich]]

:markco did this get updated in puppet? If so, please close this bug out.

Comment 25

[Amy Rich [:arr] [:arich]]

Rob, did this get carried over to TC?

Comment 26

[Mark Cornmesser [:markco]]

(In reply to Amy Rich [:arr] [:arich] from comment #24)
> :markco did this get updated in puppet? If so, please close this bug out.

This has not been added. I can add it early next week. 


Just to note this won't be straight forward since HKCU hive is not directly accessible by Puppet.

Comment 27

[Rob Thijssen [:grenade (EET/UTC+0300)]]

i would like to suggest that:
- system configuration is the wrong place to add registry settings for HKCU for several reasons:
  - tasks on windows taskcluster run under a freshly created user account and user profile.
  - the HKCU hive and clean task profile does not exist when the system is being configured. it's only created when the instance is ready to take on a new task.
- test configuration is the right place to change registry settings that affect a specific test. it makes it clear that in order for the test to succeed, some user configuration needs to exist. this way, someone running the tests outside of automation, can benefit from the test succeeding without having to trawl through our automation configuration to find the registry setting that will make the test pass. the test pre-flight scripts in-tree exist for this reason. see: https://dxr.mozilla.org/mozilla-central/rev/75be6742abb94d79f3bcb731ab7fafa3d42ac4da/testing/mozharness/configs/unittests/win_taskcluster_unittest.py#238-260

where a command like the following could be added:

reg add HKCU\SOFTWARE\Microsoft\Windows\CurrentVersion\Explorer\Advanced /v HideIcons /t REG_DWORD /d 1 /f

eg:
        {
            'name': 'set HideIcons to true',
            'cmd': [
                'reg', 'add', 'HKCU\\SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Explorer\\Advanced',
                '/v', 'HideIcons', '/t', 'REG_DWORD', '/d', '1', '/f'
            ],
            'architectures': ['32bit', '64bit'],
            'halt_on_failure': True,
            'enabled': True
        }

Comment 28

[Mark Cornmesser [:markco]]

I believe these are now handled by pre-flight scripts.