simplify temp file generation

[weltkante]

Fixes #5717

Proposed changes

• remove custom temp file name generation. It was using a newly allocated image handle to seed a random number generator, which is both unnecessary overhead and was leaking the image handle.

Customer Impact

• no more image handle leaks when assigning background images to list view

Regression?

• No

Risk

• probably none, unless someone relied on the temp file name patterns

Before

• leaking ~3 GDI handles every time the background images was assigned

After

• no image handle leak

Test methodology

• manual testing and observing GDI object count in task manager

Microsoft Reviewers: Open in CodeFlow

[weltkante]

This was the handle that leaked.

[RussKie]

Legend! 👍

[RussKie]

It would be great if we somehow could come up with an automated test for this.

@merriemcgaw @dreddy-work I think this is a servicing candidate.

[weltkante]

It would be great if we somehow could come up with an automated test for this.

I mean, once this code is removed its very unlikely to ever get reintroduced and regress ... personally I don't think a regression test for this specific issue has much value, but it would be possible

The problem with testing GDI handle leaks is that you need to make your tests single-threaded, since (as far as I know) there exist only an API to retrieve the number of allocated GDI objects is for the whole process not for an individual UI thread. So it would probably just be a performance burden on the CI with little actual value.

[dreddy-work]

Cool. I was wondering why the original implementation went that complex route. Only thing that come to my mind is, will GetTempFilename ever conflict (as duplicate/file already exist) in winforms scenarios?

We can take this to Release/6.0 right away and can request for 5.0*. @weltkante , Can you please create a PR against release/6.0 branch as well?

[kirsan31]

A small note:
Path.GetTempFile limits to 65535 files.
But this old monstrous algorithm: 65535 per rnd. num. from handle.

So potentially we add IOException here.

[merriemcgaw]

This is fantastic! I don't think it's a servicing candidate at this point unless we have customers who are actively complaining and blocked by the old way of managing this. However, I'm really happy we have this in hand to make sure that if/when that does happen we're able to address the situation right away.

[weltkante]

Can you please create a PR against release/6.0 branch as well?

Wil do.

Path.GetTempFile limits to 65535 files.

Thats a normal limitation, the native API uses the same limitation. I'd say if thats not enough it should be raised against the .NET runtime and not have WinForms do a special-purpose implementation. (WinForms does delete old files so I don't think there's high pressure being added on the temp file namespace.)

But this old monstrous algorithm: 65535 per rnd. num. from handle.

The "random name" that is formed is used as a prefix for the native temp file function, which only picks three digits and has capacity for 65535 files per prefix (see link above)

Only thing that come to my mind is, will GetTempFilename ever conflict (as duplicate/file already exist) in winforms scenarios?

So potentially we add IOException here.

Normally you don't have to concern yourself with that. If you'd have to then every .NET application would have to, meaning the system library has a useless implementation. I'd prefer raising an issue against the runtime if this ever proves unstable, so everyone else can profit as well.

If you are still concerned about temp file issues we should just pass the HBITMAP handle over instead of saving & loading a file. I kept the PR intentionally simple to just fix the issue and not potentially introduce new ones, but to me it looks like its not necessary to write the image to a file if the Windows API can just take a HBITMAP handle. (I experimented with it but transparency was handled differently between saving to bmp vs. passing a handle, and I didn't want to risk regressions here. If a handle is passed it needs investigation about whether the ListView can be made supporting transparency, it does seem to have a flag for it at the very least, but I couldn't get it to work in my first try. I'll create a follow-up issue.)

[kirsan31]

The "random name" that is formed is used as a prefix for the native temp file function, which only picks three digits and has capacity for 65535 files per prefix

exactly.

[JeremyKuhne]

Nice!

It would be nice if we could get away with HBITMAP here and avoid all of this IO. We might be able to as we're saving in the bitmap format? When you send a bitmap you're just getting a CreatePatternBrush with the bitmap. With a file, this uses the MSHTML IImgCtx. We'd want to check against different Image types to confirm that things still work as expected. My biggest question would be around transparency- which I'm not sure we retain when we save a bitmap anyway.

If we really want to avoid conflict we could just get the first three characters from Path.GetRandomFileName() and still call the API directly. It would help perf when people are heavily using Path.GetTempFileName() and/or not cleaning up their files as it wouldn't have to walk through every existing "tmp*" file to find a unique one. Better yet, we could create a subfolder with Path.GetRandomFileName() and stop tracking every single file we create, and just delete the folder.

[weltkante]

My biggest question would be around transparency- which I'm not sure we retain when we save a bitmap anyway.

In my quick first test run transparent pixels turn up black currently (i.e. transparent background doesn't seem to be working at the moment), but naive HBITMAP conversion uses a different background color to fill transparent pixels. Considering the ListView seems to have flags around transparency this needs additional investigation to see if we can do better and actually support transparency. Opening #5755 as follow-up

[RussKie]

The problem with testing GDI handle leaks is that you need to make your tests single-threaded, since (as far as I know) there exist only an API to retrieve the number of allocated GDI objects is for the whole process not for an individual UI thread. So it would probably just be a performance burden on the CI with little actual value.

I remembered I've written a test for GDI handle leaks in NativeImageList_finalizer_releases_native_handle in #4952. I do agree though that once fixed this test may a very low value.

I don't think it's a servicing candidate at this point unless we have customers who are actively complaining and blocked by the old way of managing this.

#5717 is a customer complaint.